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

Security Vulnerabilities #3

Merged
merged 10 commits into from Jan 29, 2016
Merged

Conversation

crackedeggs1
Copy link
Contributor

The following issues are resolved:

  • Potential (but unlikely) cross-site scripting vulnerabilities when admins manage users with unsafe characters in their names.
  • Cross-site scripting vulnerability when any user logs in by following a link posted by someone else.
  • Failed login phishing vulnerability when any user logs in by following a link posted by someone else.
  • Wrong success URL after failed login attempt. (Not a vulnerability but resolved nonetheless)
  • Resolved inconsistencies with encoding convention of stored usernames.
  • Potential SQL injection vulnerabilities involving an SQLite3 bug and NUL terminator bytes.

Notes:

  • Some users who were affected by data storage inconsistencies might require an admin to reset their password/qrcode.

Again, please review these changes carefully before merging. There are a number of significant changes rolled up here. The last thing we want is to prevent users from logging in, or to prevent the installer from creating the first admin. I'm pretty sure these are the changes I'm using in production, but I could have made a copy-paste error or there might be a use-case I've overlooked that is now broken...

This should fix an XSS vector, which here, could have been used to place Javascript which would listen for the user's name, password, and token, and send it to a remote server which would login first.
The method used to fix should also have the benefit of preserving ?from= when authentication fails (maybe the user ran out of time, or mistyped), so that after failure+success, the user is still forwarded to the 'from' address, rather than the URL named by AUTH_SUCCEED_REDIRECT_URL.
A lot of XSS was possible here if an admin had ever let users pick their own usernames
Really just a standards thing
Also refactor where code was reused.
Using cross-site ?from, the user can be presented with a fake "auth failed" page, which will trick them into logging in again. Since they are now on a different site, the site can capture their user, password, and current token in order to steal their access to the real site.
@Arno0x
Copy link
Owner

Arno0x commented Jan 28, 2016

oh my godness... I'm learning so much looking at your code enhancements and fixes ! Many PHP syntax tricks I was not even aware of. There's also some few things I don't quite understand why you do it this way (the sqlQuery stuff) rather than my way.

I had a quick overview of your changes and it seems fine. I will however take some further time to learn from it and check what these changes exactly do (hopefully will tell the "why" you're doing it this way).

I'll then merge, probably by the end of the week. Just one question though: how can I test this branch of changes in my environment without merging on the master branch ?

Many thanks for your contribution. (makes me feel like I should stop coding and learn how to do things properly :-)

@crackedeggs1
Copy link
Contributor Author

I'm not sure I'm not sure how to do it in git, but you can download the branch and test it separately like that.

Don't worry about it. You have no idea how often I find things like this in my own code. I've got a few more fixes to add here in general, but still trying to learn how github deals with branches, etc. My background is in mercurial, so everything is a little different here.

Regarding the sqlQuery stuff: your way worked fine and only had some minor issues (like trusting the safety of passwordhash and gauthsecret), but there is a known bug in one of the functions in the Sqlite3 library when certain characters were in the data, so we have to do it a slightly different way to prevent errors (maybe security related) if those characters ever make it to the query.


// Prepare variables before the query
$username = SQLite3::escapeString ($username);
Copy link
Owner

Choose a reason for hiding this comment

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

Just curious why removing the SQLite3::escapeString function as it supposedly adds extra security with potential characters specific to sqlite queries that might appear in the username while not being already escaped by the previous sanitization performed with htmlspecialchars ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I mentioned, there is a bug in the SQLite3 library: https://bugs.php.net/bug.php?id=62361
Also see: http://security.stackexchange.com/questions/3611/sql-injection-why-isnt-escape-quotes-safe-anymore

So considering both factors, I replaced all the escapeString calls with prepared statements, which don't need to be escaped and are more secure.

Copy link
Owner

Choose a reason for hiding this comment

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

Oh, OK, so that was that line you were referring to when mentioning the SQLite3 library bug.
So all the commits you're proposing are good with me: I tested the whole branch in a test environment, went through all use-cases and it's seems all right. I'll merge now. Thanks for all those fixes, your time, and your contribution.

Arno0x added a commit that referenced this pull request Jan 29, 2016
@Arno0x Arno0x merged commit 4353544 into Arno0x:master Jan 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants