Skip to content
This repository has been archived by the owner on Sep 8, 2021. It is now read-only.

Passwords Stored Insecurely #69

Open
issuemover631 opened this issue May 12, 2016 · 63 comments
Open

Passwords Stored Insecurely #69

issuemover631 opened this issue May 12, 2016 · 63 comments
Labels
in: data Issues in data modules (jdbc, orm, oxm, tx). Both embedded and external. neverstale This label will be removed soon status: ideal-for-contribution An issue that a contributor can help us with. status: waiting-for-feedback The replication have not been confirmed yet. Or no implementer or no PRs or tips to solve yet. type: request There is no implementer. Or there is no support from the maintainer. Please implement it and appeal.

Comments

@issuemover631
Copy link

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:

INSERT INTO USER VALUES('$USER','enc:$HEXNUMBER',1234574,0,0,FALSE,'$EMAIL')

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.

@issuemover631
Copy link
Author

Comment by NHellFire
Saturday May 14, 2016 at 14:21 GMT


I'd suggest using bcrypt for password storage. http://www.mindrot.org/projects/jBCrypt/
Unfortunately, it looks like it'll be a lot of work to change it.
Main problem is the REST API. One of the login methods uses md5(password.salt) and changing that would break any clients using it (hopefully none use that or they fix it). It also supports taking a plain text password, but the REST API would still need to accept hex passwords for clients that don't send it in plain text. That's not a problem as after decoding it, it can just be checked against the encrypted password as normal.
Last.fm password is also stored as hex, so that'd need changing to use an API token.

I'm not a Java developer, so this is out of my depth. Hopefully this'll help someone.

@issuemover631
Copy link
Author

Comment by danielreuther
Thursday Jul 21, 2016 at 14:37 GMT


It's probably worth to mention that this is only an issue if you're not using LDAP.

@issuemover631
Copy link
Author

Comment by fxthomas
Thursday Jul 21, 2016 at 20:27 GMT


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 md5(token, salt) mechanism used by the REST API? I think this could improve security while keeping the API compatible.

@issuemover631
Copy link
Author

Comment by EugeneKay
Friday Jul 22, 2016 at 16:40 GMT


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:

  1. Update the setup documentation(for administrators) to strongly suggest use of external authentication
  2. Add a notice to the local password change form along the lines of "Your password will NOT be stored securely"
  3. Add support for other external authentication mechanisms eventually

@issuemover631
Copy link
Author

Comment by danielreuther
Friday Jul 22, 2016 at 22:33 GMT


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)?

@issuemover631
Copy link
Author

Comment by danielreuther
Wednesday Aug 24, 2016 at 21:38 GMT


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?

@issuemover631
Copy link
Author

Comment by khers
Wednesday Nov 30, 2016 at 14:28 GMT


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.

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.

@4oo4
Copy link

4oo4 commented Apr 17, 2018

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/
https://www.stubbornjava.com/posts/hashing-passwords-in-java-with-bcrypt

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 HTTP 200 no matter what the API return is without any rate limiting (at least that I could tell). It would make a lot of sense to at least return an HTTP 401 on a bad authentication, so that you integrate it with fail2ban or something else to prevent brute-forcing.

@heyarne
Copy link
Contributor

heyarne commented Apr 21, 2018

@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.

@4oo4
Copy link

4oo4 commented Apr 22, 2018

@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.

@4oo4
Copy link

4oo4 commented May 13, 2018

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 /var/airsonic/airsonic.log:

2018-05-13 15:20:06.133  INFO --- o.a.p.s.LoginFailureListener             : Login failed from [127.0.0.1]
2018-05-13 15:20:42.242  INFO --- o.a.p.s.LoginFailureListener             : Login failed from [127.0.0.1]

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 proxy_set_header X-Real-IP $remote_addr; set in your nginx config. Is there a way to get LoginFailureListener to see the actual IP?

@muff1nman
Copy link
Contributor

That listener might have to get tweaked to obey X-Real-IP, should be pretty straght forward..

@pR0Ps
Copy link

pR0Ps commented Jun 21, 2018

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-term

In 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", p parameter. This sort of preserves backwards compatibility since it's still a supported method (and actually seems to be more widely used by 3rd party servers since it allows for storing hashed passwords).

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 support

The 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

  • Passwords should be stored as salted hashes.
  • Hashed passwords are impossible while the s and t parameters are supported.
  • The p parameter is already defined in the Subsonic API so requiring it instead will incur minimal (but non-zero) API breakage with existing clients.
  • Supporting something like HTTP basic auth as an alternative would be a backwards-compatible way of mitigating the issue of passwords appearing in server logs.

EDIT:

Apparently putting the t+s, or p parameters in POST bodies works too.

From the developer of the "play:Sub" client at sentriz/gonic#14 (comment):

All the Subsonic server variants play:Sub has been running against is supporting parameters in both URL and body.

This handily provides another way for clients to use the p method over HTTPS without leaking the password in the URL.

@stale
Copy link

stale bot commented Sep 19, 2018

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

@stale stale bot added the stale This label will be removed soon label Sep 19, 2018
@muff1nman muff1nman added neverstale This label will be removed soon and removed stale This label will be removed soon labels Sep 20, 2018
@heyarne
Copy link
Contributor

heyarne commented Apr 4, 2019

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 u parameter and pass the token as the p parameter. Validate this p parameter only against such tokens, not against user passwords.

To be clear: This is not an ideal solution. RFC6750 said it quite well:

   Don't pass bearer tokens in page URLs:  Bearer tokens SHOULD NOT be
      passed in page URLs (for example, as query string parameters).
      Instead, bearer tokens SHOULD be passed in HTTP message headers or
      message bodies for which confidentiality measures are taken.
      Browsers, web servers, and other software may not adequately
      secure URLs in the browser history, web server logs, and other
      data structures.  If bearer tokens are passed in page URLs,
      attackers might be able to steal them from the history data, logs,
      or other unsecured locations.

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.

@fxthomas
Copy link
Contributor

fxthomas commented Feb 4, 2020

The need for being able to choose from various types of encoders arises from admins trying to protect from different attack vectors.

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?

We just want a cred (or few) to be not sitting in the db openly that's likely sitting in user space.

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?

@randomnicode
Copy link
Contributor

randomnicode commented Feb 4, 2020

Can we expose that option globally in the properties file, just like other properties?

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.

Is there a need to protect different accounts differently?

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).

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?

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.

The encryption keys must be present somewhere on the filesystem for reversible encryption to work, right?

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.

If an attacker has write access to the database there are very easy ways to export an arbitrary file from the filesystem.

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 SELECT * FROM credentials on your DB will read out all your passwords if you store your creds openly (or hex), regardless of bitlocker or LUKS or any other file system level encryption you have. Your underlying storage level encryption simply won't matter. This won't happen with an encrypted cred sitting in there. A sql injection attack might compromise your db and its data, but the content remains safe and useless to the attacker (without possession of a secondary bit of data).

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?

@fxthomas
Copy link
Contributor

fxthomas commented Feb 8, 2020

This is news to me. No properly secured database will allow that easily.

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.

Why are we storing it as hex and calling that encrypted instead?

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.

The default is stored in the properties file, and is global. You can also override the defaults on a per-credential basis of course.

What I meant was : "only use that default", and don't expose the choice in the UI.

I'd phrase it a different way and more accurately say that there is a need to protect different accounts differently at different times.

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.

@epoupon
Copy link

epoupon commented Feb 10, 2020

Hello,

