[HN Gopher] Okta Bcrypt incident lessons for designing better APIs
___________________________________________________________________
Okta Bcrypt incident lessons for designing better APIs
Author : n0rdy
Score : 71 points
Date : 2025-02-05 21:10 UTC (1 hours ago)
(HTM) web link (n0rdy.foo)
(TXT) w3m dump (n0rdy.foo)
| jedisct1 wrote:
| 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 wrote:
| 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 wrote:
| 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.
| jedisct1 wrote:
| `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.
| masklinn wrote:
| Seems odd to keep the broken one as the default[0], make the
| "recommended" one so much longer, and provide none which simply
| errors on overlong passwords.
|
| Also neither seems to warn about the NUL issue.
|
| [0] I assume for compatibility purpose, but it still seems very
| dubious.
| IshKebab wrote:
| Disappointing that Zig would get the API design wrong too. I
| would have expected better.
|
| If you don't see the mistake, Google 'yaml.safe_load'.
| tptacek wrote:
| 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 wrote:
| 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.
| pclmulqdq wrote:
| 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 wrote:
| They clearly wanted something stronger than "a hash function"
| or they'd have reached for weaker cryptographic hashes.
| pclmulqdq wrote:
| They wanted a hard-to-compute cryptographic hash function.
| Today, that means bcrypt or something with a KDF
| construction. However, they needed one with unlimited input
| size, which rules out bcrypt.
| tptacek wrote:
| Or just a hash of the bcrypt hash, for the password!
|
| I don't like using thought-stopping cliches any more than
| anybody else does, but this design feels a little cargo-
| culted. All this stuff follows the more fundamental
| question of "why is the password mixed into a cache key"?
| pclmulqdq wrote:
| Yeah, I think both of the following would have worked if
| they wanted the password involved in a cache key and they
| wanted bcrypt to be used:
|
| * bcrypt(SHA-512(PW || stuff))
|
| * SHA(stuff || bcrypt(PW))
|
| Disclaimer: Not cryptography advice.
|
| It's still unclear to me why the password is in there.
| Vecr wrote:
| For 'unlimited' input size it should be SHA-3-512. Maybe
| too slow, but Bcrypt is slower, right? Less things to go
| wrong too.
| jedisct1 wrote:
| hmac-bcrypt solves that problem very well, and should
| replace plain bcrypt: https://github.com/epixoip/hmac-
| bcrypt
| tptacek wrote:
| I don't think it's a good idea for people to adopt new
| bcrypt constructions so that they can use it to generate
| cache keys (or, worse, other keys).
|
| (I need that "man standing up in the town hall meeting"
| meme for this.)
|
| Just use a real KDF, if that's really what you want. I'm
| still confused what password-derived material is doing in
| a Redis key.
| jedisct1 wrote:
| bcrypt-pbkdf (used in OpenSSH) exists for that purpose.
| hinkley wrote:
| I believe there have been earlier protocols where the user's
| secrets were used as a KDF to generate credentials in such a
| way that the server never sees the user's password.
|
| I'm wondering if okta was inspired by those.
| coolgoose wrote:
| 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 wrote:
| Yes, the hashed payload contained a password, so presumably
| they didn't want to just SHA it.
| coolgoose wrote:
| But why not bcrypt the password, but sha the cache key on
| top?
| stavros wrote:
| I guess because they didn't anticipate this flaw.
| masklinn wrote:
| 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 wrote:
| Thank you
| Dylan16807 wrote:
| It's astounding how bad the default API for Bcrypt is.
| tptacek wrote:
| Begs the question of why the payload contained a password,
| right?
| n0rdy wrote:
| There is a discussion about that on the security stackexchange
| (https://security.stackexchange.com/questions/133239/what-
| is-...). The TLDR:
|
| > SHA-2 family of hashes was designed to be fast. BCrypt was
| designed to be slow.
|
| Slow == harder to brute-force == more secure.
| progmetaldev wrote:
| Yes, and you can increase the work factor to make it slower
| to generate, specifically to fight against brute-force.
| nabla9 wrote:
| > 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.
| whalesalad wrote:
| damn that sounds like a rookie mistake for an organization who is
| literally in the business of secure auth
| Tostino wrote:
| 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 wrote:
| > 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 wrote:
| 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 wrote:
| 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.
| sscarduzio wrote:
| 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.
___________________________________________________________________
(page generated 2025-02-05 23:00 UTC)