6 ms·

Okta Bcrypt incident lessons for designing better APIs

377 points by n0rdy 2y ago

Lvl999Noob 2y ago
Can someone explain, in clear layman terms, what the difference is between a password hash and a KDF? I have went through this whole thread and tried to look around online but I still don't understand.
- pjc50 2y ago
  Password hash is designed for matching: take the salt, add it to the password, run it through the hash, compare it to the stored hash. The important properties are: - MUST be non-reversible, including against tricks like "rainbow tables" - should be somewhat expensive to discourage just trying all possible passwords against a (leaked) hash KDF is a key derivation function. The value will be used as a key in, say, AES. The important properties are: - should distribute entropy as well as possible, across the required width of output bits - reversibility less important as the derived key shouldn't be stored anywhere - may or may not want artificially inflated cost to discourage cracking
  - Lvl999Noob 2y ago
    I still have no idea now haha. Your answer and Fabbari's are total opposites. If I am understanding right, you are saying that Password Hash is how a password should be stored while a KDF is not meant for storing passwords. Fabbari is saying the opposite of this, that KDF should be used for storing passwords while password hashes should not.
    - pjc50 2y ago
      Further discussion upthread under https://news.ycombinator.com/item?id=42957300 https://news.ycombinator.com/item?id=42957300
- fabbari 2y ago
  A password hash is a simple hash of a password. Hash algorithms are made to be fast. KDF - key deriving functions - are slow by design and are made to derive a key from a given string. They are designed to be slow to make password searching slower. This is a 2c tour of the topic.
  - johnisgood 2y ago
    There are 2 comments to OP, and now the person can wonder which one is supposed to be slow or not.
jedisct1 2y ago
The bcrypt implementation in the Zig standard library has both the bcrypt() function (where truncation is explicitly documented) and the bcryptWithoutTruncation() function (which is recommended and automatically pre-hashes long passwords).
- n0rdy 2y ago
  Author here: thanks for reading the post. It's great to hear that Zig covered both cases. However, I'd still prefer the opposite behavior: a safe (without truncation) default `bcrypt()` and the unsafe function with the explicit name `bcryptWithTruncation()`. My opinion is based on the assumption that the majority of the users will go with the `bcrypt()` option. Having AI "helpers" might make this statistic even worse. Do you happen to know Zig team's reasoning behind this design choice? I'm really curious.
  - masklinn 2y ago
    Note that the "safe" version makes very bespoke choices: it prehashes only overlong password, and does so with hmac-sha512 (which it b64-encodes). So it would very much be incompatible with other bcrypt implementations when outside of the "correct space". These choices are documented in the function's docstring, but not obvious, nor do they seem encoded in a custom version.
    - jszymborski 2y ago
      Sounds like it would then make sense to hide crypto primitives and their footguns under a "hazmat" or "danger" namespace, sorta like webcrypto or libsodium. So something like crypto.danger.bcrypt and crypto.bcryptWithTruncation
  - jedisct1 2y ago
    `bcrypt()` is bcrypt as implemented everywhere else, and is required for interoperability with other implementations. If you don't truncate, this is not `bcrypt` any more. `bcryptWithTruncation()` is great for applications entirely written in Zig, but can create hashes that would not verify with other implementations. The documentation of these functions is very explicit about the difference. The verification function includes a `silently_truncate_password` option that is also pretty explicit.
tptacek 2y ago
Bcrypt is a password hash, not a KDF, which is the way it was used in this API. It's super unclear to me why they wanted a string-based KDF here at all; does anyone have more context? I've in the past been annoying about saying I think we should just call all password hashes "KDFs", but here's a really good illustration of why I was definitely wrong about that. A KDF is a generally-useful bit of cryptography joinery; a password hash has exactly one job.
- masklinn 2y ago
  The value is the combination of userid, username, and password, so in threads on other platforms people have hypothesised that the developer tried to play it safe and use a password hash because of the password's presence. Also I'm not sure the average developer understands the distinction.
  - deleted 2y ago
    [deleted]
  - dotancohen 2y ago
    > Also I'm not sure the average developer understands the distinction. I'm an average developer. I'm not sure that I understand exactly. What should I be reading, or what can you tell me? Thank you!
- pclmulqdq 2y ago
  They didn't want a KDF, as far as I know, but they wanted a hash function with unlimited input size. Including the username in the hash input gives you guaranteed domain separation between users that you don't get from salts/nonces. Its a generally good idea if you have a hash function with unlimited input size (all modern cryptographic hash functions except bcrypt have unlimited input size).
  - masklinn 2y ago
    They clearly wanted something stronger than "a hash function" or they'd have reached for weaker cryptographic hashes.
coolgoose 2y ago
I am curious why bcrypt was used for hashing in the first place and not something like sha-512 Is there a reason I might be missing?
- stavros 2y ago
  Yes, the hashed payload contained a password, so presumably they didn't want to just SHA it.
  - coolgoose 2y ago
    But why not bcrypt the password, but sha the cache key on top?
    - stavros 2y ago
      I guess because they didn't anticipate this flaw.
      - masklinn 2y ago
        Also prehashing opens you up to an other bcrypt flaw you need to be aware of: it stops at the first NUL byte, so you need to use some sort of binary-to-text encoding on top of the hash to ensure you don't have any of those in the data you ultimately hand off to bcrypt.
        coolgoose 2y ago
        Thank you
        Dylan16807 2y ago
        It's astounding how bad the default API for Bcrypt is.
  - tptacek 2y ago
    Begs the question of why the payload contained a password, right?
nabla9 2y ago
> was used to generate the cache key where we hash a combined string of userId + username + password. Don't conceive your own cryptographic hacks. Use existing KDF designed by professionals.
- edoceo 2y ago
  Is the functions in libsodium enough? Provided they are used correctly?
  - nabla9 2y ago
    Yes.
- ludwik 2y ago
  Simply hashing your data (using an established hashing algorithm/library combo) to later compare two hashes in order to check whether the data has changed doesn’t usually feel like rolling your own crypto.
  - nabla9 2y ago
    The use case was KDF and they decided to do simple password hash signature hack instead by combining strings. They fucked it up.
    - ludwik 2y ago
      Of course they fucked it up, as evidenced by their bad security incident. The only question is whether you can really chalk this particular one up to a problem with "rolling your own crypto." That mantra exists for a reason, but it doesn’t feel like it really applies this time. It seems more like they used established crypto—just not the right one for this particular use case.
      - nabla9 2y ago
        Concatenating strings before giving it to the hash function instead of using KFD is rolling your own.
whalesalad 2y ago
damn that sounds like a rookie mistake for an organization who is literally in the business of secure auth
Tostino 2y ago
That is such a rookie mistake. It's not some hidden information that bcrypt has a 72 char limit. Pretty widely documented in multiple implementations and languages. How does a company whose only job is security screw that up so badly?
- n0rdy 2y ago
  > How does a company whose only job is security screw that up so badly? While I don't have any answers to this, I've realized that it's an ideal showcase of why fuzzy testing is useful.
- CJefferson 2y ago
  On the other hand, why not have implementations assert if they are given a string longer than 72 chars? It feels to me like no-one would ever do that on purpose, so it's a massive issue which is easy to accidentally make with a really important function.
  - Tostino 2y ago
    Don't disagree there. I asked my self the same question the first few times I had to use it. Silently truncating the data is about the worst way to deal with it from a security standpoint. No idea why that decision was made back in the day.
  - zorgmonkey 2y ago
    It is almost never I good idea to assert in a library, unless the error is truly unrecoverable. I think returning an error code\throwing an exception would be very reasonable and a much better API than failing silently though.
    - Dylan16807 2y ago
      An exception is fine if the language has them. I don't think "assert" was meant super literally and exactly the way C does it. An error code is risky.
    - 2y ago
sscarduzio 2y ago
what I would have naturally done without anticipating any flaw (and probably be just OK): cache_key = sha(sha(id + username) + bcrypt(pass)) with sha256 or something.
- throwaway-9111 2y ago
  Why not a simple sha(id + username + bcrypt(pass)) Is there any security issues with that? I'm a "newb" in this area, so I'm genuinely curious about the flaws with the naive approach
progmetaldev 2y ago
Reminds me of when I saw a junior developer calling SHA-1 on an incrementing integer ID, with no salt. We had a long talk about it, he thought it was too "scrambled" to allow anyone to recognize what was being done. He shouldn't have been so junior, he was 4 or 5 years into his career. I had to be the bad guy and override his decision without further discussing why it was a bad idea, and I really tried for a good 45 minutes to explain things. He got it a week later when I showed him rainbow tables, and I felt bad having to tell him to just do what I said for the solution, but sometimes you just have to make the decision to say "do what I said, I'm sorry you don't understand, I tried to explain."
- bawolff 2y ago
  Rainbow tables is not the (only) reason you dont want to hash something low entropy like an incrementing int, and adding a salt wouldn't make this secure. [Im assuming the usual definition of salt where it is known by the attacker... a pepper would be fine]
  - progmetaldev 2y ago
    I agree, and I guess I did use salt differently than how most people see it, rather than how it is most effective. I never stored the salt in the database alongside the password. I would use something from the user that wouldn't change without a password change, as well as some type of semi-long data that also got hashed and put into the "pepper". Even if it's a file on disk that contains data that is read into memory and hashed with something that doesn't change (or at least can't change without the user also re-entering or creating a new password). Also, thank you for teaching me the term "pepper", because I feel like that is so relatable, but also different enough to correlate the two, but show how "pepper" is more powerful and useful!
- ack_complete 2y ago
  I've seen this before, a belief that just because the output looks random that it is secure. It's like storing license plates -- just hashing them without additional seasoning is of little use, because the number of possible license plates is so low that they can easily be brute forced. Similarly, a developer I worked with once claimed that CRC32 was sufficient verification because CRC32s changed so drastically depending on the data that they were difficult to forge. He was surprised to find out not only is it trivial to update a CRC32, but also to determine the CRC polynomial itself from very few samples.
jhhh 2y ago
I can see the incident was a jumping off point to talk about bad APIs (bcrypt probably should error >72) but it sounds like the actual bug was they weren't checking the value in the cache matched the data they used in the hash for the key. The authentication cache check should survive any arbitrarily bad hashing algorithm because all of them are going to have collisions (pigeonhole principal). Even an arbitrarily 'strong' hash function with no input truncation, as long as it has a fixed width result, will have this property. Thus, any arguing in the comments here about different hash functions with different truncation properties is moot. The analogy is something like creating a hash map whose insert function computes the slot for the key and unconditionally puts the value there instead of checking if the keys are the same during a collision. No amount of tinkering with the hash function fixes this problem. The algorithm is wrong. A hashmap should survive and be correct even giving it a hash function that always returns 4.
- tialaramex 2y ago
  I would guess that they felt comfortable that the bcrypt output (192 bits) is enough that collisions are very unlikely. If these were already partitioned by customer, rather than being a single cache for the entire Okta userbase that seems fine. You're going to have weird cosmic ray bugs more often than a natural collision. Now, the data structure they're using for a cache will use some sort of hash table, likely in memory, so maybe they've got the 192-bit bcrypt "key" and then that's hashed again, perhaps well or perhaps badly [e.g. C++ really likes using the identity function so hash(12345) = 12345] but then a modulo function is applied to find the key in an index and then we go groping about to look for the Key + Value pair. That part the API probably took care of, so even if the hash has size 6-bits, the full 192-bit key was checked. But the original data (userid:username:password) is not compared, only that 192-bit cache key.
- semicolon_storm 2y ago
  Not sure about that. A hash function suitable for security sensitive work, used properly, should make a collision so unlikely that you can basically forget it that it's even possible. Think about it, that's what hashing passwords relies on. We don't store a plaintext password for a final check if the password hash matches, we count on a collision being basically impossible. A hashmap is different, because it's using a much weaker hash function with far fewer security guarantees. Plus, you're assuming the original values are even kept around for comparison. The cache key likely just mapped to something simple like a boolean or status flag.
bawolff 2y ago
> On the other hand, such long usernames are not very usual, which I agree with Weird take. Usernames are often chosen by the user. Less so in corporate world but definitely not unheard of
- SebFender 2y ago
  Many of my usernames at my company are based on my email and it's pretty long - by the time you add the domain it's a good 47 characters...
  - eesmith 2y ago
    I was at a Python conference once and met someone whose email ended '@boehringer-ingelheim.com'. That's 25 letters right there. https://www.boehringer-ingelheim.com/media-stories/press-releases https://www.boehringer-ingelheim.com/media-stories/press-rel... has a camilla.krogh_lauritzen@boehringer-ingelheim.com at 48 characters, for example.
ww520 2y ago
Have they did a bcrypt(password + userId + username), it won't be so bad. Order of entropy is important. Also I'm not sure what functionality the authentication cache provides, but their use of bcrypt(userId + username + password) implies the password is kept around somewhere, which is not the best practice. OT. Has Argon2 basically overtaken Bcrypt in password hashing in recent years?
- buzer 2y ago
  > Have they did a bcrypt(password + userId + username), it won't be so bad. Order of entropy is important. That depends on how exactly it was used. If it was simply used to check if previous authentication was successful (without the value containing information who it was successful for) then single long password could be used to authenticate as anyone.
  - ww520 2y ago
    > single long password could be used to authenticate as anyone. Only if everyone uses the same long prefix for password.
    - buzer 2y ago
      No. If the value of the cache key is simply true/false then someone would first login to their own account using the long password. This would result in storing: bcrypt(longpassword + 123456 + me@foobar.com) = bcrypt(longpassword) = hash1 -> true If they then try login as you@bar.com using same password there would be a cache lookup: bcrypt(longpassword + 1111111 + you@bar.com) = bcrypt(longpassword) = hash1 -> true
SebFender 2y ago
I've seen this multiple times - even better I don't know how many ways we found a simple workaround or bypass of the complete process in so many apps... In essence this has nothing to do with the API itself but the way in which is another ballgame altogether. Great post though.
throwaway984393 2y ago
[dead]
renewiltord 2y ago
This is a completely unreasonable API. It reminds me of the `mysql_real_escape_string` vs. `mysql_escape_string`. The default API must be the strict one. You should be able to configure it to be broken but silent truncation is an insane piece of functionality. There is no universe in which this is logical. One might as well just have everything return void* and then put in the documentation what type to cast to. The invariant is clearly a historical accident. As a mistake, it's fine. Everyone writes up things like that. But defending it as an affirmatively good decision is wild.
- benced 2y ago
  Seriously, the number of “you weren’t meant to fire that foot gun” defenses in this thread…
sandeepkd 2y ago
I enjoyed the article and the detailed analysis for different languages. The conclusion is probably the part where most of the disagreement lies. API design is is not really at fault here if we consider the purpose of the API and the intended output. The API was designed to generate a hash for a password (knowledge factor) and for performance and practical reasons a limit has been picked up (72). The chances that some one knows your first 72 characters of password implies that the probably is a lot higher for the abuser to have remaining characters too. While smaller mistake here in my opinion was not knowing the full implementation details of a library, the bigger mistake was trying to use the library to generate hash of publicly available/visible information
- AlfeG 2y ago
  Ohhh, it's scrollable... I wondered why this small article gained so much attention...
  - hansvm 2y ago
    Yeah, the fact that I can't have my mouse in the normal position and scroll the actual article was a problem 10 times or more while trying to read the thing...
- hansvm 2y ago
  > limit has been picked up (72) There's nothing wrong with a limit. The problem is that the library silently does the wrong thing when the limit is breached, rather than failing loudly.
underdeserver 2y ago
Hold on, in the Rust example, how does `err_on_truncation` get set? TFA completely ignored that there's a setting somewhere (probably incorrectly defaulting to false)
- a-dub 2y ago
  the rust library exposes a handful of "non_truncating_*" functions that enable error handling. i would expect this to be for drop-in compatibility with old code. amusingly, the python "library" is just a thin wrapper around the same rust library. protip: a lot of cryptography primitives actually aren't that complicated in terms of the code itself (and often can be quite elegant, compact and pleasing to the eye). if it's important, probably worth just reading it. it's what people wrap them with or the systems they build that get messy!
- kmarc 2y ago
  In the bcrypt crate there is an explicit method for it: bcrypt::non_truncating_hash() https://docs.rs/bcrypt/latest/bcrypt/ https://docs.rs/bcrypt/latest/bcrypt/ Funnily, TFA later also suggests that such function should exist...
  - ratorx 2y ago
    Being pedantic, TFA suggests something slightly different. The non_truncating_hash should be the default (and called something that reflects it, eg. just hash), and a separate truncating_hash function may exist. The difference (from an API design perspective) is pretty massive.
llmthrow102 2y ago
How does anyone take Okta seriously after this incident btw?
philippta 2y ago
What's the reason behind bcrypt(userId + username + password) rather than just bcrypt(password) ?
- ReptileMan 2y ago
  rainbow tables I guess
- Tade0 2y ago
  What if two different users have the same password?
  - magicalhippo 2y ago
    Bcrypt is salted[1], so that shouldn't matter? [1]: https://en.wikipedia.org/wiki/Bcrypt#Description https://en.wikipedia.org/wiki/Bcrypt#Description
    - Tade0 2y ago
      Are you sure? bcrypt stores the salt and retrieves it for comparison - otherwise you wouldn't be able to generate a matching hash. Consider the case where a user has a very long username and sets their password to their userId + username + password thus recreating the scenario which lead to the incident.
      - magicalhippo 2y ago
        That was not my point. My point was there wouldn't be a hash collision just by two users with the same password due to the salting.
        Tade0 2y ago
        There's no hash collision here, just two different hashes, each with its own salt, matching the same original phrase. If you use only the password to generate the cache key, then this password will match regardless of salt, so users with the same password will generate a cache key matching that password.
withinboredom 2y ago
I'm really surprised they didn't cover PHP since (almost?) every framework uses bcrypt in php these days.
- duskwuff 2y ago
  PHP's password_* functions make it difficult to misuse in this particular way. There's no function in that API which hashes a password with a controllable salt and returns the result; there's only password_hash(), which always uses a random salt, and password_verify(), which rehashes a password internally and returns a bool for whether it matched. (It's still got the truncates-at-72 problem with PASSWORD_BCRYPT, though.)
mariocesar 2y ago
I'm confused, it seems that the OP wants to use Bcrypt as an encoding/decoding utility. About solutions, Django hashes by default the password only with a salt. I'm not sure why it would be valuable to combine user_id+username+password. I've always assumed that using salt+password was the best practice.
- mariocesar 2y ago
  Regarding the API design, I agree now with OP after reading other comments on HN. The API would be improved if it clearly indicates to the user when truncation is done, even if this understanding is implied by principle.
zero_k 2y ago
Another incident at Okta? Oh no! Its security has _always_ been a mess. It's a dumpster fire and no client of their cares because their identity systems are so messed up, that it's better to have the mess of Okta, than the mess they are sitting on. It's kinda crazy they get away with such incredibly bad security practices. Like... this bcrypt issue has been know for a LONG while. We used to test for it 8-10 years ago. There's either (1) nobody competent enough there to know (which is likely not true, I had a pentester friend recently join, and she is very good), or, more likely (2) management doesn't care and/or doesn't give enough authority to IT security personnel. As long as clients don't have any better options, Okta will stay this way.
- BrandoElFollito 2y ago
  > no client of their cares because their identity systems are so messed up, that it's better to have the mess of Okta, than the mess they are sitting on Yes, this is very true. Also some companies realize that they can screw up royally because they do no have the proper knowledge, and authentication is not a core business of theirs. I can understand them. I also use mail systems I am not that happy with, but I have this comforting idea that if they have a problem, 3B people are waiting together with me for it to be solved, and that's the kind of pressure that helps.
nfriedly 2y ago
I strongly agree with the conclusion that the libraries should reject input they can't correctly handle instead of silently truncating it. I co-maintain a rate-limiting library that had some similar rough edges, where it wouldn't always be obvious that you were doing it wrong. (For example: limiting the IP of your reverse proxy rather than the end user, or the inverse: blindly accepting any X-Forwarded-For header, including those potentially set by a malicious user.) A couple years back, I spent some time adding in runtime checks that detect those kinds of issues and log a warning. Since then, we've had a significant reduction in the amount of not-a-bug reports and, I assume, significantly fewer users with incorrect configurations.
- deepsun 2y ago
  If the input is 71 character, all the libraries happily accept it, but an attacker needs to guess only 1 character.
  - arccy 2y ago
    have separate salt / pepper / user id args
    - tedunangst 2y ago
      How is the library supposed to know you're doing that wrong?
  - cmgriffing 2y ago
    If these tools had a runtime check, then the cache key creation would have failed out. 72 is the max length of id, username, and password combined. If that combination is over 72, then failure and the cache key would not have been created. So, no, the attacker would not need to guess only one character of a password.
thekemkid 2y ago
In Node, you would commonly reach for the builtin core "node:crypto" module to run cryptographic functionality like this. I wondered why that wasn't used here, but bcryptjs was. After digging into it a little, node doesn't ship with core support for bcrypt, because it's not supported by OpenSSL. The node crypto module is essentially an API that offloads crypto work to OpenSSL. If we dig into OpenSSL, they won't support bcrypt. Bcrypt won't be supported by OpenSSL because of reasons to do with standardisation. https://github.com/openssl/openssl/issues/5323 https://github.com/openssl/openssl/issues/5323 Since bcrypt is not a "standardised" algorithm, it makes me wonder why Okta used it, at all? I remember in uni studying cryptography for application development and even then, back in 2013, it was used and recommended, but not standardised. it says a lot that 12 years on it still hasn't been.
jmuguy 2y ago
I was curious how bcrypt-ruby would handle this. It does not throw an error for input length larger than 72. However the actual API for the gem makes it pretty clear its for hashing a password, and not just hashing in general - as you can see from the code. https://gist.github.com/neontuna/dffd0452d09a0861106c0a46669a3ff0 https://gist.github.com/neontuna/dffd0452d09a0861106c0a46669...
veqq 2y ago
[u/forgot-CLHS](https://www.reddit.com/r/lisp/comments/1ikrz1g/shout_out_to_common_lisps_ironclad/ https://www.reddit.com/r/lisp/comments/1ikrz1g/shout_out_to_...) notes that Common Lisp's defact standard cryptography library Ironclad's [implementation](https://github.com/sharplispers/ironclad/blob/master/src/kdf/bcrypt.lisp https://github.com/sharplispers/ironclad/blob/master/src/kdf...) avoids such problems! ``` (defmethod derive-key ((kdf bcrypt) passphrase salt iteration-count key-length) (declare (type (simple-array (unsigned-byte 8) (*)) passphrase salt)) (unless (<= (length passphrase) 72) (error 'ironclad-error :format-control "PASSPHRASE must be at most 72 bytes long."))...) ```

377 points by n0rdy 2y ago

Can someone explain, in clear layman terms, what the difference is between a password hash and a KDF? I have went through this whole thread and tried to look around online but I still don't understand.

pjc50 2y ago
Password hash is designed for matching: take the salt, add it to the password, run it through the hash, compare it to the stored hash. The important properties are: - MUST be non-reversible, including against tricks like "rainbow tables" - should be somewhat expensive to discourage just trying all possible passwords against a (leaked) hash KDF is a key derivation function. The value will be used as a key in, say, AES. The important properties are: - should distribute entropy as well as possible, across the required width of output bits - reversibility less important as the derived key shouldn't be stored anywhere - may or may not want artificially inflated cost to discourage cracking
- Lvl999Noob 2y ago
  I still have no idea now haha. Your answer and Fabbari's are total opposites. If I am understanding right, you are saying that Password Hash is how a password should be stored while a KDF is not meant for storing passwords. Fabbari is saying the opposite of this, that KDF should be used for storing passwords while password hashes should not.
  - pjc50 2y ago
    Further discussion upthread under https://news.ycombinator.com/item?id=42957300 https://news.ycombinator.com/item?id=42957300
fabbari 2y ago
A password hash is a simple hash of a password. Hash algorithms are made to be fast. KDF - key deriving functions - are slow by design and are made to derive a key from a given string. They are designed to be slow to make password searching slower. This is a 2c tour of the topic.
- johnisgood 2y ago
  There are 2 comments to OP, and now the person can wonder which one is supposed to be slow or not.

jedisct1 2y ago

The bcrypt implementation in the Zig standard library has both the bcrypt() function (where truncation is explicitly documented) and the bcryptWithoutTruncation() function (which is recommended and automatically pre-hashes long passwords).

n0rdy 2y ago
Author here: thanks for reading the post. It's great to hear that Zig covered both cases. However, I'd still prefer the opposite behavior: a safe (without truncation) default `bcrypt()` and the unsafe function with the explicit name `bcryptWithTruncation()`. My opinion is based on the assumption that the majority of the users will go with the `bcrypt()` option. Having AI "helpers" might make this statistic even worse. Do you happen to know Zig team's reasoning behind this design choice? I'm really curious.
- masklinn 2y ago
  Note that the "safe" version makes very bespoke choices: it prehashes only overlong password, and does so with hmac-sha512 (which it b64-encodes). So it would very much be incompatible with other bcrypt implementations when outside of the "correct space". These choices are documented in the function's docstring, but not obvious, nor do they seem encoded in a custom version.
  - jszymborski 2y ago
    Sounds like it would then make sense to hide crypto primitives and their footguns under a "hazmat" or "danger" namespace, sorta like webcrypto or libsodium. So something like crypto.danger.bcrypt and crypto.bcryptWithTruncation
- jedisct1 2y ago
  `bcrypt()` is bcrypt as implemented everywhere else, and is required for interoperability with other implementations. If you don't truncate, this is not `bcrypt` any more. `bcryptWithTruncation()` is great for applications entirely written in Zig, but can create hashes that would not verify with other implementations. The documentation of these functions is very explicit about the difference. The verification function includes a `silently_truncate_password` option that is also pretty explicit.

tptacek 2y ago

Bcrypt is a password hash, not a KDF, which is the way it was used in this API. It's super unclear to me why they wanted a string-based KDF here at all; does anyone have more context? I've in the past been annoying about saying I think we should just call all password hashes "KDFs", but here's a really good illustration of why I was definitely wrong about that. A KDF is a generally-useful bit of cryptography joinery; a password hash has exactly one job.

masklinn 2y ago
The value is the combination of userid, username, and password, so in threads on other platforms people have hypothesised that the developer tried to play it safe and use a password hash because of the password's presence. Also I'm not sure the average developer understands the distinction.
- deleted 2y ago
  [deleted]
- dotancohen 2y ago
  > Also I'm not sure the average developer understands the distinction. I'm an average developer. I'm not sure that I understand exactly. What should I be reading, or what can you tell me? Thank you!
pclmulqdq 2y ago
They didn't want a KDF, as far as I know, but they wanted a hash function with unlimited input size. Including the username in the hash input gives you guaranteed domain separation between users that you don't get from salts/nonces. Its a generally good idea if you have a hash function with unlimited input size (all modern cryptographic hash functions except bcrypt have unlimited input size).
- masklinn 2y ago
  They clearly wanted something stronger than "a hash function" or they'd have reached for weaker cryptographic hashes.

coolgoose 2y ago

I am curious why bcrypt was used for hashing in the first place and not something like sha-512 Is there a reason I might be missing?

stavros 2y ago
Yes, the hashed payload contained a password, so presumably they didn't want to just SHA it.
- coolgoose 2y ago
  But why not bcrypt the password, but sha the cache key on top?
  - stavros 2y ago
    I guess because they didn't anticipate this flaw.
    - masklinn 2y ago
      Also prehashing opens you up to an other bcrypt flaw you need to be aware of: it stops at the first NUL byte, so you need to use some sort of binary-to-text encoding on top of the hash to ensure you don't have any of those in the data you ultimately hand off to bcrypt.
      - coolgoose 2y ago
        Thank you
      - Dylan16807 2y ago
        It's astounding how bad the default API for Bcrypt is.
- tptacek 2y ago
  Begs the question of why the payload contained a password, right?

nabla9 2y ago

> was used to generate the cache key where we hash a combined string of userId + username + password. Don't conceive your own cryptographic hacks. Use existing KDF designed by professionals.

edoceo 2y ago
Is the functions in libsodium enough? Provided they are used correctly?
- nabla9 2y ago
  Yes.
ludwik 2y ago
Simply hashing your data (using an established hashing algorithm/library combo) to later compare two hashes in order to check whether the data has changed doesn’t usually feel like rolling your own crypto.
- nabla9 2y ago
  The use case was KDF and they decided to do simple password hash signature hack instead by combining strings. They fucked it up.
  - ludwik 2y ago
    Of course they fucked it up, as evidenced by their bad security incident. The only question is whether you can really chalk this particular one up to a problem with "rolling your own crypto." That mantra exists for a reason, but it doesn’t feel like it really applies this time. It seems more like they used established crypto—just not the right one for this particular use case.
    - nabla9 2y ago
      Concatenating strings before giving it to the hash function instead of using KFD is rolling your own.

whalesalad 2y ago

damn that sounds like a rookie mistake for an organization who is literally in the business of secure auth

Tostino 2y ago

That is such a rookie mistake. It's not some hidden information that bcrypt has a 72 char limit. Pretty widely documented in multiple implementations and languages. How does a company whose only job is security screw that up so badly?

n0rdy 2y ago
> How does a company whose only job is security screw that up so badly? While I don't have any answers to this, I've realized that it's an ideal showcase of why fuzzy testing is useful.
CJefferson 2y ago
On the other hand, why not have implementations assert if they are given a string longer than 72 chars? It feels to me like no-one would ever do that on purpose, so it's a massive issue which is easy to accidentally make with a really important function.
- Tostino 2y ago
  Don't disagree there. I asked my self the same question the first few times I had to use it. Silently truncating the data is about the worst way to deal with it from a security standpoint. No idea why that decision was made back in the day.
- zorgmonkey 2y ago
  It is almost never I good idea to assert in a library, unless the error is truly unrecoverable. I think returning an error code\throwing an exception would be very reasonable and a much better API than failing silently though.
  - Dylan16807 2y ago
    An exception is fine if the language has them. I don't think "assert" was meant super literally and exactly the way C does it. An error code is risky.
  - 2y ago

sscarduzio 2y ago

what I would have naturally done without anticipating any flaw (and probably be just OK): cache_key = sha(sha(id + username) + bcrypt(pass)) with sha256 or something.

throwaway-9111 2y ago
Why not a simple sha(id + username + bcrypt(pass)) Is there any security issues with that? I'm a "newb" in this area, so I'm genuinely curious about the flaws with the naive approach

progmetaldev 2y ago

Reminds me of when I saw a junior developer calling SHA-1 on an incrementing integer ID, with no salt. We had a long talk about it, he thought it was too "scrambled" to allow anyone to recognize what was being done. He shouldn't have been so junior, he was 4 or 5 years into his career. I had to be the bad guy and override his decision without further discussing why it was a bad idea, and I really tried for a good 45 minutes to explain things. He got it a week later when I showed him rainbow tables, and I felt bad having to tell him to just do what I said for the solution, but sometimes you just have to make the decision to say "do what I said, I'm sorry you don't understand, I tried to explain."

bawolff 2y ago
Rainbow tables is not the (only) reason you dont want to hash something low entropy like an incrementing int, and adding a salt wouldn't make this secure. [Im assuming the usual definition of salt where it is known by the attacker... a pepper would be fine]
- progmetaldev 2y ago
  I agree, and I guess I did use salt differently than how most people see it, rather than how it is most effective. I never stored the salt in the database alongside the password. I would use something from the user that wouldn't change without a password change, as well as some type of semi-long data that also got hashed and put into the "pepper". Even if it's a file on disk that contains data that is read into memory and hashed with something that doesn't change (or at least can't change without the user also re-entering or creating a new password). Also, thank you for teaching me the term "pepper", because I feel like that is so relatable, but also different enough to correlate the two, but show how "pepper" is more powerful and useful!
ack_complete 2y ago
I've seen this before, a belief that just because the output looks random that it is secure. It's like storing license plates -- just hashing them without additional seasoning is of little use, because the number of possible license plates is so low that they can easily be brute forced. Similarly, a developer I worked with once claimed that CRC32 was sufficient verification because CRC32s changed so drastically depending on the data that they were difficult to forge. He was surprised to find out not only is it trivial to update a CRC32, but also to determine the CRC polynomial itself from very few samples.

jhhh 2y ago

I can see the incident was a jumping off point to talk about bad APIs (bcrypt probably should error >72) but it sounds like the actual bug was they weren't checking the value in the cache matched the data they used in the hash for the key. The authentication cache check should survive any arbitrarily bad hashing algorithm because all of them are going to have collisions (pigeonhole principal). Even an arbitrarily 'strong' hash function with no input truncation, as long as it has a fixed width result, will have this property. Thus, any arguing in the comments here about different hash functions with different truncation properties is moot. The analogy is something like creating a hash map whose insert function computes the slot for the key and unconditionally puts the value there instead of checking if the keys are the same during a collision. No amount of tinkering with the hash function fixes this problem. The algorithm is wrong. A hashmap should survive and be correct even giving it a hash function that always returns 4.

tialaramex 2y ago
I would guess that they felt comfortable that the bcrypt output (192 bits) is enough that collisions are very unlikely. If these were already partitioned by customer, rather than being a single cache for the entire Okta userbase that seems fine. You're going to have weird cosmic ray bugs more often than a natural collision. Now, the data structure they're using for a cache will use some sort of hash table, likely in memory, so maybe they've got the 192-bit bcrypt "key" and then that's hashed again, perhaps well or perhaps badly [e.g. C++ really likes using the identity function so hash(12345) = 12345] but then a modulo function is applied to find the key in an index and then we go groping about to look for the Key + Value pair. That part the API probably took care of, so even if the hash has size 6-bits, the full 192-bit key was checked. But the original data (userid:username:password) is not compared, only that 192-bit cache key.
semicolon_storm 2y ago
Not sure about that. A hash function suitable for security sensitive work, used properly, should make a collision so unlikely that you can basically forget it that it's even possible. Think about it, that's what hashing passwords relies on. We don't store a plaintext password for a final check if the password hash matches, we count on a collision being basically impossible. A hashmap is different, because it's using a much weaker hash function with far fewer security guarantees. Plus, you're assuming the original values are even kept around for comparison. The cache key likely just mapped to something simple like a boolean or status flag.

