Passwords Stored Insecurely #69
Comments
Comment by NHellFire I'd suggest using bcrypt for password storage. http://www.mindrot.org/projects/jBCrypt/ I'm not a Java developer, so this is out of my depth. Hopefully this'll help someone. |
Comment by danielreuther It's probably worth to mention that this is only an issue if you're not using LDAP. |
Comment by fxthomas Anything wrong with upgrading regular logins to bcrypt, but generating a separate, random password ("mobile API token" could be a nice name for this) for the current |
Comment by EugeneKay I think that adding a separate API Token would be unnecessarily complex, since everything that exists is built around "user / password" mechanics. Explaining how to deal with a single-use account code would cause a lot of confusion. In my opinion(and this is just an opinion, guided mostly by laziness) secure password storage is not Libresonic's job. External authentication mechanisms(ie, LDAP) do a much better job than we ever could, so let's not re-implement it. My suggestion is threefold:
|
Comment by danielreuther I agree that having the (existent) option to use an LDAP to store the passwords securely might lower the priority of this issue a little. But I still believe that there are many legitimate reasons for not wanting to introduce a dependency to another external system just for password hashing -- even if it's just for the sake of keeping the overall complexity low. Don't get me wrong, I too would prefer an LDAP over having the web app store the passwords any time. But one size doesn't fit all :) I would personally go with JCE and something like PBKDF2 (https://www.owasp.org/index.php/Hashing_Java) Can someone explain the md5(token, salt) bit talked about above? How does that work in a scenario where Libresonic doesn't know the password at all (i.e. LDAP)? |
Comment by danielreuther To answer my own question: LDAP of course suffers of the very same problems you have described above: Since Subsonic doesn't have access to the clear text password, it cannot md5 it with the client-submitted random(!) salt. This is a severe design limitation. Alas, there's not much we can do about it apart from asking client developers to not use that form of authentication. So, to cut a long story short: If we introduce a password encryption for the DB, we should make it configurable, so that users can decide for themselves if they want to compromise security for the sake of staying compatible with a certain client that implements the s=&t= scheme. Or am I overlooking something here? |
Comment by khers
I see where you are coming from, but configuring an LDAP setup is non-trivial but installing Libresonic behind a proxy for TLS is pretty simple. I always thought of this as a deficiency in Subsonic, but the maintainer never seemed to take security all that seriously. If you are going to offer the ability to store passwords at all, it should be done well. |
Hashing and salting the password should be a must, not that people would be likely to have sensitive info in Airsonic but it feels like a cop-out to not make it more secure and to tell users not to expect basic security. For the API, using md5 nowadays is a really bad idea, something like jBcrypt looks like the way to go: http://www.mindrot.org/projects/jBCrypt/ It's too bad that would break the API compatibility with Subsonic, I really wish they were more security-conscious. Speaking of, while messing with the API last night I discovered that it always returns |
@4oo4 The subsonic API is quite bad and it's really barely RESTful. It's basically a protocol invented on top of HTTP GET / 200 OK requests. That being said, there is quite a bit of software that knows how to cope with that and provide a nice experience on top of it. I think it would really have to be evaluated how people use airsonic currently. Which clients do they use, over which protocols do they interface with it etc. After we know that we could check whether old clients break if we do stuff like return addtional values in JSON responses, return proper status codes etc. Any idea how to best assess that? I don't know of any good way to do polls in github issues. On the other hand I don't know how representative the share of users on IRC / reddit is. |
@heyarne That's a good question, not sure where else you could reach users besides those places and here, and that might risk not being fully representative. Would it be worth looking into breaking the API where needed for these kinds of things, but then include a 'legacy' option to fallback to the Subsonic API standard? I know that would be a lot of work and add quite a bit of complexity though since you're essentially designing a second API, I would be curious to hear what Airsonic and Subsonic-compatible app devs would think about that. |
Since rewriting the API is a huge task (especially since you'd have to convince people to start using it), I realized that for now the best thing to do is use fail2ban. I noticed that authentication failures will give you something like this in
I can get some fail2ban config working off of that and will post it here, maybe it would be worth adding that to the docs? EDIT: Actually, it seems that won't work if you're running it behind a reverse proxy like I do (nginx), then everything shows up as localhost, even if you have |
That listener might have to get tweaked to obey X-Real-IP, should be pretty straght forward.. |
To get this back on the subject of insecurely storing passwords: (the API reference for reference) As long as the token + salt method of authentication is supported, passwords will have to be stored in plaintext. For modern software, this is unacceptable. Without digging too deeply into the LDAP authentication, it seems like this fact is already known since error 41 in the Subsonic API is "Token authentication not supported for LDAP users". I would assume this is because LDAP doesn't allow you to just pull the password out to check against so the token+salt method doesn't work. One thing to keep in mind is backwards compatibility. There is a whole ecosystem of 3rd party servers and clients that speak the Subsonic API. Changing anything that's incompatible with them will instantly alienate anyone who uses anything but the Airsonic web UI (think mobile users). My suggestions: Near-termIn my (very biased) opinion, the token+salt method should be deprecated and eventually removed entirely. The API should revert back to using the original, more secure, now deprecated, "only to be used for testing purposes", A gradual deprecation is entirely possible. For the moment, since passwords are stored in plaintext, both methods work. This would allow a grace period of a major version or two where both methods are supported. When removing the token+salt method entirely, the stored passwords would be converted to salted hashes of the passwords via a database migration, and the API would start returning an error code ("42: Token authentication not supported"?) for any token-based requests. While this would be a hard fork from the current API (see compatibility notes above), it's still a supported method so it would be a less disruptive change than doing something entirely new. Anecdotally, I use a few clients (a Clementine extension, the DSub Android app, Jamstash) with a server that doesn't support the token+hash method with no issues, meaning client developers still seem to use (or at least support) the old authentication method. The downside to this is now user passwords will be required to be transmitted over the wire in the URL. This means that it will provide no security to clients using HTTP, and the password will appear in the server access logs. IMO clients using HTTP shouldn't really expect anything different since this has always been an issue with creds over HTTP (see the security caveats of HTTP basic auth). The server logs are a genuine concern but since the passwords are already stored in plaintext on the server anyway it doesn't seem that much worse. At least logs can be disabled/sanitized/deleted. The upside is you could then advertise the project as "The only Subsonic fork that takes security seriously" (or something less inflammatory, up to you 😉 ). Additional authentication supportThe previously-mentioned HTTP basic authentication is an acceptable way of authenticating over HTTPS. By also supporting it, clients could effectively opt-in to not having their password shown in server-side logs. An alternate Subsonic API implementation, supysonic, has code that supports this. This type of authentication should generally only require minimal changes to servers and clients to implement since it's just swapping a URL parameter for an HTTP header (see the linked supysonic code). What's also nice about HTTP basic auth is that's it's generally pretty well supported by libraries and browsers. This makes it easier to implement, as well as test in a browser or other tools. Summary
EDIT:Apparently putting the From the developer of the "play:Sub" client at sentriz/gonic#14 (comment):
This handily provides another way for clients to use the |
This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions. |
I have thought about this a bit and I have come up with a way that would allow us to store passwords more securely (i.e. hashed) and still keep API compatibility: Offer a way to generate tokens that have an expiration date (we already have a mechanism for this with JWTs), have this token be tied to a specific set of permissions (which, conveniently, could be stored in the token itself and then signed by the airsonic instance) and give the user an option to revoke this token. Now in REST api endpoints that require authentication, ignore the To be clear: This is not an ideal solution. RFC6750 said it quite well:
I would argue that a token that is unique and revocable is still better than a password that might be exposed without a user's knowledge (because it requires an understanding of HTTP requests, web servers and the intricacies of the airsonic API). It might be hard to communicate to people that this is what they should enter when using an app such as DSub but it would drastically improve the security of the system. |
Can we expose that option globally in the properties file, just like other properties? Is there a need to protect different accounts differently? Also, I don't recall e.g. GitHub or GitLab exposing such parameters, or at least not in the UI, and source code can be way more valuable than a music collection. Is there any reason why Airsonic should do differently?
The encryption keys must be present somewhere on the filesystem for reversible encryption to work, right? If the attacker has read access to the filesystem then it's trivial to retrieve the encryption key. If an attacker has write access to the database there are very easy ways to export an arbitrary file from the filesystem. So the only case you'd be really protecting against would be where the db is exposed read-only. I guess that could happen, but did you have anything in mind? |
The default is stored in the properties file, and is global. You can also override the defaults on a per-credential basis of course. The defaults are just that: defaults. Recommendations and suggestions. If you know better, then the system isn't trying to stop you. But the defaults are pretty strong.
I'd phrase it a different way and more accurately say that there is a need to protect different accounts differently at different times. Imagine you start off everyone with bcrypt. Then due to an ongoing attack, you're worried, so you change the defaults to scrypt. That won't automatically transition all existing creds from bcrypt to scrypt. You can't, you don't have the password, only its hash. So now you will have 2 types, at least. Moreover, consider the added need for a decodable cred for retaining backwards-compatible clients. Let's say hex. Now you have 3 types. Furthermore, you need to store airsonic creds differently from third-party creds. Airsonic creds you can store the hashes of. Thirdparty creds you need to know the full creds so you can forward them. So you absolutely need multiple types of encoders. Even if you store creds in open text (for third party), that's still a type of encoder (a do-nothing encoder).
GitHub is a private service that you can't install on your local containers (unlike Airsonic). They have complete control over their ecosystem, which airsonic doesn't. They also don't need to deal with API compatibility with subsonic. Additionally, of course they provide you with these options: your ssh keys. You can add all types of ssh keys, rsa, ecdsa, dsa etc. Gitlab uses bcrypt for password storage, which is the default for Airsonic (in this PR) also. The source code is open. Everyone knows it. You're trying to protect the data.
Sure. They sit right next to your JWT keys. In the properties file. Yes, there is a high likelihood that wherever the db is, the properties file is also and you can copy them both. But that argument more or less realistically applies to any MFA scheme. In my opinion, 2 factors is still better than 1 and both are better than 0. Any place we can thwart an attack is a win. You're trying to increase the number of checkpoints the attacker needs to possess before completing an attack.
This is news to me. No properly secured database will allow that easily. I guess I'm not understanding the hesitation or the resistance to add actual encryption here. You use "hex" as "form of encryption" (judging by the name of the methods in there), so clearly folks understand it's needed. A simple Would you rather have your creds sitting in your db in open? Your last.fm creds and your listenbrainz tokens? I know I wouldn't. You add about 2 lines for encryption, and it's not like we're creating some convoluted encryption scheme or devising a new algorithm from scratch. It's provided to us by Spring out of the box. And you don't really have to manipulate the system for some "special case" behaviors or edge case scenarios to accommodate it either. The infrastructure is built in this manner and behaves exactly in the same way. The cost is barely tangible to us code-wise. Instead of storing it openly by default, we store it encrypted by default. There is no cost to the user. You need to store those creds in a reversible manner. Your options are store it in open text (or for some reason hex, as a measure of "encryption"), or store it actually encrypted. Why is this even a debate about not storing it encrypted? Why are we storing it as hex and calling that encrypted instead? |
The database itself won't, but anybody gaining write access to e.g. Airsonic's transcoding configuration can ultimately run arbitrary commands with the current user's permissions.
I guess the answer to that is "for historical reasons"? Hex is definitely not encrypted, you're right about that, calling it that way is misleading.
What I meant was : "only use that default", and don't expose the choice in the UI.
You're right about that. We definitely need to support different encoders for backward-compatibility. What I meant there was : don't allow the user to change it, pick a good default and stick with it. If a vulnerability is discovered, then change the default in a new release (and then all new passwords will be encoded with the new one). Changing the encoder is something I suspect very few users will want to ever do on a running instance. And even fewer will want custom encoders per-user. |
Hello, Interesting topic! What I really wanted to say is that I feel there is a real need to fork the subsonic API, not only to correct this authentication method, but also to support more features, like multi artists, multi genres, and more generally multi tags (and a lot of other stuff). Some projects have done this kind of thing before, with success. We would need to involve various people (client and server sides) and formalize things in a centralized, public place. By the way, if you go for the API token, you really should prevent the user from doing any user management related operation, like changing passwords, adding/deleting users, etc. |
Right, except it won't allow access to the files in the file system itself. Here's the point: let's say a malevolent entity does get access to the db.
I suspect the historical reasons for that is the recognition of the need to encrypt the data before putting it in the DB. When encryption wasn't easily available, they put a placeholder in instead (hex) with the easiest effort they could to not leave it open. It meant the need was recognized, thus the solution attempt.
And you'll still need to continue to support the old one, or everyone will get locked out at upgrade. Over time and releases, you'll build a portfolio just like this one.
That's totally fine, and I understand, especially in keeping with simplicity. More choice overwhelms, and sometimes it's easier to just make a choice for the user. You can easily prune the list of available encoders you want to display in the UI, and I have no qualms. However, again, I see that as a separate PR. I've mentioned multiple times I see the existing PR as more of the infrastructure. What gets exposed or available can be a different PR. I can easily see not exposing all the digest-based encoders (which are a bad idea anyway). But may I please at least get a review on the infrastructure PR before we proceed? |
This is an excellent point and it should be noted down.
Like only tokens that are stored openly in the backend? Or all tokens? That is an interesting proposal, let me think about it. I guess we're saying, these creds you're using are decodable, thus easily compromisable, therefore we're not going to let you do dangerous stuff with it.
No arguments from me here. Personally, I think this is a good idea, though perhaps this should be a new issue? I'd be curious to know these other projects. |
When you authenticate using cookie, you are considered as weakly authenticated, and sensitive operations must require you to strongly authenticate first (for example using your password). That is how it is done on various websites, including github.
Yes, it should definitely be a new issue. As an example OpenZFS did that before (fork of ZFS which became close source), but I admit that it requires a lot of commitment. |
Yes (albeit with some caveats). Jsessionid is used for the cookie. The cookie originates on the server, and the client just reuses it. If the cookie is captured, your account is accessible. Thus, the need for stronger auth, where a piece of data that originates on the client is used for auth.
Why? How is it different from providing a password? The point is origination. An API key that originates from the client is no different than providing a password on a website by the client. Both are provided by the client and do not use a server-originated cred. I thought your reasoning had more to do with credential security in the back-end. |
So far I'm necessarily going to be biased in my review since we don't agree on the features that need to be included, and given the scope of the changes this will need a significant amount of work for me to rebase and test this against Airsonic At this point, I'm not sure I care enough to work on this alone, especially since I'm not the most qualified here. If somebody volunteers to review your PR and rebase it against master, I will gladly assist to test it and will help getting it integrated. |
You are right, it actually just has to do with back end security, since the API key has to be stored cleartext in the database. |
A suggestion: Try and cherry-pick the commits in the PR only. You cannot rebase all of one fork's PR onto another's master, there are way too many other changes that have been merged since airsonic master moves slowly. git fetch and then git cherry-pick the commit hashes to apply only the changes in the PR against airsonic's master. I meant the review as a comment for everyone in general. Anyone should feel free to review and provide input. Not being qualified doesn't mean there isn't room for questions or provide other ideas (like coming up with better naming conventions). Plus code reviews are usually a good way to become qualified.
Fair enough, I've provided a prototype, I hope someone on the airsonic-dev team can pick up and integrate it into airsonic. I'll integrate it into the advanced fork shortly so at least one fork has secure credentials. |
It doesn't. Only creds used for authenticating via t+s need to be stored in a decodable manner (and still don't have to be open-text, you can encrypt them). My idea was to allow multiple tokens/passwords per user. You can use one encoded with bcrypt for most of your clients' auth. But every once in a while someone will use a client that'll use t+s, and for that, add another credential for that account that's encrypted using a decodable encoder. The first cred then continues to handle auth for the p-based clients. The new one handles auth for the t+s based clients. I can see some reasoning behind not allowing t+s clients to do sensitive stuff since their creds are decodable and thus recoverable. But then any t+s clients cannot perform any account management functions. |
The randomnicode proposal contains all the considerations and is probably hard to point out weaknesses. For Airsonic, the requirements may be a little more.
Ultimately, we don't need consensus on such topics? |
I've merged my proposal and associated fixes into the
Seems to work fine, and if there are bugs, I'm sure they'll surface and we can take care of them. Ultimately, I don't know if consensus is required here, but in my opinion, credential security is a critical issue. All the CVE fixes and library upgrades and maven dependency checks are meaningless if the basic credential security is flawed. I don't think the proposal is that complicated to begin with to need "simplifying" any design. It's about as straightforward as possible: migrate to using a nondecodable algorithm. Add ability to add a decodable cred for t+s and for third-party vendors. Never store open creds. I don't know how you could simplify it more. The maintainers here are free grab the commits and use all or a pruned version of the complete proposal. I just couldn't wait anymore to have such a glaring security issue for longer than necessary. I suspect that the hesitance to make big, bold or significant changes for fear of "breaking things" not only slow down critical issues like this, but also cause debate paralysis (this issue is now roughly 4 years old and it should really be a show stopper for any non-amateur usage). |
I think it would be best to ask the person creating a new account what the purpose of this account will be - if it's just for web/online playing, if they intend to use an API client, etc. If they intend to use an API client, ask them if they want their password stored securely, or if they don't care. If the want it secure, print on screen what their encrypted password will be for API clients, because it will differ from what they enter online. I say this because I just set up airsonic-advanced for the first time, intending to use API clients (Dsub, etc) and kept getting password errors that did not point me to this whole topic. It was by pure luck and perseverance that I came across this discussion, as I was only moments away from giving up on the whole airsonic solution and just putting up with old Subsonic. |
I'm far from a beginner but also not well versed on cryptography.
|
I would say: |
Issue by alexfornuto
Thursday May 12, 2016 at 15:04 GMT
Originally opened as https://github.com/Libresonic/libresonic/issues/69
Currently Subsonic (and thus LibreSonic) stores user names and passwords in
/var/subsonic/db/subsonic.script
insecurely:The value
$HEXNUMBER
can be trivially decoded to reveal a user's password. I, for one, am tired of having to qualify to every new user that they shouldn't make their password something they wouldn't want me to be able to see.The text was updated successfully, but these errors were encountered: