auth0 / node-jsonwebtoken

JsonWebToken implementation for node.js http://self-issued.info/docs/draft-ietf-oauth-json-web-token.html
MIT License
17.73k stars 1.23k forks source link

Use a better algorithm than UTF-8 to derive keys from string secrets. #620

Open madmox opened 5 years ago

madmox commented 5 years ago

Currently, when I use the following code to generate a JWT:

const jwt = require('jsonwebtoken');
const secret = "my secret";
const token = jwt.sign({ "foo": "bar" }, secret); 

The actual binary key used for signing is derived from the secret using a simple UTF-8 string-to-byte.

I am surprised that the default sign method of the package uses no key derivation mechanism (like PBKDF2) to generate the signing key from given the secret. PBKDF2 does not solve the secret length issue, but it does mitigate the fact that simple UTF-8 string-to-byte is a very poor algorithm for key derivation, given that UTF-8 is not a bijection to binary data, and given that passphrases are generally plain ASCII strings, which has even less entropy than the full UTF-8 character set...

I raised an issue on the jws github project, but it could also be something to consider at the jsonwebtoken package level, e.g. change method signature and prefer buffer input rather than string secrets (buffer input is more likely to be generated from a truly random base64 key using Buffer.from(keyAsBase64String, "base64")).

panva commented 5 years ago

@madmox Buffer is already an accepted secret argument type, you're not required to pass a string.

const s = crypto.randomBytes(32).toString('hex')
const b = Buffer.from(string, 'hex')
jsonwebtoken.sign({}, b)

Should you want to use a KDF to derive the secret binary buffer you're free to do so after your token consumers do the same, we cannot however do this on a package level by default since we have stay interoperable with every other JWT/S module.

madmox commented 5 years ago

Thanks for the quick reply. I already use a similar setup in my projects (const key = Buffer.from(base64Key, "base64");). I understand the retrocompatibility issue, I just find it strange that the library's default behavior (jwt.sign({ "foo": "bar" }, "my-secret")) is something that should never be done, for obvious security reasons :/

This library is used by thousands of projects, and 95% of them just use simple string secrets, which are more than often plain alphanumeric strings, thus each key "byte" actually spans 62 values instead of 256, making JWT generated by this library very weak compared to others.

Speaking of compatibility with other libraries, this is not entirely true, as I know that at least Java's jjwt and dotnet's Jose.JWT don't behave the same way. They simply don't support string secrets in the sign methods and only accept byte arrays or compatible types (base64 strings, etc.), which is in my opinion the best way to educate people on generating strong keys.

The Java lib provides helpers to derive keys from string secrets (e.g. using PBKDF2), but the functionality is decoupled from the signing part - check this message from one of the Java lib authors in one of your closed issues for more infos.

I'm not saying you should change the method signatures overnight, but maybe a deprecation, or at least a documentation update, could be considered.

panva commented 5 years ago

We could certainly update the examples and documentation, keeping in mind it's not always the developer who's choosing the secret values, such as in cases of OAuth 2.0 client authentication assertions with a shared client_secret or an OIDC AS signing ID Tokens with HMAC based JWAs. In all these instances the one who generates the random secret uses sufficient entropy to generate a hex or base64/url string value which is then by said specifications used, e.g.

the octets of the UTF-8 representation of the client_secret are used as the key to validate the signature

Bottom line library can do more to educate in its README but still has to accept a string for its face value as the signing key.

I for one would love to see a proposal going to the appropriate IETF WG for extending the JWA alg support with HS based methods that use KDF to get their secrets rather than having each implementer "do its own thing".

madmox commented 5 years ago

You're right, this is OIDC spec, I'm a bit surprised :/ Perhaps they judged the loss of entropy is not that problematic.

still has to accept a string for its face value as the signing key.

Well it doesn't have to (except for backwards compatibility), but it's a conveniance method to avoid letting developers get the UTF-8 representation of the secret themselves (using Buffer.from(secret, "utf8")). Other libraries chose not to offer this conveniance, at the detriment of the number of lines of code to write I suppose.

Now I understand your choice better - it's more a matter of wanting the developer to better understand what he's doing or being more "plug'n'play".

panva commented 5 years ago

Would you like to propose a change to the README.md file?