bawolff 2y ago

> On the other hand, such long usernames are not very usual, which I agree with Weird take. Usernames are often chosen by the user. Less so in corporate world but definitely not unheard of

SebFender 2y ago
Many of my usernames at my company are based on my email and it's pretty long - by the time you add the domain it's a good 47 characters...
- eesmith 2y ago
  I was at a Python conference once and met someone whose email ended '@boehringer-ingelheim.com'. That's 25 letters right there. https://www.boehringer-ingelheim.com/media-stories/press-releases https://www.boehringer-ingelheim.com/media-stories/press-rel... has a camilla.krogh_lauritzen@boehringer-ingelheim.com at 48 characters, for example.

ww520 2y ago

Have they did a bcrypt(password + userId + username), it won't be so bad. Order of entropy is important. Also I'm not sure what functionality the authentication cache provides, but their use of bcrypt(userId + username + password) implies the password is kept around somewhere, which is not the best practice. OT. Has Argon2 basically overtaken Bcrypt in password hashing in recent years?

buzer 2y ago
> Have they did a bcrypt(password + userId + username), it won't be so bad. Order of entropy is important. That depends on how exactly it was used. If it was simply used to check if previous authentication was successful (without the value containing information who it was successful for) then single long password could be used to authenticate as anyone.
- ww520 2y ago
  > single long password could be used to authenticate as anyone. Only if everyone uses the same long prefix for password.
  - buzer 2y ago
    No. If the value of the cache key is simply true/false then someone would first login to their own account using the long password. This would result in storing: bcrypt(longpassword + 123456 + me@foobar.com) = bcrypt(longpassword) = hash1 -> true If they then try login as you@bar.com using same password there would be a cache lookup: bcrypt(longpassword + 1111111 + you@bar.com) = bcrypt(longpassword) = hash1 -> true

SebFender 2y ago

I've seen this multiple times - even better I don't know how many ways we found a simple workaround or bypass of the complete process in so many apps... In essence this has nothing to do with the API itself but the way in which is another ballgame altogether. Great post though.

throwaway984393 2y ago

[dead]

renewiltord 2y ago

This is a completely unreasonable API. It reminds me of the `mysql_real_escape_string` vs. `mysql_escape_string`. The default API must be the strict one. You should be able to configure it to be broken but silent truncation is an insane piece of functionality. There is no universe in which this is logical. One might as well just have everything return void* and then put in the documentation what type to cast to. The invariant is clearly a historical accident. As a mistake, it's fine. Everyone writes up things like that. But defending it as an affirmatively good decision is wild.

benced 2y ago
Seriously, the number of “you weren’t meant to fire that foot gun” defenses in this thread…

sandeepkd 2y ago

I enjoyed the article and the detailed analysis for different languages. The conclusion is probably the part where most of the disagreement lies. API design is is not really at fault here if we consider the purpose of the API and the intended output. The API was designed to generate a hash for a password (knowledge factor) and for performance and practical reasons a limit has been picked up (72). The chances that some one knows your first 72 characters of password implies that the probably is a lot higher for the abuser to have remaining characters too. While smaller mistake here in my opinion was not knowing the full implementation details of a library, the bigger mistake was trying to use the library to generate hash of publicly available/visible information

AlfeG 2y ago
Ohhh, it's scrollable... I wondered why this small article gained so much attention...
- hansvm 2y ago
  Yeah, the fact that I can't have my mouse in the normal position and scroll the actual article was a problem 10 times or more while trying to read the thing...
hansvm 2y ago
> limit has been picked up (72) There's nothing wrong with a limit. The problem is that the library silently does the wrong thing when the limit is breached, rather than failing loudly.

underdeserver 2y ago

Hold on, in the Rust example, how does `err_on_truncation` get set? TFA completely ignored that there's a setting somewhere (probably incorrectly defaulting to false)

a-dub 2y ago
the rust library exposes a handful of "non_truncating_*" functions that enable error handling. i would expect this to be for drop-in compatibility with old code. amusingly, the python "library" is just a thin wrapper around the same rust library. protip: a lot of cryptography primitives actually aren't that complicated in terms of the code itself (and often can be quite elegant, compact and pleasing to the eye). if it's important, probably worth just reading it. it's what people wrap them with or the systems they build that get messy!
kmarc 2y ago
In the bcrypt crate there is an explicit method for it: bcrypt::non_truncating_hash() https://docs.rs/bcrypt/latest/bcrypt/ https://docs.rs/bcrypt/latest/bcrypt/ Funnily, TFA later also suggests that such function should exist...
- ratorx 2y ago
  Being pedantic, TFA suggests something slightly different. The non_truncating_hash should be the default (and called something that reflects it, eg. just hash), and a separate truncating_hash function may exist. The difference (from an API design perspective) is pretty massive.

llmthrow102 2y ago

How does anyone take Okta seriously after this incident btw?

philippta 2y ago

What's the reason behind bcrypt(userId + username + password) rather than just bcrypt(password) ?

