Harden against SQL injection attacks #38

Closed
roeme opened this Issue Jan 22, 2014 · 5 comments

Comments

3 participants
@roeme

roeme commented Jan 22, 2014

As hinted at https://news.ycombinator.com/item?id=7102109, the use of mysqli prepared statements should be considered – or, referring to http://stackoverflow.com/a/12118602/1057969, at least a warning message could be emitted when the environment where Lychee is being run is unsafe (mysql version etc).

@electerious

This comment has been minimized.

Show comment
Hide comment
@electerious

electerious Jan 22, 2014

Owner

Thanks for opening an issue for this flaw here on GitHub. I will improve this part.

Owner

electerious commented Jan 22, 2014

Thanks for opening an issue for this flaw here on GitHub. I will improve this part.

@electerious

This comment has been minimized.

Show comment
Hide comment
@electerious

electerious Feb 8, 2014

Owner

Update: The php/check.php shows a warning and Lychee uses the GBK charset when running on an older MySQL version.

Owner

electerious commented Feb 8, 2014

Update: The php/check.php shows a warning and Lychee uses the GBK charset when running on an older MySQL version.

@electerious

This comment has been minimized.

Show comment
Hide comment
@electerious

electerious Aug 16, 2014

Owner

To clarify the issue: Lychee is not open for SQL injections (as far as I can tell). All params and requests are routed though the api.php where everything will be escaped. There's a edge case with older MySQL versions where mysql_real_escape_string() won't prevent injections. This case is prevented by Lychee by using the GBK charset when running an older MySQL version.

However, using prepared statements would be even better. Changing all those MySQL statements is a bunch of work. I'm open for every help and PR.

Owner

electerious commented Aug 16, 2014

To clarify the issue: Lychee is not open for SQL injections (as far as I can tell). All params and requests are routed though the api.php where everything will be escaped. There's a edge case with older MySQL versions where mysql_real_escape_string() won't prevent injections. This case is prevented by Lychee by using the GBK charset when running an older MySQL version.

However, using prepared statements would be even better. Changing all those MySQL statements is a bunch of work. I'm open for every help and PR.

@mbartosch

This comment has been minimized.

Show comment
Hide comment
@mbartosch

mbartosch Aug 21, 2014

Well, this may or may not be the case. The point is: it is very hard to see if it is safe.

I am used to looking for certain patterns that usually indicate security problems. Potentially dangerous statements such as SQL queries and command execution using user supplied input should get some extra "developer lover" nearby, either by using special escaping functions, bound parameters or by sanity checks against parameters (e. g. regex match). It can even be a comment ("this parameter is safe, it has been sanitized in api.php, line 77").
When I see a number of potentially vulnerably statements without one single line of code nearby indicating that the developer is aware of a potential problem I am inclined to assume that the code is potentially vulnerable.

Well, this may or may not be the case. The point is: it is very hard to see if it is safe.

I am used to looking for certain patterns that usually indicate security problems. Potentially dangerous statements such as SQL queries and command execution using user supplied input should get some extra "developer lover" nearby, either by using special escaping functions, bound parameters or by sanity checks against parameters (e. g. regex match). It can even be a comment ("this parameter is safe, it has been sanitized in api.php, line 77").
When I see a number of potentially vulnerably statements without one single line of code nearby indicating that the developer is aware of a potential problem I am inclined to assume that the code is potentially vulnerable.

@electerious

This comment has been minimized.

Show comment
Hide comment
@electerious

electerious Aug 22, 2014

Owner

@mbartosch Totally agree with you. Would be great if you could help to convert statements into prepared statements.

Owner

electerious commented Aug 22, 2014

@mbartosch Totally agree with you. Would be great if you could help to convert statements into prepared statements.

electerious added a commit that referenced this issue Aug 29, 2014

electerious added a commit that referenced this issue Aug 29, 2014

electerious added a commit that referenced this issue Aug 30, 2014

@electerious electerious referenced this issue Sep 7, 2014

Merged

v2.6.2 #226

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