Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JSON.stringify order is not deterministic, can lead to signature mismatches for same payload #404

Open
kevinburke opened this issue Sep 26, 2017 · 3 comments
Labels

Comments

@kevinburke
Copy link

If you pass an object as the payload to sign, it eventually gets serialized to a string by calling JSON.stringify, here: https://github.com/brianloveswords/node-jws/blob/master/lib/tostring.js#L9

The output order of JSON.stringify is not guaranteed. For example, one implementation may serialize {one: 'two', three: 'four'} as '{"one":"two","three":"four"}' and another may serialize it as '{"three":"four","one":"two"}'. The former would create a different MAC than the latter.

The latest ECMA spec for JSON.stringify says that it should produce the same order as Object.keys, which should produce the same order as for ... in, which uses Object.[[Enumerate]]() to enumerate object keys. [[Enumerate]] is a) deprecated, and b) the specification contains this note:

The mechanics and order of enumerating the properties is not specified but must conform to the rules specified below.

It might be fine if all you are doing is verifying that a generated signature is valid for the provided payload, and the verification operates on a provided string. But it would be confusing if signing the same payload generated different signatures on different Node versions, or if I generate two different JWT's using the same payload and secret key and, say, HMAC-SHA256, and expect the signatures to be equal.

In any event, you may want to provide a large warning saying that the key ordering isn't deterministic, and you may generate different signatures for the same payload and secret key pair.

@kevinburke
Copy link
Author

I proposed adding a fix for this in the standard library but it sounds like that is not going to happen, so you need to address the problem here.

nodejs/node#15628

@ziluvatar
Copy link
Contributor

ziluvatar commented Sep 29, 2017

Interesting, I didn't think about it, it's good to keep this issue open until we find a solution, however, I think you should open this issue in the library where the code lives on, in this case node-jws, once it is solved there we can update the version and close this issue.

Could you ask them?

@fenichelar
Copy link

auth0/node-jws#83

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants