Skip to content
This repository has been archived by the owner on Feb 27, 2023. It is now read-only.

jwe using Curve P521, ECDH-ES, and A256GCM does not always work with with other implementations #228

Closed
dodgeware opened this issue Apr 16, 2019 · 23 comments

Comments

@dodgeware
Copy link

dodgeware commented Apr 16, 2019

I am not able to encrypt/decrypt within Java and Go using Curve P521, ECDH-ES, and A256GCM using the same values for curve X, Y, and D. When I try to take the encrypted value from Java and Decrypt in Go it fails (about half the time) and vice versa it fails with:

Go: square/go-jose: error in cryptographic primitive

Sometimes it works though.

I am manually creating the KeyPair in both programs with the same X, Y, and D. Here is a link to my stackoverflow question.

https://stackoverflow.com/questions/55711277/why-do-i-get-an-error-decrypting-jwe-between-java-and-go

@csstaub
Copy link
Collaborator

csstaub commented Apr 16, 2019

Can you give us more information to help debug?

  • What version of Go and go-jose are you using?
  • What library are you using in Java to perform the JWE operations?
  • Do you have example messages/ciphertexts to test?

@dodgeware
Copy link
Author

dodgeware commented Apr 17, 2019

go version: go version go1.11.8 darwin/amd64
go-jose version from go mod: gopkg.in/square/go-jose.v2 v2.3.1

Java Libraries POM:

	<dependency>
		<groupId>com.nimbusds</groupId>
		<artifactId>nimbus-jose-jwt</artifactId>
		<version>7.0</version>
	</dependency>
	<dependency>
		<groupId>org.bouncycastle</groupId>
		<artifactId>bcprov-jdk15on</artifactId>
		<version>1.61</version>
	</dependency>

JWK:
{
"kty":"EC",
"d":"AGPoHUdXajyyRLV0bAokQTnDzlO7Kjs1zSucSu69CGfSwpg7oXSxlfptApD-5O47d1PX3y0ag5228XsPFXVzYnH0",
"crv":"P-521",
"x":"AVuFsno89wJ5xT2z63iznxVO8H5gsfcHmS1XJ_JbfEzIsudqjrvKGrzxJT96-dmP_NY7KeMvyJEUInmqcqCWbzcQ",
"y":"ANv5hayQ3_TwMcFqPrtw-a9wNkfQuynuWhhbWOXYvGArdvibDGYRIRx3O5gAjfyumpibZFQ0K0jrjb09YP3AVbtc"
}

Encryption Text: "test string"

Sample Encryption from Java (copied into Go Program and attempt decryption):

eyJlcGsiOnsia3R5IjoiRUMiLCJjcnYiOiJQLTUyMSIsIngiOiJBUHVxYm5Kd29sSnV4NzRLVjRGSlJDMUZDejVFRzVxQ0pkNlh4TThkVm5DVTlrR2lwaktOVWk2T3BORWx1d0xOVzVHV1ozRktNa18wcWFSX0lTTko3Uk1KIiwieSI6IkFYUW5UWkRUOUNrTEtzYnAwelBoeEZwVC1CQkQwVmh2S3BIZ0o4VTVYdFM1M09VVzRaeEpmbTZZbkplZEFjV2lMRm1LVlJzR3lrYlN4YjhmdTUwbkh3SjcifSwiZW5jIjoiQTI1NkdDTSIsImFsZyI6IkVDREgtRVMifQ..6GaCfQBmGoZqdSdp.yid9Ty4obq77NwY.g7R-OpwzH4ySkPqcuO6y_w

Program Output:

X: 4659511641062964782021875499623429969441821155897294879230806072110741671346740049237806735165617028886069507735635813077932132536190200253111449908755248912
Y: 2949378472807810092520841653271031714422549745831173574227955697023791698149385034684706074532264566125936337443094912552337043915553882158181087772690201436
D: 1339529801035093667883223508197773343470746385462856606416195633179696731618091331774947385000382341345692790110247233785645952796728460311294997483026018804

Encrypted: eyJlcGsiOnsia3R5IjoiRUMiLCJjcnYiOiJQLTUyMSIsIngiOiJBUHVxYm5Kd29sSnV4NzRLVjRGSlJDMUZDejVFRzVxQ0pkNlh4TThkVm5DVTlrR2lwaktOVWk2T3BORWx1d0xOVzVHV1ozRktNa18wcWFSX0lTTko3Uk1KIiwieSI6IkFYUW5UWkRUOUNrTEtzYnAwelBoeEZwVC1CQkQwVmh2S3BIZ0o4VTVYdFM1M09VVzRaeEpmbTZZbkplZEFjV2lMRm1LVlJzR3lrYlN4YjhmdTUwbkh3SjcifSwiZW5jIjoiQTI1NkdDTSIsImFsZyI6IkVDREgtRVMifQ..6GaCfQBmGoZqdSdp.yid9Ty4obq77NwY.g7R-OpwzH4ySkPqcuO6y_w

