oidc: at_hash / c_hash mismatch

[janekolszak]

Hi!
I get a c_hash mismatch when using https://www.npmjs.com/package/openid-client.

Query:

code=ayOThtgZ1P-nwPyCFUKlHxHtXKMoVuUozAbS9U6dUE0.uRBdJFQHGtL0Tcek3Ws1F2h0r1h_wkwFn2KJ07k59AM

id_token=eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJhdWQiOiJ3dGEtZCIsImF1dGhfdGltZSI6MTQ4Mjk2ODUyMywiY19oYXNoIjoiWlVVM1RVbGlOWGg2VUZST1pVb3lUWEJQT1VZM1FUMDkiLCJleHAiOjEuNDgyOTcyMTIzZSswOSwiaWF0IjoxLjQ4Mjk2ODUyM2UrMDksImlzcyI6Imh0dHBzOi8vb2lkYy53b3JrdGltZWFzc2lzdGFudC5jb20iLCJub25jZSI6IjhmYWU0NDUxLWRmMTItNGZlYy04Y2M0LTFmNWZmZjQzYjI4MyIsInN1YiI6ImIzMjhjNDcyLTRmMTktNDBmNy1hODM5LWRiMGY3ZTkwYmMyMyJ9.joNG_ZKjJh9z2LTSuFtK1cvK8xgnyx9W_nxx-rSxQYmNR6--382qtSbzK0GWF-qltaN_YZaj3hHVEBv5-brsXGaKhXSyN3J4RLG5qtr1ULGKehUDGJPV-n9NS4zL0GuohnVT8hzRnyQ9rAW2OE9upGL3W_6Jhs8WnTq5qd1_wvDDlRj6r08YrTfaL6Za3V_vD9jUvXvWb3GfS6jdLau2Rm0UL1zV126ODjU4AvhM0vKX2nsSlBHIz2Jwoh46GefxoYlIgsAk5oPNJNVcaxuPev8-4tufdhDJ6GwlQfGgXxo4VVBTjZhaedl1kkxYePNAda8ZofUOFh2LpjHwXmQyPYba9Y1ZWUwNsVknn_k7gwBdwQiEXb7AqzitySGbIP9AJms69ofd6bc8czR9WeaaKPP-GeOLb6WmH5wu88Ilrjo4pOcarv48fj1JoBLnE03a-BI6bDYIW2MXrdR3Rg_5_czLecpUSslKAPNpzTsKT2stSTdUKtcf6BOdfZyGUaqSlCrAQKKsLN55_AQ70Ipb1BmzILPgsFX3wdWUIt0rsiWRN-3x3J5o8ZykuRupZTIFsfyFxt3aGb8KpYfU-SsfePK8WcKN0TEo7d2BFFaQg7EjUOicayNJNqy4pjccMqszJc8ifq6qMHkAlY1zfbT4HDT130gn5As_Su0adu6hpGo

state=feb3fa83-7695-4a4d-9cd6-342a7f5a6baf

Decoded id_token:

{
  "alg": "RS256",
  "typ": "JWT"
}
{
  "aud": "wta-d",
  "auth_time": 1482968523,
  "c_hash": "ZUU3TUliNXh6UFROZUoyTXBPOUY3QT09",
  "exp": 1482972123,
  "iat": 1482968523,
  "iss": "https://oidc.worktimeassistant.com",
  "nonce": "8fae4451-df12-4fec-8cc4-1f5fff43b283",
  "sub": "b328c472-4f19-40f7-a839-db0f7e90bc23"
}

c_hash in the id_token seems to be too wrong. sha256sum returns:

echo -n ayOThtgZ1P-nwPyCFUKlHxHtXKMoVuUozAbS9U6dUE0.uRBdJFQHGtL0Tcek3Ws1F2h0r1h_wkwFn2KJ07k59AM | sha256sum

784ecc21be71ccf4cd789d8ca4ef45ec9a7d051b98747b26be93482e8e4eecf7

[aeneasr]

Thanks for reporting, I think I know what the issue is. Go is encoding []byte values as base64 when using json.Encode() which leads to encoding the c_hash value twice here and isn't catched by the unit tests, as Go also base64 decodes the values when it finds a []byte slice in the struct. Obviously, other languages don't know about this and thus the values are invalid

[janekolszak]

Maybe at_hash is also broken?

[aeneasr]

Yes, I think the bug applies to all hashes!

[aeneasr]

I hope the commit above resolves this issue. Would you mind trying it out? The PR is #329

[janekolszak]

Are the prerelease builds stored anywhere?

[aeneasr]

No unfortunately not, I'll just release it and fix it in the next minor release if there's still something wrong

[aeneasr]

I'll reopen if the issues persist

[janekolszak]

Please reopen. The seems to persist for "code" response_type:

wta://auth#code=l7KUM--gWVE2wRoQPj5D61r0byDaXdCsh5ed9SdYih4.q2mJqC4D_q-fOEjclrzOovIk8ESaSbusyFAeRrAroKU&id_token=eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJhdWQiOiJ3dGEtZCIsImF1dGhfdGltZSI6MTQ4MzIwOTY3NCwiY19oYXNoIjoiRTBHalJvamIyVlp5UzBHYnVndFIzUT09IiwiZXhwIjoxLjQ4MzIxMzI3NGUrMDksImlhdCI6MS40ODMyMDk2NzRlKzA5LCJpc3MiOiJodHRwczovL29pZGMud29ya3RpbWVhc3Npc3RhbnQuY29tIiwibm9uY2UiOiIwNGI0OWQwYy01YTczLTQyMmMtYWViNy1jMjdkNzZjMmFjNzIiLCJzdWIiOiJiMzI4YzQ3Mi00ZjE5LTQwZjctYTgzOS1kYjBmN2U5MGJjMjMifQ.tBufjkHkwLliL5G8oMeRXMlcVHCqkTYOC3WCtfHnBW6VlNETfCIYr1IU0ivC7HnxEY8ywMzim7SrKiktO-BDZ2zGZu1ZrCKHeQ_mfPKxY1q9sFX6bLzzBZ8b6klQujXumjhjG5dxy76S-1UzdJBeC8Eum1rtke_q2nXcoTBnt1B6J7S908ihjbryo5GA9wtnDntb-uvKFkyOwx_5fwoJIcTPhi5ZTcxmJufNR7DTJX0YRtiM31eKl8TcD9aJyLFmabYnrEM55Jk1m-6RZ41sOph6YyMN-lLNd1EuDuvEDsZjpTUoB3E9wNPC1BI0muclPo2ijFFABtexyOX1oZ5LGqQDvlGH8Oz70DjfUjC0Z0B_Lu2bjjz6WKb5MKn9BpQcKXRiEpO1sWOrk94ajtozBcZ3GOKCTnQzGQUokuPJ1gKfeW1nYr2B-5BAqxNdjCZjA4_SMyjqSOljy6fN2zM-ePahBU6Qwl1S1nitaC-rko3heBduFzq9lA6BLtUHDYgYyqYilppMiCQ9tPj2B3PN02cnhpY6UtsQSJdfUTjmS8jAra1kLN9YSdW7T0Df9sJcKAgHeXnOiG5Vc6eKurMLO4Y7qMi_a6hhT3iI18WjbwTvxM9k02591ZR9KPhgMN0bjVzsSM0EbG2VobBSJBRie08DpH-ca1r4_YFctT2U6RI&state=a02cc43a-2d57-464e-ad73-a5049587ac9d

[aeneasr]

Ah, apparently the problem now is the padding, Go adds two equals E0GjRojb2VZyS0GbugtR3Q== but oidc-token-hash (used by open-id-client) requires no padding E0GjRojb2VZyS0GbugtR3Q. The problem itself is within the oidc-token-hash library, which uses predefined lengths to validate the input, but does not strip away padding:

const LENGTHS = { 22: 'sha256', 32: 'sha384', 43: 'sha512' };
const ALGS = ['sha256', 'sha384', 'sha512'];

function validate(actual, token) {
  if (!actual) return false;
  const alg = LENGTHS[actual.length];
  if (!alg) return false;
  return generate(token, alg) === actual;
}

Since libraries should be able to deal with base64url encoding and padding (which is not omitted by default), I will create a patch for the dependency. I was not able to find information on padding in the oidc spec.

The patch is quite trivial:

function validate(actual, token) {
  if (!actual) return false;
  actual = actual.replace(/=+$/, '')
  const alg = LENGTHS[actual.length];
  if (!alg) return false;
  return generate(token, alg) === actual;
}

[aeneasr]

The PR is now here: panva/oidc-token-hash#1

Please notify the maintainers of the change, maybe they need to upgrade their package.json

[janekolszak]

Thank you!
But are you sure this is the right way? I'm certain the library already works with google OIDC.
Maybe hydra should strip the padding?

[aeneasr]

No, I'm not certain that the padding should be left in, but let's see what the maintainer says! The library (oidc-token-hash) does not seem to be certified and also not widely used (this was the first PR). Unfortunately, I could not find a oidc connect reference implementation for Go.

[janekolszak]

The linki https://www.npmjs.com/package/openid-client says it's certified.

[aeneasr]

Yes, that one is certified, I'm just not sure if that includes the oidc-token-hash dependency :)

[aeneasr]

Even the reference table on wikipedia is confusing :D

https://en.wikipedia.org/wiki/Base64#Implementations_and_history

[janekolszak]

And I can't find any base64url encoded values (in the docs and here:
https://community.pingidentity.com/servlet/fileField?retURL=%2Fapex%2FPingIdentityArticle%3Fid%3DkA340000000Gs7WCAS&entityId=ka340000000GvdRAAS&field=Associated_File__Body__s ) that contain ==

Maybe it's yet another thing that's undocumented but everyone does it.

[aeneasr]

Let's wait and see what the maintainer says. I'm open to remove the padding, just want to make sure it's not a special case :)

[panva]

= is not url safe, so not part of base64url :) but i'm happy to accept the PR as it does not necessarily break anything per-se

[aeneasr]

Ok, I think I'll just omit it then! Feel free to accept/close the PR :)

[aeneasr]

Resolved with https://github.com/ory-am/fosite/releases/tag/v0.6.11

[panva]

I think stripping the paddings to produce valid hashes is the way to go, nevertheless the PR is merged, version published and @janekolszak just needs to update his locked dependencies.

[aeneasr]

Ok, thanks for the quick response times!

[panva]

oidc-token-hash is simply a shared piece of code for node-openid-client and node-oidc-provider to produce and validate these hashes, made no sense to have the same js file in both libraries :)

[aeneasr]

@panva sorry if I ask so directly, but how did you get the library certified and did it cost money? I intend to get some certifications for this repo as well!

[panva]

@arekkas the tools and processes are linked at http://openid.net/certification/, they are btw great tools to validate your functionality, for example the padding would have been checked and rejected by the OP testing tool. Hit me up via email if you'd like more details.

[aeneasr]

Awesome, thank you so much!