Interesting topic!
I started my own project LMS (https://github.com/epoupon/lms/) a few years ago. I decided to store passwords using bcrypt with salt because it is just one of the correct ways to store passwords these days.
I added the Subsonic API years later and I discovered how badly designed it is, especially for the authentication part. Due to the t+s authentication method, I just report the server API version to be 1.12 and that's fine for most clients.
I even started a branch with a subsonic API token, as it is done in funkwhale, but after a while I just realized: "why am I spending my time in implenting this crappy workaround for a badly designed API instead?". I was at this time more for a global configuration switch named "I_dont_care_about_security_and_want_to_store_cleartext_passwords=" in order to handle the t+s method and move forward on more interesting features.
Then I changed the strategy and now I just send requests for the "legacy" authentication to the incompatible clients to make them work, sometimes with success, sometimes not.

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.
About bcrypt, I originally set a high number in order to make the authentication long enough (something like 0,5s). Unfortunately, I had to reduce it a lot in order for the API calls to be responsive enough (most of each API call time is spent in hashing the password)

@randomnicode
Copy link
Contributor

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.

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.

  • Can s/he decode the passwords if stored openly (including hex)? Yes. They don't even need write access. SELECT * FROM credentials will do nicely.
  • Can s/he decode the passwords that're stored encrypted? No! They still need to get the shared key, which isn't stored in the db, and getting access to the DB (write, read, or whatever, it doesn't matter) doesn't automatically give them the key. The data sits encrypted in the db, and any amount of DB access doesn't compromise the data content.

I guess the answer to that is "for historical reasons"

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.

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).

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.

What I meant there was : don't allow the user to change it, pick a good default and stick with it.

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?

@randomnicode
Copy link
Contributor

Unfortunately, I had to reduce it a lot in order for the API calls to be responsive enough (most of each API call time is spent in hashing the password)

This is an excellent point and it should be noted down.

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.

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.

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.

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.

@epoupon
Copy link

epoupon commented Feb 10, 2020

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.

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.

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.
I think that being authenticated by an API key should be considered weak too.

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.

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.

@randomnicode
Copy link
Contributor

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 (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.

I think that being authenticated by an API key should be considered weak too.

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.

@fxthomas
Copy link
Contributor

But may I please at least get a review on the infrastructure PR before we proceed?

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 master.

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.

@epoupon
Copy link

epoupon commented Feb 10, 2020

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.

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.

@randomnicode
Copy link
Contributor

randomnicode commented Feb 10, 2020

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 master.

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.

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.

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.

@randomnicode
Copy link
Contributor

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.

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.

@tesshucom
Copy link
Contributor

The randomnicode proposal contains all the considerations and is probably hard to point out weaknesses.
This is also useful when relatively large systems require flexible authentication.

For Airsonic, the requirements may be a little more.
Since users are not only experts, an easy and secure way is liked probably.

  • What specifications are acceptable to users?
  • How technically is it possible to simplify the design?

Ultimately, we don't need consensus on such topics?

@randomnicode
Copy link
Contributor

I've merged my proposal and associated fixes into the airsonic-advanced fork.

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).

@tesshucom tesshucom added in: data Issues in data modules (jdbc, orm, oxm, tx). Both embedded and external. type: request There is no implementer. Or there is no support from the maintainer. Please implement it and appeal. status: waiting-for-feedback The replication have not been confirmed yet. Or no implementer or no PRs or tips to solve yet. and removed security labels May 8, 2020
@glenstewart
Copy link

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.

@frdbonif
Copy link

I'm far from a beginner but also not well versed on cryptography.

  1. Is it currently secure to use the Subsonic API:
    A. With LDAPS?
    B. With local accounts?

  2. Do these answers apply to all Subsonic API servers or just Airsonic-Advanced?

@epoupon
Copy link

epoupon commented Jan 29, 2021

I would say:
1A: likely to be yes
1B: no
2. no, some of these servers always store hashed and salted passwords (preventing them from using the "new" auth system defined from API version 1.13)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
in: data Issues in data modules (jdbc, orm, oxm, tx). Both embedded and external. neverstale This label will be removed soon status: ideal-for-contribution An issue that a contributor can help us with. status: waiting-for-feedback The replication have not been confirmed yet. Or no implementer or no PRs or tips to solve yet. type: request There is no implementer. Or there is no support from the maintainer. Please implement it and appeal.
Projects
None yet
Development

No branches or pull requests