ReptileMan 2y ago
rainbow tables I guess
Tade0 2y ago
What if two different users have the same password?
- magicalhippo 2y ago
  Bcrypt is salted[1], so that shouldn't matter? [1]: https://en.wikipedia.org/wiki/Bcrypt#Description https://en.wikipedia.org/wiki/Bcrypt#Description
  - Tade0 2y ago
    Are you sure? bcrypt stores the salt and retrieves it for comparison - otherwise you wouldn't be able to generate a matching hash. Consider the case where a user has a very long username and sets their password to their userId + username + password thus recreating the scenario which lead to the incident.
    - magicalhippo 2y ago
      That was not my point. My point was there wouldn't be a hash collision just by two users with the same password due to the salting.
      - Tade0 2y ago
        There's no hash collision here, just two different hashes, each with its own salt, matching the same original phrase. If you use only the password to generate the cache key, then this password will match regardless of salt, so users with the same password will generate a cache key matching that password.

withinboredom 2y ago

I'm really surprised they didn't cover PHP since (almost?) every framework uses bcrypt in php these days.

duskwuff 2y ago
PHP's password_* functions make it difficult to misuse in this particular way. There's no function in that API which hashes a password with a controllable salt and returns the result; there's only password_hash(), which always uses a random salt, and password_verify(), which rehashes a password internally and returns a bool for whether it matched. (It's still got the truncates-at-72 problem with PASSWORD_BCRYPT, though.)

mariocesar 2y ago

I'm confused, it seems that the OP wants to use Bcrypt as an encoding/decoding utility. About solutions, Django hashes by default the password only with a salt. I'm not sure why it would be valuable to combine user_id+username+password. I've always assumed that using salt+password was the best practice.

mariocesar 2y ago
Regarding the API design, I agree now with OP after reading other comments on HN. The API would be improved if it clearly indicates to the user when truncation is done, even if this understanding is implied by principle.

zero_k 2y ago

Another incident at Okta? Oh no! Its security has _always_ been a mess. It's a dumpster fire and no client of their cares because their identity systems are so messed up, that it's better to have the mess of Okta, than the mess they are sitting on. It's kinda crazy they get away with such incredibly bad security practices. Like... this bcrypt issue has been know for a LONG while. We used to test for it 8-10 years ago. There's either (1) nobody competent enough there to know (which is likely not true, I had a pentester friend recently join, and she is very good), or, more likely (2) management doesn't care and/or doesn't give enough authority to IT security personnel. As long as clients don't have any better options, Okta will stay this way.

BrandoElFollito 2y ago
> no client of their cares because their identity systems are so messed up, that it's better to have the mess of Okta, than the mess they are sitting on Yes, this is very true. Also some companies realize that they can screw up royally because they do no have the proper knowledge, and authentication is not a core business of theirs. I can understand them. I also use mail systems I am not that happy with, but I have this comforting idea that if they have a problem, 3B people are waiting together with me for it to be solved, and that's the kind of pressure that helps.

nfriedly 2y ago

I strongly agree with the conclusion that the libraries should reject input they can't correctly handle instead of silently truncating it. I co-maintain a rate-limiting library that had some similar rough edges, where it wouldn't always be obvious that you were doing it wrong. (For example: limiting the IP of your reverse proxy rather than the end user, or the inverse: blindly accepting any X-Forwarded-For header, including those potentially set by a malicious user.) A couple years back, I spent some time adding in runtime checks that detect those kinds of issues and log a warning. Since then, we've had a significant reduction in the amount of not-a-bug reports and, I assume, significantly fewer users with incorrect configurations.

deepsun 2y ago
If the input is 71 character, all the libraries happily accept it, but an attacker needs to guess only 1 character.
- arccy 2y ago
  have separate salt / pepper / user id args
  - tedunangst 2y ago
    How is the library supposed to know you're doing that wrong?
- cmgriffing 2y ago
  If these tools had a runtime check, then the cache key creation would have failed out. 72 is the max length of id, username, and password combined. If that combination is over 72, then failure and the cache key would not have been created. So, no, the attacker would not need to guess only one character of a password.

thekemkid 2y ago

In Node, you would commonly reach for the builtin core "node:crypto" module to run cryptographic functionality like this. I wondered why that wasn't used here, but bcryptjs was. After digging into it a little, node doesn't ship with core support for bcrypt, because it's not supported by OpenSSL. The node crypto module is essentially an API that offloads crypto work to OpenSSL. If we dig into OpenSSL, they won't support bcrypt. Bcrypt won't be supported by OpenSSL because of reasons to do with standardisation. https://github.com/openssl/openssl/issues/5323 https://github.com/openssl/openssl/issues/5323 Since bcrypt is not a "standardised" algorithm, it makes me wonder why Okta used it, at all? I remember in uni studying cryptography for application development and even then, back in 2013, it was used and recommended, but not standardised. it says a lot that 12 years on it still hasn't been.

jmuguy 2y ago

I was curious how bcrypt-ruby would handle this. It does not throw an error for input length larger than 72. However the actual API for the gem makes it pretty clear its for hashing a password, and not just hashing in general - as you can see from the code. https://gist.github.com/neontuna/dffd0452d09a0861106c0a46669a3ff0 https://gist.github.com/neontuna/dffd0452d09a0861106c0a46669...

veqq 2y ago

[u/forgot-CLHS](https://www.reddit.com/r/lisp/comments/1ikrz1g/shout_out_to_common_lisps_ironclad/ https://www.reddit.com/r/lisp/comments/1ikrz1g/shout_out_to_...) notes that Common Lisp's defact standard cryptography library Ironclad's [implementation](https://github.com/sharplispers/ironclad/blob/master/src/kdf/bcrypt.lisp https://github.com/sharplispers/ironclad/blob/master/src/kdf...) avoids such problems! ``` (defmethod derive-key ((kdf bcrypt) passphrase salt iteration-count key-length) (declare (type (simple-array (unsigned-byte 8) (*)) passphrase salt)) (unless (<= (length passphrase) 72) (error 'ironclad-error :format-control "PASSPHRASE must be at most 72 bytes long."))...) ```