panic: square/go-jose: error in cryptographic primitive

goroutine 1 [running]:
main.testgojmx()
/Users/mjz340/Repos/BrandenDodge/go-jose-test/main.go:155 +0x803
main.main()
/Users/mjz340/Repos/BrandenDodge/go-jose-test/main.go:28 +0x20

@dodgeware dodgeware changed the title jwe using Curve P521, ECDH-ES, and A256GCM does not allows work with with other implementations jwe using Curve P521, ECDH-ES, and A256GCM does not always work with with other implementations Apr 17, 2019
@dodgeware
Copy link
Author

dodgeware commented Apr 17, 2019

Here is a sample encryption (just keep running the java program encrypting the same text until I find one that is successful decrypting in go) that works:

eyJlcGsiOnsia3R5IjoiRUMiLCJjcnYiOiJQLTUyMSIsIngiOiJBTWpjeWNSNW9yWW9sRDZoa1ByM0NGVlVHNm8tRjVreFNpNHJ4dUNGVXNsRGcwUTg1Q2lJWXJtZmRuR1MzblZoR3lvV0RXVEJTakpJTXpoV3ZSWDV0ODZEIiwieSI6IkFYRk82TklsNFY2MnB2OGhGY0kwRk14QWFmbkZ0MDQxSzJSUFlEYks1TG5rZFRtOXgwcGdiWlk5ck9rdEpTdXZUMnk2WGdfS0tNTjhqdk9OaE1ZdDJkV20ifSwiZW5jIjoiQTI1NkdDTSIsImFsZyI6IkVDREgtRVMifQ..VuJ8ahL6lKq1nez7.8wuC1K6mxM_rtzo.HBs5Hd2N1s-03Sg6lKuQhg

@csstaub
Copy link
Collaborator

csstaub commented Apr 17, 2019

Is that encrypted with (to) the same key as the broken one?

@dodgeware
Copy link
Author

dodgeware commented Apr 17, 2019

Yes... this is how the keypair is generated in Java: ( I then printed it out and parsed in Go to a keypair)

String x = "AVuFsno89wJ5xT2z63iznxVO8H5gsfcHmS1XJ_JbfEzIsudqjrvKGrzxJT96-dmP_NY7KeMvyJEUInmqcqCWbzcQ";
String y = "ANv5hayQ3_TwMcFqPrtw-a9wNkfQuynuWhhbWOXYvGArdvibDGYRIRx3O5gAjfyumpibZFQ0K0jrjb09YP3AVbtc";
String d = "AGPoHUdXajyyRLV0bAokQTnDzlO7Kjs1zSucSu69CGfSwpg7oXSxlfptApD-5O47d1PX3y0ag5228XsPFXVzYnH0";

return new ECKey.Builder(Curve.P_521, new Base64URL(x), new Base64URL(y))
.d(new Base64URL(d))
.build();

@csstaub
Copy link
Collaborator

csstaub commented Apr 17, 2019

Taking a quick look I don't see any obvious differences. Can you try this with P-256 instead of P-521? I have a suspicion that the bug might stem from the fact that the x/y params must be padded, and the 521-bit x/y values from P-521 don't round nicely to a byte boundary. It looks like the x/y values in the header are, indeed, 66 bytes in length, but maybe they were padded incorrectly.

@dodgeware
Copy link
Author

I have run it 5 times with no errors using P-256. That never happened using P-521. The problem is I have to use P-521 to be compatible with client applications.

@csstaub
Copy link
Collaborator

csstaub commented Apr 17, 2019

That means there's likely a bug in how either go-jose or nimbus-jose-jwt handles the padding on the 521-bit ephemeral key embedded in the header.

Here's the X and Y of the EPK on the working ciphertext:

❯❯❯ echo 'eyJlcGsiOnsia3R5IjoiRUMiLCJjcnYiOiJQLTUyMSIsIngiOiJBTWpjeWNSNW9yWW9sRDZoa1ByM0NGVlVHNm8tRjVreFNpNHJ4dUNGVXNsRGcwUTg1Q2lJWXJtZmRuR1MzblZoR3lvV0RXVEJTakpJTXpoV3ZSWDV0ODZEIiwieSI6IkFYRk82TklsNFY2MnB2OGhGY0kwRk14QWFmbkZ0MDQxSzJSUFlEYks1TG5rZFRtOXgwcGdiWlk5ck9rdEpTdXZUMnk2WGdfS0tNTjhqdk9OaE1ZdDJkV20ifSwiZW5jIjoiQTI1NkdDTSIsImFsZyI6IkVDREgtRVMifQ..VuJ8ahL6lKq1nez7.8wuC1K6mxM_rtzo.HBs5Hd2N1s-03Sg6lKuQhg' | jose-util expand --format JWE | jq -r .protected | b64decode | jq -r .epk.x | b64decode | hexdump -C
00000000  00 c8 dc c9 c4 79 a2 b6  28 94 3e a1 90 fa f7 08  |.....y..(.>.....|
00000010  55 54 1b aa 3e 17 99 31  4a 2e 2b c6 e0 85 52 c9  |UT..>..1J.+...R.|
00000020  43 83 44 3c e4 28 88 62  b9 9f 76 71 92 de 75 61  |C.D<.(.b..vq..ua|
00000030  1b 2a 16 0d 64 c1 4a 32  48 33 38 56 bd 15 f9 b7  |.*..d.J2H38V....|
00000040  ce 83                                             |..|
❯❯❯ echo 'eyJlcGsiOnsia3R5IjoiRUMiLCJjcnYiOiJQLTUyMSIsIngiOiJBTWpjeWNSNW9yWW9sRDZoa1ByM0NGVlVHNm8tRjVreFNpNHJ4dUNGVXNsRGcwUTg1Q2lJWXJtZmRuR1MzblZoR3lvV0RXVEJTakpJTXpoV3ZSWDV0ODZEIiwieSI6IkFYRk82TklsNFY2MnB2OGhGY0kwRk14QWFmbkZ0MDQxSzJSUFlEYks1TG5rZFRtOXgwcGdiWlk5ck9rdEpTdXZUMnk2WGdfS0tNTjhqdk9OaE1ZdDJkV20ifSwiZW5jIjoiQTI1NkdDTSIsImFsZyI6IkVDREgtRVMifQ..VuJ8ahL6lKq1nez7.8wuC1K6mxM_rtzo.HBs5Hd2N1s-03Sg6lKuQhg' | jose-util expand --format JWE | jq -r .protected | b64decode | jq -r .epk.y | b64decode | hexdump -C
00000000  01 71 4e e8 d2 25 e1 5e  b6 a6 ff 21 15 c2 34 14  |.qN..%.^...!..4.|
00000010  cc 40 69 f9 c5 b7 4e 35  2b 64 4f 60 36 ca e4 b9  |.@i...N5+dO`6...|
00000020  e4 75 39 bd c7 4a 60 6d  96 3d ac e9 2d 25 2b af  |.u9..J`m.=..-%+.|
00000030  4f 6c ba 5e 0f ca 28 c3  7c 8e f3 8d 84 c6 2d d9  |Ol.^..(.|.....-.|
00000040  d5 a6                                             |..|

And here's the broken one:

❯❯❯ echo 'eyJlcGsiOnsia3R5IjoiRUMiLCJjcnYiOiJQLTUyMSIsIngiOiJBUHVxYm5Kd29sSnV4NzRLVjRGSlJDMUZDejVFRzVxQ0pkNlh4TThkVm5DVTlrR2lwaktOVWk2T3BORWx1d0xOVzVHV1ozRktNa18wcWFSX0lTTko3Uk1KIiwieSI6IkFYUW5UWkRUOUNrTEtzYnAwelBoeEZwVC1CQkQwVmh2S3BIZ0o4VTVYdFM1M09VVzRaeEpmbTZZbkplZEFjV2lMRm1LVlJzR3lrYlN4YjhmdTUwbkh3SjcifSwiZW5jIjoiQTI1NkdDTSIsImFsZyI6IkVDREgtRVMifQ..6GaCfQBmGoZqdSdp.yid9Ty4obq77NwY.g7R-OpwzH4ySkPqcuO6y_w' | jose-util expand --format JWE | jq -r .protected | b64decode | jq -r .epk.x | b64decode | hexdump -C
00000000  00 fb aa 6e 72 70 a2 52  6e c7 be 0a 57 81 49 44  |...nrp.Rn...W.ID|
00000010  2d 45 0b 3e 44 1b 9a 82  25 de 97 c4 cf 1d 56 70  |-E.>D...%.....Vp|
00000020  94 f6 41 a2 a6 32 8d 52  2e 8e a4 d1 25 bb 02 cd  |..A..2.R....%...|
00000030  5b 91 96 67 71 4a 32 4f  f4 a9 a4 7f 21 23 49 ed  |[..gqJ2O....!#I.|
00000040  13 09                                             |..|
❯❯ echo 'eyJlcGsiOnsia3R5IjoiRUMiLCJjcnYiOiJQLTUyMSIsIngiOiJBUHVxYm5Kd29sSnV4NzRLVjRGSlJDMUZDejVFRzVxQ0pkNlh4TThkVm5DVTlrR2lwaktOVWk2T3BORWx1d0xOVzVHV1ozRktNa18wcWFSX0lTTko3Uk1KIiwieSI6IkFYUW5UWkRUOUNrTEtzYnAwelBoeEZwVC1CQkQwVmh2S3BIZ0o4VTVYdFM1M09VVzRaeEpmbTZZbkplZEFjV2lMRm1LVlJzR3lrYlN4YjhmdTUwbkh3SjcifSwiZW5jIjoiQTI1NkdDTSIsImFsZyI6IkVDREgtRVMifQ..6GaCfQBmGoZqdSdp.yid9Ty4obq77NwY.g7R-OpwzH4ySkPqcuO6y_w' | jose-util expand --format JWE | jq -r .protected | b64decode | jq -r .epk.y | b64decode | hexdump -C
00000000  01 74 27 4d 90 d3 f4 29  0b 2a c6 e9 d3 33 e1 c4  |.t'M...).*...3..|
00000010  5a 53 f8 10 43 d1 58 6f  2a 91 e0 27 c5 39 5e d4  |ZS..C.Xo*..'.9^.|
00000020  b9 dc e5 16 e1 9c 49 7e  6e 98 9c 97 9d 01 c5 a2  |......I~n.......|
00000030  2c 59 8a 55 1b 06 ca 46  d2 c5 bf 1f bb 9d 27 1f  |,Y.U...F......'.|
00000040  02 7b                                             |.{|

Unfortunately I don't see an obvious difference here. I don't have a Java env set up right now, could you instrument nimus-jose-jwt such that it dumps the X/Y values for the ephemeral public key used for ECDH and then compare those values to what it outputs in the final message?

@dodgeware
Copy link
Author

Got your message. I will but it will be a little while. Got pulled in another direction.

@Spomky
Copy link

Spomky commented Apr 18, 2019

Hi,

As commented in this SO question, the EPK looks to be on the curve. My assumption is that the agreed key is wrongly computed.

Can you compute the key agreement for the following keys?

Key 1 => {"kty":"EC","crv":"P-521","x":"BaugQuRCQzkSQ46nXbRFxp4mrHajnEuVT0YCHheuNQzMp7y7pVNL30ABSBtNSi5EBsyal8DUHsYIIXAmai57E1E","y":"AXg4ZO6W6AYx-B_CTS_HlIId4aLuAZATtNG6ythg8FxwOWbtmzP86bAbvigQ_GSbGskoodEkkFlFf7OZ0n1g9VPc","d":"BZI3mr4hrdlK6LXr3FZ_XHFJet2_iDKRQsmtAdzj-MMjIDyD3KulZ5wpg2oG5n4oS1T4wTMiCpg-iQWSaH7b1zI"}
Key 2 => {"kty":"EC","crv":"P-521","x":"NkK3GcAJkPACowAvtztU7p3l9zYawjv-mfud4BNPn00jdKejjb-QNGG0TfHi9HC6spHcqD9efPctjF1SEfAZQss","y":"nEMEGtGEJNIyO2GhO2reECCu7ZELOu9liSQtCDZpGVogY8BlrZtoT1RhtsRCzYF0N-zV0GgLCNkDrmYimlsibUs","d":"ATFPB85Gr75k-Tr4wdwkoV8X2RyzVOw2IdVICKg8eidc4Sl0VqUKwKSBkVIn4Dd1soIN2_aPzYIQfI_n4Snp5IWk"}

You should have 86c7d0c862e836a999b537df8542ab91d5788514669edc9de916dd3f77d52a71ce9fa5e476205ee167a757523cc2c6813a9e4730e3b3b0c1588ff720425b3f6a4a (in HEX).

@csstaub
Copy link
Collaborator

csstaub commented Apr 18, 2019

I can try that, but the computation path for P-256 and P-521 is the same. Given that it works with one curve but not the other I'm skeptical that the issue is in there. Right now I believe it is an issue with how the X/Y values are parsed or serialized on one or the other side. For the JWKs posted above, what are the X/Y values in decimal that the Java library had parsed (generated) at runtime?

@csstaub
Copy link
Collaborator

csstaub commented Apr 18, 2019

@Spomky What are the alg-id, apu/apv, values you used for that key derivation?

@Spomky
Copy link

Spomky commented Apr 18, 2019

I can try that, but the computation path for P-256 and P-521 is the same. Given that it works with one curve but not the other I'm skeptical that the issue is in there. Right now I believe it is an issue with how the X/Y values are parsed or serialized on one or the other side. For the JWKs posted above, what are the X/Y values in decimal that the Java library had parsed (generated) at runtime?

I’m not convinced the issue comes from the serialization of the point.
Decimal values are:

  • x(Key 1): 76027823716625994586287801504354590904740274333306306229831335861872211046533430383183170729063777452517027314173449219074203783288553453189324501763691345

  • y(Key 1):
    5044289389007378139368377120387415230891771794757927039967812398177656791214965351396092833521513650312827810286051481348272486194568012851465321047387362268

  • x(Key 2): 727515788682599477939529584943965672003252492223950034706429731590457294997124739272632480873500661542022783429927428042660688270293625991119475538277253835

  • y(Key 2): 2095127951582776989108268120769401489602868619145867447482535563574141678172406162792153682021882557985612400866451518832497994856482967634423957641226251595

What are the alg-id, apu/apv, values you used for that key derivation?

The alg ID corresponds to the content encryption algorithm i.e. A256GCM in that case.
apu/apv are not used here and are empty strings. These values correspond to the apu and apv header parameter if present (base64 url-safe decoded values).

The PHP implementation I use may help you.

With the A256GCM algorithms, the values used to derive the CEK are:

  • $Z: the agreed key computed from Key 1 and 2 (0x86c7d0c862e836a999b537df8542ab91d5788514669edc9de916dd3f77d52a71ce9fa5e476205ee167a757523cc2c6813a9e4730e3b3b0c1588ff720425b3f6a4a),
  • $algorithm: A256GCM
  • $encryption_key_size: 256 bits
  • $apu/$apv: empty strings

Result should be 0xe1813a1bb50c00627e83e9895db6b4f86d1fe8892b64b38013f82ec187ba64e0

@csstaub
Copy link
Collaborator

csstaub commented Apr 18, 2019

Tried to reproduce that, but wasn't able to. The keys you posted seem to have X/Y values that are of the incorrect length:

❯❯❯ echo '{"kty":"EC","crv":"P-521","x":"BaugQuRCQzkSQ46nXbRFxp4mrHajnEuVT0YCHheuNQzMp7y7pVNL30ABSBtNSi5EBsyal8DUHsYIIXAmai57E1E","y":"AXg4ZO6W6AYx-B_CTS_HlIId4aLuAZATtNG6ythg8FxwOWbtmzP86bAbvigQ_GSbGskoodEkkFlFf7OZ0n1g9VPc","d":"BZI3mr4hrdlK6LXr3FZ_XHFJet2_iDKRQsmtAdzj-MMjIDyD3KulZ5wpg2oG5n4oS1T4wTMiCpg-iQWSaH7b1zI"}' | jq -r .x | b64decode | wc -c
      65
❯❯❯ echo '{"kty":"EC","crv":"P-521","x":"BaugQuRCQzkSQ46nXbRFxp4mrHajnEuVT0YCHheuNQzMp7y7pVNL30ABSBtNSi5EBsyal8DUHsYIIXAmai57E1E","y":"AXg4ZO6W6AYx-B_CTS_HlIId4aLuAZATtNG6ythg8FxwOWbtmzP86bAbvigQ_GSbGskoodEkkFlFf7OZ0n1g9VPc","d":"BZI3mr4hrdlK6LXr3FZ_XHFJet2_iDKRQsmtAdzj-MMjIDyD3KulZ5wpg2oG5n4oS1T4wTMiCpg-iQWSaH7b1zI"}' | jq -r .y | b64decode | wc -c
      66
❯❯❯ echo '{"kty":"EC","crv":"P-521","x":"NkK3GcAJkPACowAvtztU7p3l9zYawjv-mfud4BNPn00jdKejjb-QNGG0TfHi9HC6spHcqD9efPctjF1SEfAZQss","y":"nEMEGtGEJNIyO2GhO2reECCu7ZELOu9liSQtCDZpGVogY8BlrZtoT1RhtsRCzYF0N-zV0GgLCNkDrmYimlsibUs","d":"ATFPB85Gr75k-Tr4wdwkoV8X2RyzVOw2IdVICKg8eidc4Sl0VqUKwKSBkVIn4Dd1soIN2_aPzYIQfI_n4Snp5IWk"}' | jq -r .x | b64decode | wc -c
      65
❯❯❯ echo '{"kty":"EC","crv":"P-521","x":"NkK3GcAJkPACowAvtztU7p3l9zYawjv-mfud4BNPn00jdKejjb-QNGG0TfHi9HC6spHcqD9efPctjF1SEfAZQss","y":"nEMEGtGEJNIyO2GhO2reECCu7ZELOu9liSQtCDZpGVogY8BlrZtoT1RhtsRCzYF0N-zV0GgLCNkDrmYimlsibUs","d":"ATFPB85Gr75k-Tr4wdwkoV8X2RyzVOw2IdVICKg8eidc4Sl0VqUKwKSBkVIn4Dd1soIN2_aPzYIQfI_n4Snp5IWk"}' | jq -r .y | b64decode | wc -c
      65

All of these need to be correctly padded to 66 bytes for P-521, as 521/8 = 65.125, which yields 66 bytes rounded up. See here: https://tools.ietf.org/html/rfc7518#section-6.2.1.2

@Spomky
Copy link

Spomky commented Apr 19, 2019

Indeed that’s correct. Here are the correct keys:

  • {"kty":"EC","crv":"P-521","x":"AAWroELkQkM5EkOOp120RcaeJqx2o5xLlU9GAh4XrjUMzKe8u6VTS99AAUgbTUouRAbMmpfA1B7GCCFwJmouexNR","y":"AXg4ZO6W6AYx-B_CTS_HlIId4aLuAZATtNG6ythg8FxwOWbtmzP86bAbvigQ_GSbGskoodEkkFlFf7OZ0n1g9VPc","d":"AAWSN5q-Ia3ZSui169xWf1xxSXrdv4gykULJrQHc4_jDIyA8g9yrpWecKYNqBuZ-KEtU-MEzIgqYPokFkmh-29cy"}
  • {"kty":"EC","crv":"P-521","x":"ADZCtxnACZDwAqMAL7c7VO6d5fc2GsI7_pn7neATT59NI3Sno42_kDRhtE3x4vRwurKR3Kg_Xnz3LYxdUhHwGULL","y":"AJxDBBrRhCTSMjthoTtq3hAgru2RCzrvZYkkLQg2aRlaIGPAZa2baE9UYbbEQs2BdDfs1dBoCwjZA65mIppbIm1L","d":"ATFPB85Gr75k-Tr4wdwkoV8X2RyzVOw2IdVICKg8eidc4Sl0VqUKwKSBkVIn4Dd1soIN2_aPzYIQfI_n4Snp5IWk"}

This does not change the key agreement and the computed derived key values.

@csstaub
Copy link
Collaborator

csstaub commented Apr 19, 2019

Thanks @Spomky. I'm getting the same results for ECDH as you are:

❯❯❯ go test -v -run=TestECDH
=== RUN   TestECDHKeyAgreement
k1.x: 76027823716625994586287801504354590904740274333306306229831335861872211046533430383183170729063777452517027314173449219074203783288553453189324501763691345
k1.y: 5044289389007378139368377120387415230891771794757927039967812398177656791214965351396092833521513650312827810286051481348272486194568012851465321047387362268
k2.x: 727515788682599477939529584943965672003252492223950034706429731590457294997124739272632480873500661542022783429927428042660688270293625991119475538277253835
k2.y: 2095127951582776989108268120769401489602868619145867447482535563574141678172406162792153682021882557985612400866451518832497994856482967634423957641226251595
s: e1813a1bb50c00627e83e9895db6b4f86d1fe8892b64b38013f82ec187ba64e0

Here's the test code I ran:

func TestECDHKeyAgreement(t *testing.T) {
	raw1 := `{"kty":"EC","crv":"P-521","x":"AAWroELkQkM5EkOOp120RcaeJqx2o5xLlU9GAh4XrjUMzKe8u6VTS99AAUgbTUouRAbMmpfA1B7GCCFwJmouexNR","y":"AXg4ZO6W6AYx-B_CTS_HlIId4aLuAZATtNG6ythg8FxwOWbtmzP86bAbvigQ_GSbGskoodEkkFlFf7OZ0n1g9VPc","d":"AAWSN5q-Ia3ZSui169xWf1xxSXrdv4gykULJrQHc4_jDIyA8g9yrpWecKYNqBuZ-KEtU-MEzIgqYPokFkmh-29cy"}`
	raw2 := `{"kty":"EC","crv":"P-521","x":"ADZCtxnACZDwAqMAL7c7VO6d5fc2GsI7_pn7neATT59NI3Sno42_kDRhtE3x4vRwurKR3Kg_Xnz3LYxdUhHwGULL","y":"AJxDBBrRhCTSMjthoTtq3hAgru2RCzrvZYkkLQg2aRlaIGPAZa2baE9UYbbEQs2BdDfs1dBoCwjZA65mIppbIm1L","d":"ATFPB85Gr75k-Tr4wdwkoV8X2RyzVOw2IdVICKg8eidc4Sl0VqUKwKSBkVIn4Dd1soIN2_aPzYIQfI_n4Snp5IWk"}`

	parsed1 := JSONWebKey{}
	parsed2 := JSONWebKey{}
	var err error
	err = json.Unmarshal([]byte(raw1), &parsed1)
	assert.Nil(t, err)
	err = json.Unmarshal([]byte(raw2), &parsed2)
	assert.Nil(t, err)

	key1 := parsed1.Key.(*ecdsa.PrivateKey)
	key2 := parsed2.Key.(*ecdsa.PrivateKey)

	pub1 := key1.PublicKey
	pub2 := key2.PublicKey

	shared := josecipher.DeriveECDHES(
		"A256GCM",
		[]byte{},
		[]byte{},
		key1,
		&pub2,
		32,
	)

	fmt.Fprintf(os.Stderr, "k1.x: %s\n", pub1.X.String())
	fmt.Fprintf(os.Stderr, "k1.y: %s\n", pub1.Y.String())
	fmt.Fprintf(os.Stderr, "k2.x: %s\n", pub2.X.String())
	fmt.Fprintf(os.Stderr, "k2.y: %s\n", pub2.Y.String())
	fmt.Fprintf(os.Stderr, "s: %s\n", hex.EncodeToString(shared))
}

@csstaub
Copy link
Collaborator

csstaub commented Apr 19, 2019

So, this bug is still a bit of a mystery. @dodgeware I'd be curious to know what the internal X/Y values are in the Java version and what algorithm id is passed to the derivation. Maybe there's a disagreement there somewhere.

@dodgeware
Copy link
Author

I am not available to do this right now. As soon as I get time to come back to this I will.

@csstaub
Copy link
Collaborator

csstaub commented May 31, 2019

I managed to reproduce this with jose4j, here's what I found.

First, the code to repro:

func TestPrecomputedECDHMessagesFromJose4j(t *testing.T) {
	data := []struct{ key, message string }{
		{
			`{"kty":"EC","x":"fXx-DfOsmecjKh3VrLZFsF98Z1nutsL4UdFTdgA8S7Y","y":"LGzyJY99aqKk52UIExcNFSTs0S7HnNzQ-DRWBTHDad4","crv":"P-256","d":"OeVCWbXuFuJ9U16q7bhLNoKPLLnK-yTx95grzfvQ2l4"}`,
			`eyJlbmMiOiJBMjU2Q0JDLUhTNTEyIiwiYWxnIjoiRUNESC1FUyIsImVwayI6eyJrdHkiOiJFQyIsIngiOiJ3ZlRHNVFHZkItNHUxanVUUEN1aTNESXhFTV82ZUs5ZEk5TXNZckpxWDRnIiwieSI6Ik8yanlRbHQ2TXFGTGtqMWFCWW1aNXZJWHFVRHh6Ulk3dER0WmdZUUVNa0kiLCJjcnYiOiJQLTI1NiJ9fQ..mk4wQzGSSeZ8uSgEYTIetA.fCw3-TosL4p0D5fEXw0bEA.9mPsdmGTVoVexXqEOdN5VUKk-ZNtfOtUfbdjVHoko_o`,
		},
		{
			`{"kty":"EC","x":"nBr92fh2JsEjIF1LR5PKICBeHNIBe0xb7nlBrrU3WoWgfJYfXve1jxC-5VT5EPLt","y":"sUAxL3L5lJdzFUSR9EHLniuBhEbvXfPa_3OiR6Du0_GOlFXXIi4UmbNpk10_Thfq","crv":"P-384","d":"0f0NnWg__Qgqjj3fl2gAlsID4Ni41FR88cmZPVgb6ch-ZShuVJRjoxymCuzVP7Gi"}`,
			`eyJlbmMiOiJBMTkyQ0JDLUhTMzg0IiwiYWxnIjoiRUNESC1FUyIsImVwayI6eyJrdHkiOiJFQyIsIngiOiJsX3hXdzIyb1NfOWZGbV96amNzYkstd3R3d0RHSlRQLUxnNFVBWDI3WWF1b1YwNml2emwtcm1ra2h6ci11SDBmIiwieSI6IloyYmVnbzBqeE9nY0YtNVp4SFNBOU5jZDVCOW8wUE1pSVlRbm9sWkNQTHA3YndPd1RLUEZaaFZVUlFPSjdoeUciLCJjcnYiOiJQLTM4NCJ9fQ..jSWP7pfa4KcpqKWZ1x8awg.osb-5641Ej1Uon_f3U8bNw.KUQWwb35Gxq3YQ34_AVkebugx4rxq1lO`,
		},
		{
			`{"kty":"EC","x":"AH3rqSYjKue50ThW0qq_qQ76cNtqWrc7hU6kZR6akxy8iTf8ugcpqnbgbi98AgSwIqgJZDBMCk-8eoiGaf3R_kDD","y":"AeafPdJjHLf6pK5V7iyMsL3-6MShpHS6jXQ8m-Bcbp06yxAMn6TJbdkacvj45dy_pdh1s6XZwoxRxNETg_gj-hq9","crv":"P-521","d":"AB2tm9vgGe2BaxZmJQ016GY-U7NV_EWhrPsLDC5l9tAM9DGEwI2cT2HcO20Z6CQndw0ZhqLZ6MEvS8siL-SCxIl2"}`,
			`eyJlbmMiOiJBMjU2Q0JDLUhTNTEyIiwiYWxnIjoiRUNESC1FUyIsImVwayI6eyJrdHkiOiJFQyIsIngiOiJBQ1RLMlVPSjJ6SVk3U1U4T0xkaG1QQmE4ZUVpd2JrX09UMXE0MHBsRlRwQmJKUXg3YWdqWG9LYml2NS1OTXB6eXZySm1rblM3SjNRUWlUeFgwWmtjemhEIiwieSI6IkFXeTZCR1dkZld2ekVNeGIxQklCQnZmRDJ4bEh6Rjk2YzVVRVQ4SFBUS0RSeUJyMnQ4T2dTX1J2MnNoUmxGbXlqUWpyX25uQk94akcxVTZNWDNlZ2VETzciLCJjcnYiOiJQLTUyMSJ9fQ..EWqSGntxbO_Y_6JRjFkCgg.DGjDNjAYdsnYTpUFJi1gEI4YtNd7gBPMjD3CDH047RAwZKTme6Ah_ztzxSfVg5kG.yGm5jn2LtbFXaK_yf0b0932sI2O77j2gwmL1Y09YC_Y`,
		},
	}

	for i, vector := range data {
		var jwk JSONWebKey
		err := jwk.UnmarshalJSON([]byte(vector.key))
		if err != nil {
			t.Fatal(i, err)
		}

		jwe, err := ParseEncrypted(vector.message)
		if err != nil {
			t.Fatal(i, err)
		}

		_, err = jwe.Decrypt(jwk)
		if err != nil {
			t.Fatal(i, err)
		}
	}
}
  • P-256 and P-384 work fine, and all internal values match what Jose4j sees.
  • For P-521 the X/Y/D inputs into the ScalarMult call appear to match the Jose4j inputs.
  • The produced shared secret, Z, returned in Go is different than the one returned in Java.
  • Other than that I found no discrepancies, the KDF inputs are the same.

Here's the X/Y/D values (match in Go/Java):

493305133257357048619197795638588325797400450725893345279566539029118248008085840834931645721517067050551731502235657601881986349729005555067943775116671043
4890184596503430709216186741058449389829849403618932389998733112774613363382780584467706988984774439888319484546345800116804785060151366477174854706485801915
397919061859804402264928471666392054870838857803436415331149736548594401034626325252304299797073957743302040497083676277402027536984091444246917698677934454

Shared secret from scalar mult in Go:

BmPvXAskqcc/ORA1l4UMngzHf1NvRMZL4eZER1Dy9DY+DDlkmW4/L+rWUmXGZdMJ6rflpvEFjHfdsCSUzkxOGHM

Shared secret from scalar mult in Java:

AAZj71wLJKnHPzkQNZeFDJ4Mx39Tb0TGS+HmREdQ8vQ2Pgw5ZJluPy/q1lJlxmXTCeq35abxBYx33bAklM5MThhz

Since this works fine for P-256/P-384, and I don't see any diff in the inputs in the code, the only other reason this isn't working that I can think of is that either Java or Go has a broken P-521 implementation? Seems unlikely. I'll see if I can repro this with another Golang JOSE implementation.

@csstaub
Copy link
Collaborator

csstaub commented May 31, 2019

Looks like the two other popular JOSE libraries I was able to find (lestrrat-go/jwx and dgrijalva/jwt-go) don't seem to support JWE+ECDH-ES, if someone knows of another JOSE library written in Golang please let me know.

@csstaub
Copy link
Collaborator

csstaub commented May 31, 2019

Alright, I figured it out. It was cutting off the first byte of the Z value returned from the scalar base mult giving us a 65-byte array as opposed to a 66-byte one because calling z.Bytes() on a big integer strips the leading zero. This caused input to the the KDF to be incorrect as a result. I'll open a pull request in a sec.

csstaub added a commit that referenced this issue May 31, 2019
Fixes issue #228. After calling ScalarMult for P-521, the output can
sometimes be 65 bytes long instead of 66 bytes. This happens when the
first bit of the computed value is zero. Calling z.Bytes() on the big
integer will then omit the leading zero, giving us a 65-byte value.

This subsequently causes the shared secret computation to be incorrect
as the input into the KDF function should always be 66 bytes which is
the full length for a P-521 coordinate value.
@dodgeware
Copy link
Author

That is great. I appreciate the effort. I have not been able to get back to this so this news is great.

@csstaub csstaub closed this as completed Jun 3, 2019
@cgarvis
Copy link

cgarvis commented Oct 8, 2019

Could we get a release cut for this bug?

csstaub added a commit that referenced this issue Oct 22, 2019
Fixes issue #228. After calling ScalarMult for P-521, the output can
sometimes be 65 bytes long instead of 66 bytes. This happens when the
first bit of the computed value is zero. Calling z.Bytes() on the big
integer will then omit the leading zero, giving us a 65-byte value.

This subsequently causes the shared secret computation to be incorrect
as the input into the KDF function should always be 66 bytes which is
the full length for a P-521 coordinate value.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants