Skip to content

Made some additions to elliminate the following security issues: #2

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

Closed
wants to merge 2 commits into from

Conversation

dmcbeing
Copy link

Gia sou, Antoni.

Ime o Stelios Mavridis(mavridis@csd.uoc.gr).
Ida to mail sto ugrads, ke mou kinise tin aporia.(Opensource&&Csd => woot?)
Nwmizw pws vrika kapia thematakia security, ke pira to tharos na ta diwrthoso wste na min anoixoume 'trypes' sto idrima(arketes exi idi ;) ).
Pes mou pia ine i apopsi sou gia tis allages.

Filika Stelios.

Password was sent in plaintext over the internet, changed to use the hash instead.
Protocol was prone to replay attacks, added a nonce to counter against it.
@daknob
Copy link
Owner

daknob commented May 28, 2015

JavaScript Cryptography is known to have a lot of problems and currently few libraries are audited (they all use openssl binaries and a Binary to JavaScript executor). SHA-256.js isn't one of them AFAIK.

The script ensures that at all times the connection is over HTTPS, otherwise it drops the connection. Currently, if I send my password: "hello" or its sha256 hash doesn't really benefit malicious logins in the sense that if an attacker captures the request (which is the MitM scenario), and replays the request, both are going to let him in. The only downside is that if he captures the request in plain-text, he may see the user's password, but for most security teams in the industry this is a more acceptable solution that using JavaScript Crypto (!).. If you check the login pages of Facebook, Twitter, Google, Wordpress, ... they all avoid (last time I checked) JavaScript Hashing and send the password by trusting HTTPS and their other mechanisms.

I wanted to implement a hash check (JavaScript checks if SHA-256 of selected file in browser matches the SHA-256 of the uploaded file in the server), but didn't do it since JS Crypto isn't the best thing ever made.

Also, by uploading any number of files as another user, you do not cause any problem, in a sense that:

  • The user's quota is not affected
  • You need ssh access as the user himself to view the files
  • The files aren't executable

In terms of the session implementation I don't think it benefits somehow the solution since if someone has READ access to the file and can see the SHA-256 in the variable, he can send that in the request. Also, it may not allow you login some times if I'm not mistaken.

TL;DR
The current program uses industry-common login and since the only request submitted requires also your password, it may not have exploitable CSRF vulnerabilities. Also, it's a JS-Free Solution.. ;-)

Thanks for helping and let me know of your opinion.

@zakkak
Copy link

zakkak commented May 28, 2015

Stelios' approach also takes in account the session ID to avoid using
the transmitted hash later. In that sense he narrows the time window in
which a replay attack is plausible.

I don't agree that bypassing the log in process does not cause
problems...

  • One could deploy a DoS attack by uploading huge files.
  • At some time it would be convenient to be able to also read/download
    files from the interface
  • What if they are scripts!?

The industry-common approaches are not necessarily good (see
visa/mastercard contactless payments)

@daknob
Copy link
Owner

daknob commented May 28, 2015

If the uploaded files are scripts, then they can't be executed unless the user specifically goes and executes them using ssh. The web server cannot execute / run files from the STORAGE folder.

Also, in case of the replay attack, if the user has a successful Man In The Middle position between the Web Server and the User, despite https, then he can server any content he wants, including any JavaScript he wants, rendering the entire mechanism useless. He can simply replace the page served with the page the current script serves, and send the password in plaintext. He can even use browser exploits and hack the victim's computer.. These are problems generally solved by HTTPS and its extensions in the industry.. I don't see a way that Stelios' implementation offers any additional protection in a case the attacker has MitM position and I also don't see that in the case that the attacker doesn't have a MitM position. On the contrary, the JavaScript crypto can introduce more risks..

@zakkak
Copy link

zakkak commented May 28, 2015

OK, I checked the code and since you do not allow overwrites it looks safe.

Considering it operates always over HTTPS you are right. The only benefit I see is that in his approach he never transfers the plaintext password to the webserver.

@daknob
Copy link
Owner

daknob commented May 28, 2015

This doesn't provide any security since if you think about it, if someone controls the server, he can make the user send the plaintext password. If he doesn't, then he can't get the password.

@dmcbeing
Copy link
Author

Hello zakkak,DaKnOb.

The intention of the patch was to not send the password in plain-text and also avoid any replay attacks.
Since i am hashing the concatenation of the password hash and the nonce(session_id) there is no (easy) way a man-in-the-middle to be able to create an appropriate hash without knowing the password hash and the session_id.
The password hash can be easily read from the .php file,perhaps we could move it to a different (and protected) file?

The session id seems to be and 128-bit value(http://www.perlmonks.org/?node_id=214293).
I think that for the foreseeable future we are safe from replay attacks, since the nonce changes on every request.

" if someone controls the server":All your bases are belong to us?

ps:About the sha256.js.It was formated that way in the original source, regardless i will reformat it and update.

@zakkak
Copy link

zakkak commented May 28, 2015 via email

@daknob
Copy link
Owner

daknob commented May 28, 2015

@dmcbeing
Let's assume that an attacker has Man In The Middle position. That means he can not only listen to all the traffic, but also modify it. That is, he can remove all the JavaScript and make the user submit his plaintext password. Good guys lose, he has the password.
Let's assume that an attacker has Man In The Middle position but can only read the content of the connection (Don't know how, but whatever.. :P). That means he has access to the PHPSESSID cookie that is essentially the session identifier, since it's sent back and forth. He also has access to the contents of the session JavaScript variable. This makes the nonce useless, since the attacker already knows it.
HTTPS does a good way protecting the user against impersonation attacks / and/or data modification attacks and also prevents replay attacks since it requires a handshake.

Additionally, this is a PHP Session ID, that remains the same until the user quits their browser in most configurations, so it won't be different every time but every time the browser restarts.

@dmcbeing
Copy link
Author

@daknob I must admit that you are correct in that HTTPS is sufficient on its own.I noticed https after this discussion.
As for the session id create a new session id on every transaction, so i at least got that one right ;-).

Thanks for the feedback.

@daknob
Copy link
Owner

daknob commented May 29, 2015

Thanks for opening this discussion.. :-)

@daknob daknob closed this May 29, 2015
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.

3 participants