Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Escape data before using them in SQL #289

Merged
merged 1 commit into from
Sep 21, 2017
Merged

Escape data before using them in SQL #289

merged 1 commit into from
Sep 21, 2017

Conversation

nitriques
Copy link
Member

This commit fixes a couple of possible SQLi errors.

Fixes #286

This commit fixes a couple of possible SQLi errors.

Fixes #286
Copy link
Member

@michael-e michael-e left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know which bugs are supposed to be fixed here. Do you have any test cases for me to try?

@nitriques
Copy link
Member Author

@michael-e see #286 when Nils is searching for a user Director's cut

@nitriques
Copy link
Member Author

Try to add some ' in any input and see if it breaks.

@michael-e
Copy link
Member

Try to add some ' in any input and see if it breaks.

Even without your fix, using plain Symphony 2.6.11 and Members 1.6.0, I added ' to my Member name and was able to login nevertheless. Nothing broke. (However, the fact that my Member name was accepted doesn't feel very good, to be honest.)

I don't think that I understand all the issues well enough to do systematic testing here.

Copy link
Member

@jensscherbl jensscherbl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m currently not using the members extension in any relevant project and don’t understand the code well enough to do a proper review and testing, sorry.

@nitriques
Copy link
Member Author

No problems Jens thanks.

@nitriques
Copy link
Member Author

@michael-e Nils also had problems with publish filtering I think. Nevertheless, the code needs those fixes.

@nitriques
Copy link
Member Author

Just to make sure everybody's on the same page:

When I go in the members section, I filter by username with ', I get

image

The user does not see the end result, but can still do things like '; DROP sym_entries; --

Same thing happens with a datasource filtered by username usign {$url-q}. Issuing /?q=' crashes the datasource. The end user does not see any errors message (you have to hit /?q='&debug to see it, but it still allows a remote user to issue random SQL commands.

image

@nitriques
Copy link
Member Author

If @nilshoerrmann could confirm that it fixes his problems, that would be cool ;)

@nilshoerrmann
Copy link
Contributor

I sadly won't have time this week to look into this. Feel free to ping me again next week.

@michael-e
Copy link
Member

@nitriques Thanks for the the explanation. It sounds like a severe issue to me, because it is not limited to backend users. We should release a hotfix version soon.

@nitriques
Copy link
Member Author

Thanks @nilshoerrmann

@michael-e

It sounds like a severe issue to me

It is! I've warned everybody last June but never got the chance to actually patch it :(

@nitriques nitriques merged commit 3bbe64d into integration Sep 21, 2017
@nitriques nitriques deleted the issue-286 branch September 21, 2017 20:18
@nitriques
Copy link
Member Author

Version 1.7.0 is out

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants