search

eclipse / tinydtls

Eclipse tinydtls
https://projects.eclipse.org/projects/iot.tinydtls
Other
105 stars 58 forks source link

sha256-ecdsa signature encoding #56

Closed boaks closed 3 years ago

boaks commented 3 years ago

branch: development commit: 290c48d262b6 with PR #52 and PR #53 cherry picked

(I assume, this is not related to this exact version.)

Running Eclipse Californium Interoperability Tests continuously, I faced rare failures using RPK.

Starting with one of the reported failure:

Mar 23 01:17:48 DEBG got packet 22 (98 bytes)
Mar 23 01:17:48 DEBG new packet arrived with seq_nr: 4
Mar 23 01:17:48 DEBG new bitfield is               : b
Mar 23 01:17:48 DEBG receive header: (13 bytes):
00000000 16 FE FD 00 00 00 00 00  00 00 04 00 55 
Mar 23 01:17:48 DEBG receive unencrypted: (85 bytes):
00000000 0F 00 00 49 00 04 00 00  00 00 00 49 04 03 00 45 
00000010 30 43 02 1F 4C E9 93 20  8C B2 18 B2 33 C2 CF 99 
00000020 C8 C1 AA 16 FA FA 86 8C  9C 23 3D DA 4C 8D 70 B7 
00000030 16 ED 5D 02 20 67 0F B5  2A 8B 58 BE 31 96 D5 D6 
00000040 BB 19 02 EA 52 9C 5B 4D  F1 47 72 28 8E F1 EC BC 
00000050 96 2F FF A2 DC 
Mar 23 01:17:48 DEBG received handshake packet of type: certificate_verify (15)
Mar 23 01:17:48 DEBG handle handshake packet of type: certificate_verify (15)
Mar 23 01:17:48 ALRT the packet length does not match the expected
Mar 23 01:17:48 WARN error in check_client_certificate_verify err: -562

I recognized, that tinydtls seems to assume, that the SHA256-ECDSA signature has a fixed length (86 bytes). According Shouldn't a signature using ECDSA be exactly 96 bytes, not 102 or 103? the length of such a signature may be encoded in a variable way.

I got additionally faced similar failures:

Mar 21 11:22:28 DEBG got packet 22 (99 bytes)
Mar 21 11:22:28 DEBG new packet arrived with seq_nr: 4
Mar 21 11:22:28 DEBG new bitfield is               : b
Mar 21 11:22:28 DEBG receive header: (13 bytes):
00000000 16 FE FD 00 00 00 00 00  00 00 04 00 56 
Mar 21 11:22:28 DEBG receive unencrypted: (86 bytes):
00000000 0F 00 00 4A 00 04 00 00  00 00 00 4A 04 03 00 46 
00000010 30 44 02 1F 22 1A F7 1E  EE 84 DB 50 51 4A DC C9 
00000020 16 67 A9 16 56 CC B0 79  73 A2 80 7B 6C C4 74 44 
00000030 A1 C3 D7 02 21 00 99 AC  0A 91 80 09 6B 8A D2 DA 
00000040 32 7A E0 C0 2F 1D 9F 92  4C EA 2B BE 89 9F D7 09 
00000050 3F E6 AE 7C 91 37 
Mar 21 11:22:28 DEBG received handshake packet of type: certificate_verify (15)
Mar 21 11:22:28 DEBG handle handshake packet of type: certificate_verify (15)
Mar 21 11:22:28 ALRT wrong signature err: -1
Mar 21 11:22:28 WARN error in check_client_certificate_verify err: -552

where the total length is 86, but the keys have different lengths. Generally, zero-bytes seems to be removed, if the next byte has less than 0x7f. Otherwise one 0x00 is kept for values 0x80 and larger.

mrdeep1 commented 3 years ago

The case where there is a leading 0x00 to make the integer non-negative is handled correctly. However, if the length is < 0x20 because there are a number of leading 0 bits (at least 8), the resultant EC key includes the preceding length byte (and possibly more depending on the number of leading 0 bits) and hence signature failure in example 2. Looking at how best to fix this decoding issue.

mrdeep1 commented 3 years ago

@boaks Can you checkout PR #58 to check whether this fixes all the issues you are seeing?

boaks commented 3 years ago

Yes, sure. It will take some time. With the current tests, the failures are rare. I plan to prepare a special test for that, but not in the next days.

mrdeep1 commented 3 years ago

I could not think of an easy way to generate keys that exhibit the shorter length (e.g. have > 8 leading 0 bits), so code changes are by inspection and have confirmed all works with "proper" keys. It is possible that other areas of the code may have the same issue of SHA256 encoding length handling.

boaks commented 3 years ago

Yes, but I can exclude other tests, and repeat the specific tests more times. For now, I just put your patch on my CT system.

boaks commented 3 years ago

Without PR, my tests fails randomly within 100-400 runs. With the PR, none of 5000 test runs has failed.

mrdeep1 commented 3 years ago

That is good news. I have recently pushed a code change to move code duplication into separate function with a few more checks added, but functionally there should be no changes.

boaks commented 3 years ago

I will retest that tomorrow.

boaks commented 3 years ago

With commit dceae2072b744db0748f1ea3bced4d9390511be0

Mar 24 07:10:25 DEBG send handshake packet of type: server_key_exchange (12)
Mar 24 07:10:25 DEBG send header: (13 bytes):
00000000 16 FE FD 00 00 00 00 00  00 00 03 00 9A 
Mar 24 07:10:25 DEBG send unencrypted: (12 bytes):
00000000 0C 00 00 8E 00 03 00 00  00 00 00 8E 
Mar 24 07:10:25 DEBG send unencrypted: (142 bytes):
00000000 03 00 17 41 04 24 96 6A  B2 A6 23 56 26 9C DC 8F 
00000010 A8 69 EF A5 42 BB F0 71  F3 F6 60 D8 E6 39 65 06 
00000020 14 63 3D 04 6F 6B B6 0A  22 2F DC 5F D7 94 8A 17 
00000030 0D 2E A9 89 2A 2B DB 2A  A7 53 7C 25 B3 B2 0D C2 
00000040 15 61 BE 5B D9 04 03 00  45 30 43 02 1F 00 79 07 
00000050 4E 2C 90 ED C0 CE 79 0B  84 C7 AC 7A D3 66 9B EF 
00000060 C5 76 B7 5F 64 A7 30 D0  04 D8 44 9C 02 20 7B 9D 
00000070 53 69 20 68 D4 20 37 4A  BC D2 AD F7 3C 8D E2 18 
00000080 8D BF BB D6 EA 51 7D 28  AC 6D 99 AA D1 30 
Mar 24 07:10:25.869 DEBG *  [::ffff:127.0.0.1]:5684 <-> [::ffff:127.0.0.1]:56350 (if1) DTLS: sent 167 bytes
Mar 24 07:10:25 DEBG add MAC data: (12 bytes): 0D0000080004000000000008
Mar 24 07:10:25 DEBG add MAC data: (8 bytes): 0140000204030000
Mar 24 07:10:25 DEBG send handshake packet of type: certificate_request (13)
Mar 24 07:10:25 DEBG send header: (13 bytes):
00000000 16 FE FD 00 00 00 00 00  00 00 04 00 14 
Mar 24 07:10:25 DEBG send unencrypted: (12 bytes):
00000000 0D 00 00 08 00 04 00 00  00 00 00 08 
Mar 24 07:10:25 DEBG send unencrypted: (8 bytes):
00000000 01 40 00 02 04 03 00 00  
Mar 24 07:10:25.869 DEBG *  [::ffff:127.0.0.1]:5684 <-> [::ffff:127.0.0.1]:56350 (if1) DTLS: sent 33 bytes
Mar 24 07:10:25 DEBG add MAC data: (12 bytes): 0E0000000005000000000000
Mar 24 07:10:25 DEBG send handshake packet of type: server_hello_done (14)
Mar 24 07:10:25 DEBG send header: (13 bytes):
00000000 16 FE FD 00 00 00 00 00  00 00 05 00 0C 
Mar 24 07:10:25 DEBG send unencrypted: (12 bytes):
00000000 0E 00 00 00 00 05 00 00  00 00 00 00 
Mar 24 07:10:25.869 DEBG *  [::ffff:127.0.0.1]:5684 <-> [::ffff:127.0.0.1]:56350 (if1) DTLS: sent 25 bytes
Mar 24 07:10:25.869 DEBG ***new session 0x55961580e870
Mar 24 07:10:25.869 DEBG ** DTLS global timeout set to 1928ms
07:10:25.879 ERROR [EcdhEcdsaServerKeyExchange]: Could not verify the server's signature. (o.e.c.s.d.EcdhEcdsaServerKeyExchange.verifySignature:211)
java.security.SignatureException: Invalid encoding for signature
    at java.base/sun.security.util.ECUtil.decodeSignature(ECUtil.java:293)
    at jdk.crypto.ec/sun.security.ec.ECDSASignature.engineVerify(ECDSASignature.java:498)
    at java.base/java.security.Signature$Delegate.engineVerify(Signature.java:1416)
    at java.base/java.security.Signature.verify(Signature.java:790)
    at org.eclipse.californium.scandium.dtls.EcdhEcdsaServerKeyExchange.verifySignature(EcdhEcdsaServerKeyExchange.java:208)
    at org.eclipse.californium.scandium.dtls.ClientHandshaker.receivedServerKeyExchange(ClientHandshaker.java:501)
    at org.eclipse.californium.scandium.dtls.ClientHandshaker.doProcessMessage(ClientHandshaker.java:264)
    at org.eclipse.californium.scandium.dtls.Handshaker.processNextHandshakeMessages(Handshaker.java:862)
    at org.eclipse.californium.scandium.dtls.Handshaker.processNextMessages(Handshaker.java:718)
    at org.eclipse.californium.scandium.dtls.Handshaker.processMessage(Handshaker.java:676)
    at org.eclipse.californium.scandium.DTLSConnector.processHandshakeRecord(DTLSConnector.java:1850)
    at org.eclipse.californium.scandium.DTLSConnector.processRecord(DTLSConnector.java:1602)
    at org.eclipse.californium.scandium.DTLSConnector$14.run(DTLSConnector.java:1459)
    at org.eclipse.californium.elements.util.SerialExecutor$1.run(SerialExecutor.java:290)
    at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
    at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
    at java.base/java.lang.Thread.run(Thread.java:834)
Caused by: java.io.IOException: Invalid encoding: redundant leading 0s
    at java.base/sun.security.util.DerInputBuffer.getBigInteger(DerInputBuffer.java:161)
    at java.base/sun.security.util.DerValue.getPositiveBigInteger(DerValue.java:558)
    at java.base/sun.security.util.ECUtil.decodeSignature(ECUtil.java:277)
    ... 16 common frames omitted

I'm not sure, if this is really introduced by the latest changes or was just not detected.

02 1F 00 79 07

I guess, the leading 00 before 79 is at least not, what java expects.

boaks commented 3 years ago

Using the libcoap-tinydtls client, I get similar errors for the server_key_exchange (I retested the privous commit of the PR, not the latest).

Mar 24 07:44:25 DEBG receive header: (13 bytes):
00000000 16 FE FD 00 00 00 00 00  00 00 03 00 9A 
Mar 24 07:44:25 DEBG receive unencrypted: (154 bytes):
00000000 0C 00 00 8E 00 03 00 00  00 00 00 8E 03 00 17 41 
00000010 04 FD AF E7 A2 E0 69 71  6A 29 E7 0B 7F FF D9 04 
00000020 B2 E7 59 2D 91 37 25 E8  0A 24 55 3F FC C2 07 15 
00000030 99 75 AD 6A 9A 9A 9E ED  BA C9 E0 F4 A8 61 C3 29 
00000040 17 92 5F 39 1C 49 CA 5E  FD A7 A8 0D CE 24 99 2F 
00000050 57 04 03 00 45 30 43 02  20 29 D1 B6 AE 84 22 ED 
00000060 60 57 45 59 98 56 AB 90  C7 6F 61 81 C7 C9 8D 23 
00000070 B8 BB C2 62 E2 92 C1 AC  AE 02 1F 2A 16 89 59 EF 
00000080 C9 E8 66 05 9E 3D B2 32  54 33 A8 82 F3 61 B8 1A 
00000090 56 4B 75 4C 8F E8 D9 B2  EA F6 
Mar 24 07:44:25 DEBG received handshake packet of type: server_key_exchange (12)
Mar 24 07:44:25 DEBG handle handshake packet of type: server_key_exchange (12)
Mar 24 07:44:25 DEBG add MAC data: (154 bytes): 0C00008E000300000000008E0300174104FDAFE7A2E069716A29E70B7FFFD904B2E7592D913725E80A24553FFCC207159975AD6A9A9A9EEDBAC9E0F4A861C32917925F391C49CA5EFDA7A80DCE24992F57040300453043022029D1B6AE8422ED605745599856AB90C76F6181C7C98D23B8BBC262E292C1ACAE021F2A168959EFC9E866059E3DB2325433A882F361B81A564B754C8FE8D9B2EAF6
Mar 24 07:44:25 ALRT the packet length does not match the expected
Mar 24 07:44:25 WARN error in check_server_key_exchange err: -562
Mar 24 07:44:25 WARN error while handling handshake packet

The second INTEGER is 02 1F.

(The libcoap-tinydtls server of that previous PR commit, has passed 5000 test again without errors.)

boaks commented 3 years ago

OK, a second 5000 run with the previous commit stopped at 30 with:

Mar 24 07:51:17 DEBG send handshake packet of type: server_key_exchange (12)
Mar 24 07:51:17 DEBG send header: (13 bytes):
00000000 16 FE FD 00 00 00 00 00  00 00 03 00 9B 
Mar 24 07:51:17 DEBG send unencrypted: (12 bytes):
00000000 0C 00 00 8F 00 03 00 00  00 00 00 8F 
Mar 24 07:51:17 DEBG send unencrypted: (143 bytes):
00000000 03 00 17 41 04 8F F9 B5  F4 4D 82 33 C9 D7 53 85 
00000010 6A C5 B8 AF 91 4B D0 B1  FF 12 BB F4 C0 E0 99 30 
00000020 16 89 13 56 04 75 C8 49  85 36 E4 2E 51 5A ED CF 
00000030 56 B9 D0 28 6B 21 B3 A5  01 32 71 CC EA 84 EA 84 
00000040 E8 12 1E 05 EC 04 03 00  46 30 44 02 1F 00 62 D9 
00000050 DC DC 7E 32 92 AA 84 49  DD 8D EE E9 8C AB 8D 28 
00000060 38 81 64 14 11 78 D3 05  01 F8 65 0C 02 21 00 BC 
00000070 6D 19 C2 C1 56 F0 14 51  67 A1 9D 4C 4D DB 03 11 
00000080 1F F2 1D FF 8C 05 DA E9  CD 13 6E 21 74 49 AF

A 02 1F 00 62 with the 00 in front of 62.

boaks commented 3 years ago

With the latest failure, I consider the issue affects, encoding and decoding of signatures, used in the server_key_exchange and certificate_verify messages. So, should I split this issue? Basically it's about "signature" so one may be also OK.

mrdeep1 commented 3 years ago

Yes, the encoding of an integer is broken. No need to split the issue.

mrdeep1 commented 3 years ago

Updated #58 to correct the encoding of the integer.

boaks commented 3 years ago

The latest commit works (ate least until now with 20000 test runs).

boaks commented 3 years ago

Fixed with PR #58.