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

Insecure password hashing: salt needed #2521

Closed
3 tasks done
nbros652 opened this issue Nov 26, 2018 · 9 comments
Closed
3 tasks done

Insecure password hashing: salt needed #2521

nbros652 opened this issue Nov 26, 2018 · 9 comments

Comments

@nbros652
Copy link

In raising this issue, I confirm the following: {please fill the checkboxes, e.g: [X]}

How familiar are you with the the source code relevant to this issue?:

8


Expected behaviour:

When creating a password for the web interface, the password should be hashed with a salt to protect against rainbow table attacks. Changing this salt on each password change would probably be a good idea as well. This salt value should be stored in /etc/pihole/setupVars.conf along with the password hash so that passwords can be rehashed for authentication.

Actual behaviour:

Reviewing pi-hole/advanced/Scripts/webpage.sh reveals in the HashPassword function at lines 86-91 (latest commit of file on master branch as of reporting: b54e32f), that passwords are simply hashed twice. While a comment indicates that the intent is to "avoid rainbow table vulnerability," multiple hash iterations alone is insufficient protection against rainbow table attacks! Multiple iterations is good and should remain.

Steps to reproduce:

Create a web console password.

Other affected repositories/files:

Fixing this behaviour will require changes to the repository that houses the web interfaces as well, namely pi-hole/AdminLTE. In particular, it is evident that the AdminLTE/scripts/pi-hole/php/password.php file would need to be updated around line 61 (latest commit of file on master branch as of reporting: 96039f5).

Debug token provided by uploading pihole -d log:

N/A

Troubleshooting undertaken, and/or other relevant information:

N/A

@DL6ER
Copy link
Member

DL6ER commented Nov 26, 2018

The password is never stored in plaintext anywhere. The procedure is that we hash the password twice on the client and only send this hash back to the server where it is compared to the local hash. How would you like to see this changed exactly, e.g., how should the client get informed about the salt? Furthermore, this could work only for future password changes as the fact that noone (except the user in his head) knows the password, prevents us from doing an automated re-encoding.

@nbros652
Copy link
Author

You are right, but as of right now the password is only hashed twice. This is insufficient in guarding against rainbow table attacks. A salt is not something that is ever revealed to the end user. It is a stored random value that is hashed with the users password to prevent two users with the same password from having the same password hash, thus guarding against rainbow table attacks. As the code is written now, rainbow tables could be generated for all possible password hashes of a given simply by hashing all possible passwords of a given length twice. There's a really good explanation here.
I've never generated a pull request, but I'm working on the modifications I've described now. If I can get them working in the development branch, I will generate pull requests in both the pi-hole and AdminLTE repos.

@DL6ER
Copy link
Member

DL6ER commented Nov 26, 2018

A salt is not something that is ever revealed to the end user.

You misunderstood me. There is no secure way of transporting the password over HTTP so we need to do the cryptographic work in the user's browser (in Javascript) before we send it over the network. This is what I meant with the "client".

As the code is written now, rainbow tables could be generated for all possible password hashes of a given simply by hashing all possible passwords of a given length twice.

I do understand this but the keyword in your sentence is could be generated. How we implemented it in Pi-hole guards against any commonly known rainbow tables. I perfectly agree that specific rainbow tables could be generated for double hashed passwords, but this may go beyond any meaningful amount of work. If your password is sufficiently long (and not vulnerable by, e.g., dictionary attacks), you will have a hard time generating brute force double-hash tables for Pi-hole passwords. I doubt anyone would ever do this.
Further, the Pi-hole interface is not meant to be facing the public Internet at all. If you are concerned regarding attack from within your network, then you should probably apply some other measures to protect your Pi-hole, e.g., firewall rules to only allow certain devices to access your Pi-hole on port 80 or completely block this port and only allow connections to it using tunneling over a (key-authentication only!) ssh connection.

If I can get them working in the development branch, I will generate pull requests in both the pi-hole and AdminLTE repos.

No doubt, we will make sure to carefully review and discuss any submission that may come out of your work. It is still not entirely clear to me how this would help in practice as I still see the need to send the salt to the user's browser for this. The salt could easily be intercepted here and suitable rainbow tables could be generated as the salt's intention is merely to modify (lengthen) the password or even the intermediate hashes.
Please don't get me wrong, we are not against this at all, it just was never on our top priority list as our current implementation is "good enough" in terms of robustness against standard rainbow tables and typically the threat potential from within a properly shielded network is rather low.

@nbros652
Copy link
Author

Your recommendation of only accessing over SSH and denying everyone but local host is probably best. I checked no hashing is actually done on the client side! I did a header inspection, and my password is transmitted in plaintext over HTTP, yikes! It is not hashed until after the server receives it (in file AdminLTE/scripts/pi-hole/php/password.php). If you can tell me which file should be handling the hashing in javascript on the client side, I'll look into this further. I checked this on a standard installation of the current release.

@AzureMarker
Copy link
Contributor

The new web interface will hash the password client-side before sending it to the server:
https://github.com/pi-hole/web/blob/9377f7cf3bcd4a63430db26536393d1a25d7e050/src/views/Login.js#L51-L53

If you allow the web interface to be accessed externally, it should be put through HTTPS. This is actually feasible in that case, because you can easily get a cert from places like Let's Encrypt.

@nbros652
Copy link
Author

At this point, I admit that we're digressing from the issue topic

Other than concealing what the user typed into the password field, what difference does that make? Correct me if I'm reading the code wrong, we're effectively authenticating against the hash itself, but the hash is transmitted in plaintext over HTTP. We still have the same issue. The authentication token is being transmitted in plaintext.
Exploit: sniff the hash from the network exchanges between client and server (like we would have the password) and use it for authentication.

Authentication on the client side is out of the question for obvious reasons. Authentication on the server side is not feasible because the client still has to transmit the authentication token to the server in plaintext. I don't see a workable solution that involves HTTP without some kind of pre-shared secret that is never transmitted over the network, but that would require a browser extension or additional software.

I can only think of one bad option:

  • Store the actual password on the server -- VERY insecure and bad idea
  • Send clients a one-time-use salt for hashing.
  • On the client side, salt the authentication token and hash it.
  • Client sends the hash back to the server
  • The server recomputes the hash with its copy of the one-time-use salt and the stored password.
  • The server verifies that no other authentications have been made using this hash
  • Authenticate the client
  • Log the hash so that it cannot be resused for authentications, ensuring that each salt is a one-time-use salt.

@AzureMarker
Copy link
Contributor

Yes, that's about the gist of it. There's no good way to secure HTTP authentication without rolling your own authentication protocol or implementing something like TLS (making the connection now HTTPS).

The issue has been largely ignored or deferred in other applications. Most routers log in via Basic Authentication over HTTP, which is basically just base64 encoding the password. Other locally installed applications either just live with HTTP or add a self-signed cert, which needs to be accepted on all the devices that connect to it (and is usually still marked as insecure or untrusted).

@nbros652
Copy link
Author

I'm going to close this for now. I'll wait and see where the login stuff ends up at the next version before I add any salting of passwords.

@void-m4110c
Copy link

I'm going to close this for now. I'll wait and see where the login stuff ends up at the next version before I add any salting of passwords.

Hi,
any news on this?
Is salting the hashes enabled?

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

No branches or pull requests

4 participants