security: fix secret leak

[po5]

This does not fix the underlying issue, and there are other ways for someone snooping on the database to derive the secret.

This is a breaking change. All existing post image and thumbnail links will stop working.
No migration script is provided.

The idea behind the current password hashing model is that an attacker needs access to the database and either arbitrary file read or memory read.
This model is broken when we're providing attackers with the result of hashing a known string (any post id) with our secret.

It means that right now, password security is actually the same as a standard "unsalted" argon2id hash, and that only a database compromise is required.
Note argon2id already comes with a per-password salt by design, we are just essentially making the salt longer with a cryptographically-insecure random.choice string with known pattern of cvcvnncvcv (wtf?).

This PR stops the leak. There is nothing we can do about already-leaked data, existing installs are affected until they change their secret.
Custom salting should be entirely removed, but I haven't done it here because that'd require writing migrations for user passwords.

[neobooru]

Please correct me if I am wrong, but as I understand it the issue is that an attacker can brute-force the secret without access to either the db or the filesystem. This is possible because the get_post_security_hash function calls HMAC_MD5(key=config.secret, msg=postId) and the resulting hash is returned in the URL of the post content.

While this isn't ideal, this is only an issue if your secret is weak, e.g. because you didn't change it or because you used a short/non-random secret.

The idea behind the current password hashing model is that an attacker needs access to the database and either arbitrary file read or memory read.
This model is broken when we're providing attackers with the result of hashing a known string (any post id) with our secret.

True, but you still need to actually brute-force the key/secret.

And you still need database access if you want to crack the passwords. And if you have database access then you can just create a new user with a known password, read the hash & salt from the database, and then brute-force the secret that way?

... we are just essentially making the salt longer with a cryptographically-insecure random.choice string with known pattern of cvcvnncvcv (wtf?).

Yeah I guess that is silly, but it also doesn't hurt the security in any way.

In short, I agree that 'leaking' the secret this way is technically an issue and that we should fix that, but I don't think that this is a big issue in practice. Though please correct me if I made any mistakes.

[noirscape]

I'd like to point out that bruteforcing the secret doesn't uh... inherently get you access to the password? It just gives you access to the secret, which seems to just be a bit of extra salt on top of an automatically generated salt if I understand the password generation process (not to mention that the function being used already injects it's own salt on top of that.

Basically the password that gets fed to pynacl's argon2id.str function (which does it's own salting since it's meant to be a braindead argon2id hasher) ends up being:

secret + cvcnncvcv + password

The thing is that because pynacl already does it's own salting, you don't actually get access to the password even if you break the secret. You just know one part of the password as it exists after pyncal verifies it, alongside a 9 character long somewhat insecure string. Assuming your secret string is long enough, that still makes bruteforcing passwords pretty infeasible, especially when you get to the fact that the default password regex is at least 5 characters (which honestly should be bumped up to at least 8 but that's configurable).

The risk kinda exists, I just don't think it's all that feasible for an attacker.

--

That ASIDE - I seriously cannot in good conscience say that I think this patch is worth merging as is, since it's both a breaking change (without offering any indication on how someone could actually fix it) as well as incomplete since you forgot to update szuru-admins fixer script (which would have been a manual way to mitigate the breaking changes here).

Breaking content hotlinks is one thing (as annoying as it is, it is at least mitigable), but this does more than just that - in the event you somehow forgot, szurubooru serves all of its file paths directly from the filesystem by design so that nginx or a similar reverse proxy can be used to serve that directory statically in the event that the database grows so large that waitress would clog up serving static files. Just changing how the application generates it's URLs will just straight up break everything and your PR explanation offers no explanation on how to fix it.

The actual way to fix this is to add a _live_migration step to facade that fixes the files only if this fix hasn't already been applied.

[po5]

I don't expect this to be merged as-is, but I wanted others to be aware of this issue, and allow them to sidestep it for new deployments.
But the issue isn't actually fixed as @neobooru pointed out.

It would be better to introduce a secret2/password_secret later on to use for sensitive operations, and keep the current one for compatibility with old filenames.

While this isn't ideal, this is only an issue if your secret is weak

Absolutely not. If someone can crack an argon2id hash, they most certainly can crack an hmac-md5 with reasonable entropy.

I'd like to point out that bruteforcing the secret doesn't uh... inherently get you access to the password?

The thing is that because pynacl already does it's own salting, you don't actually get access to the password even if you break the secret.

These are all things I said. But it makes the use of our secret pointless (+ salt which was always pointless, since the switch to argon2id anyway).
We should remove the fake security, clearly labeling it as such in code comments if keeping it for convenience, or introduce a real solution.

And if you have database access then you can just create a new user with a known password, read the hash & salt from the database, and then brute-force the secret that way?

Yes, although you'll have to crack a stronger hash to get the secret, the issue is the same. The current model is fundamentally broken.

An idea would be to use AES (or equivalent) to encrypt the final hash in database, which we decrypt when comparing password hashes.
Marking as draft, but will probably close this in favor of another PR implementing this idea.

Since we require Javascript to display the site, we could also take the Mega approach of deriving a public/private key pair from the user's password and other account info on the client, and signing requests for authentication.
This would make the main login flow more complex, but wouldn't make it harder to write 3rd party scripts. They just need to use the token system.

[neobooru]

I don't see a reason to change the password hashing approach. Using AES to encrypt the hash just shifts the responsibility, because now you need to protect the AES key somehow.
Our custom salt is weak, but not terribly weak. This however does not matter at all because of the built-in salt in argon2id.
Using a better password regex might be a good idea, but that is a separate issue. You can only go so far to protect user's from themselves, without enforcing a lot of things which might be overkill in certain scenarios.

One solution which might be possible:

1. Store the current content url/hash in the database (we current always calculate this for each request, I think).
2. Change the content url/hash generation (get_post_security_hash) to something else. Using the checksum is a possibility, but then you could brute-force query a website to check whether they host a certain image, which is a privacy issue. (Keep in mind that you don't need to be logged in to directly access content)
   • This new content url then only applies to new content, as older content has its original ('insecure') url stored in the database.
   • Honestly, just a os.urandom * 16 could do the trick here. The url doesn't actually need to make sense.
3. Figure out a way to change the password secret without invalidating all the old password hashes.
   • I didn't think long about this one, but I don't think this is possible without using a second secret and then 'upgrading'/rehashing the hashes.

This is quite a complex solution though, but it shouldn't break anything.

Easier solution:

1. Change get_post_security_hash to os.urandom * 16
2. Call szuru-admin --reset-filenames
3. Direct links to images are broken, but this is the easiest solution by far.

I didn't actually test this, but it sounds like it could work. This does assume that the secret didn't already leak.

Or a combination of the both. Or maybe nothing at all, because in practice I don't think we really have an issue.

[po5]

The solution I went with in po5@8c56e94 was to add a new image_key column to users and posts that then gets used in filenames.
I still haven't rewritten this in a way that would provide seamless upgrades, if someone wants to take a stab at it.