Make ECDSA sign/verify format spec-compliant #8236

tcarmelveilleux · 2021-07-09T03:02:49Z

Problem

Fixes Discrepancy in P256 signature format between impl and spec #8209 (Discrepancy in P256 signature format between impl and spec)

Change overview

Use raw <r,s> concatenated point format for input/output
to ECDSA signature primitives, so that format on the wire
matches spec. This will allow CASE to follow spec encoding.
This is also needed for device attestation and CSR response
spec compliance.
Refactors OpenSSL/mbedTLS to use EC primitives rather than ASN.1
TLS wrappers.
Fixed potential crash in SHA256 when nullptr passed-in (zero length buffers
should be non-nullptr, but with size zero)
Update some utilities to add missing bits/pieces.
Updated CHIPCert library to use raw signatures.
Added custom DER encoding/decoding to avoid pulling-in
more code, more heap and more ASN.1 handling dependencies
if a hardware-based TLS accelerator does not support raw
signatures.

Testing

Added necessary unit tests for new code
Ran ninja -C out/host check against latest merge with master with both OpenSSL and mbedTLS on Linux, all tests still pass (after fixes)

- Use raw <r,s> concatenated point format for input/output to ECDSA signature primitives, so that format on the wire matches spec. This will allow CASE to follow spec encoding. - Update some utilities to add missign bits/pieces. - Add associated unit tests Fixes project-chip#8209

- Method is cheaper and more direct than conversions. - Overall code is smaller. - Refactor the message versions to defer to the sign_hash/verify_hash

…format-issue8209

src/crypto/CHIPCryptoPALOpenSSL.cpp

todo · 2021-07-13T15:33:40Z

Need to make this large number (1k+) to catch some signature serialization corner cases

connectedhomeip/src/crypto/tests/CHIPCryptoPALTest.cpp

Lines 794 to 804 in 57e6a60

    
           // TODO: Need to make this large number (1k+) to catch some signature serialization corner cases 
        
           //       but this is too slow on QEMU/embedded, so we need to parametrize. Signing with ECDSA 
        
           //       is non-deterministic by design (since knowledge of the value `k` used allows recovery 
        
           //       of the private key). 
        
           constexpr int kNumSigningIterations = 3; 
        
           for (int i = 0; i < kNumSigningIterations; ++i) 
        
           { 
        
               P256ECDSASignature signature; 
        
               CHIP_ERROR signing_error = keypair.ECDSA_sign_hash(hash, hash_length, signature); 
        
               NL_TEST_ASSERT(inSuite, signing_error == CHIP_NO_ERROR);

This comment was generated by todo based on a `TODO` comment in `57e6a60` in #8236. cc @tcarmelveilleux.

andy31415 · 2021-07-13T18:08:36Z

src/crypto/CHIPCryptoPALOpenSSL.cpp

    {
-        EC_POINT_clear_free(key_point);
-        key_point = nullptr;
+        BN_clear_free(s);


ScopedBigNum and ScopedPoint would be so nice. I think some OpenSSL C++ headers have unique_ptr implementations for this.

andy31415 · 2021-07-14T18:39:02Z

@woody-apple @bzbarsky-apple @jmartinez-silabs @LuDuda?

scripts/tests/test_suites.sh

src/crypto/CHIPCryptoPAL.cpp

src/crypto/CHIPCryptoPAL.h

bzbarsky-apple · 2021-07-14T20:05:35Z

@tcarmelveilleux will deal with my comments in a followup; merging to get other things unblocked, since none of those comments are critical.

@bzbarsky-apple

…ort Span - Fix leftover nits from @bzbarsky-apple's review of project-chip#8236 - In order to add span support cleanly, added Span support to Reader and BufferWriter, and fixed all necessary breakage. Testing done: pass all unit tests and CASE cert tests

tcarmelveilleux · 2021-07-15T02:51:18Z

@tcarmelveilleux will deal with my comments in a followup; merging to get other things unblocked, since none of those comments are critical.

See #8408 for follow-up.

@bzbarsky-apple

) * Fix leftover from #8236 and make BufferReader/Writer support Span - Fix leftover nits from @bzbarsky-apple's review of #8236 - In order to add span support cleanly, added Span support to Reader and BufferWriter, and fixed all necessary breakage. Testing done: pass all unit tests and CASE cert tests * Fix mbedTLS usage of EcdsaAsn1SignatureToRaw * Apply review comments from @bzbarsky-apple * Use some span reassign instead of out_size ref * Restyled by clang-format * Remove unnecessary nullptr checks handled by Span::size() * Improve IntegerToDer test coverage - Assign to output spans - Use new span function for validity checks (`is_span_usable`) - Replace an untested CHIPCert.cpp usage with tested version * Add is_span_usable() tests * Restyled by clang-format * Grammar fix to kick CI * Commit forgotten removal of obsolete ConvertIntegerRawToDER from CHIPCert.h * Apply review comments from @mspang * Fix clang Co-authored-by: Restyled.io <[email protected]>

@andy31415

* Make ECDSA sign/verify format spec-compliant - Use raw <r,s> concatenated point format for input/output to ECDSA signature primitives, so that format on the wire matches spec. This will allow CASE to follow spec encoding. - Update some utilities to add missign bits/pieces. - Add associated unit tests Fixes project-chip#8209 * Fix some comment typos * Make OpenSSL and mbedTLS use raw signatures directly - Method is cheaper and more direct than conversions. - Overall code is smaller. - Refactor the message versions to defer to the sign_hash/verify_hash * Restyled by clang-format * Fix issue with cert conversion * Use CHIPCert module for raw to DER conversion * Ran clang-format * Restyled by clang-format * Fix unit tests * Remove debug logging left by mistake * Restyled by shellharden * Restyled by shfmt * Address review comments from @andy31415 * Fix merge issues * Restyled by clang-format * Fix test script and improve signing unit test * Restyled by shellharden * Reduce signing unit test case numbers for now * Apply review from @pan-apple * Fix test_suites.sh * Please the gods of shell-harden * Please the gods of shell-harden some more Co-authored-by: Restyled.io <[email protected]>

@bzbarsky-apple

…ort Span (project-chip#8408) * Fix leftover from project-chip#8236 and make BufferReader/Writer support Span - Fix leftover nits from @bzbarsky-apple's review of project-chip#8236 - In order to add span support cleanly, added Span support to Reader and BufferWriter, and fixed all necessary breakage. Testing done: pass all unit tests and CASE cert tests * Fix mbedTLS usage of EcdsaAsn1SignatureToRaw * Apply review comments from @bzbarsky-apple * Use some span reassign instead of out_size ref * Restyled by clang-format * Remove unnecessary nullptr checks handled by Span::size() * Improve IntegerToDer test coverage - Assign to output spans - Use new span function for validity checks (`is_span_usable`) - Replace an untested CHIPCert.cpp usage with tested version * Add is_span_usable() tests * Restyled by clang-format * Grammar fix to kick CI * Commit forgotten removal of obsolete ConvertIntegerRawToDER from CHIPCert.h * Apply review comments from @mspang * Fix clang Co-authored-by: Restyled.io <[email protected]>

tcarmelveilleux added the crypto label Jul 9, 2021

boring-cyborg bot added the lib label Jul 9, 2021

Fix some comment typos

1a4c735

restyled-io bot mentioned this pull request Jul 9, 2021

Restyle Make ECDSA sign/verify format spec-compliant #8237

Closed

tcarmelveilleux marked this pull request as draft July 9, 2021 13:46

tcarmelveilleux and others added 2 commits July 9, 2021 12:47

Make OpenSSL and mbedTLS use raw signatures directly

24f5133

- Method is cheaper and more direct than conversions. - Overall code is smaller. - Refactor the message versions to defer to the sign_hash/verify_hash

Restyled by clang-format

a04b6cc

tcarmelveilleux marked this pull request as ready for review July 9, 2021 16:52

pullapprove bot requested review from chrisdecenzo, jelderton, jmartinez-silabs, LuDuda, mspang, pan-apple, saurabhst and woody-apple July 9, 2021 16:52

pullapprove bot added the review - pending label Jul 9, 2021

project-chip deleted a comment from github-actions bot Jul 9, 2021

tcarmelveilleux marked this pull request as draft July 9, 2021 19:20

tcarmelveilleux added 4 commits July 9, 2021 16:19

Fix issue with cert conversion

854de59

Merge remote-tracking branch 'upstream/master' into fixes/crypto-sig-…

f2bcf06

…format-issue8209

Use CHIPCert module for raw to DER conversion

762df27

Ran clang-format

e0ea5e4

tcarmelveilleux marked this pull request as ready for review July 9, 2021 20:32

Restyled by clang-format

5f6d450

restyled-io bot mentioned this pull request Jul 9, 2021

Restyle Make ECDSA sign/verify format spec-compliant #8261

Closed

tcarmelveilleux added 2 commits July 10, 2021 08:32

Merge remote-tracking branch 'upstream/master' into fixes/crypto-sig-…

9194c08

…format-issue8209

Merge remote-tracking branch 'upstream/master' into fixes/crypto-sig-…

8f3f995

…format-issue8209

tcarmelveilleux and others added 2 commits July 13, 2021 10:00

Fix test script and improve signing unit test

0bdb057

Restyled by shellharden

e0ef2b9

restyled-io bot mentioned this pull request Jul 13, 2021

Restyle Make ECDSA sign/verify format spec-compliant #8328

Closed

pan-apple approved these changes Jul 13, 2021

View reviewed changes

src/crypto/CHIPCryptoPALOpenSSL.cpp Outdated Show resolved Hide resolved

Reduce signing unit test case numbers for now

57e6a60

pan-apple requested a review from balducci-apple July 13, 2021 15:33

Apply review from @pan-apple

890b070

andy31415 reviewed Jul 13, 2021

View reviewed changes

andy31415 approved these changes Jul 13, 2021

View reviewed changes

Fix test_suites.sh

bff8c11

restyled-io bot mentioned this pull request Jul 13, 2021

Restyle Make ECDSA sign/verify format spec-compliant #8344

Closed

tcarmelveilleux added 2 commits July 13, 2021 17:03

Please the gods of shell-harden

66afb86

Please the gods of shell-harden some more

af85572

saurabhst approved these changes Jul 14, 2021

View reviewed changes

jpk233 approved these changes Jul 14, 2021

View reviewed changes

balducci-apple approved these changes Jul 14, 2021

View reviewed changes

bzbarsky-apple approved these changes Jul 14, 2021

View reviewed changes

pullapprove bot added review - approved and removed review - pending labels Jul 14, 2021

bzbarsky-apple merged commit 51a49a9 into project-chip:master Jul 14, 2021

todo bot mentioned this pull request Jul 14, 2021

Need to make this large number (1k+) to catch some signature serialization corne... #8394

Closed

tcarmelveilleux mentioned this pull request Jul 15, 2021

Fix leftover from #8236 and make BufferReader/Writer support Span #8408

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make ECDSA sign/verify format spec-compliant #8236

Make ECDSA sign/verify format spec-compliant #8236

tcarmelveilleux commented Jul 9, 2021 •

edited

Loading

todo bot commented Jul 13, 2021

andy31415 Jul 13, 2021

andy31415 commented Jul 14, 2021

bzbarsky-apple commented Jul 14, 2021

tcarmelveilleux commented Jul 15, 2021

Make ECDSA sign/verify format spec-compliant #8236

Make ECDSA sign/verify format spec-compliant #8236

Conversation

tcarmelveilleux commented Jul 9, 2021 • edited Loading

Problem

Change overview

Testing

todo bot commented Jul 13, 2021

Need to make this large number (1k+) to catch some signature serialization corner cases

This comment was generated by todo based on a TODO comment in 57e6a60 in #8236. cc @tcarmelveilleux.

andy31415 Jul 13, 2021

Choose a reason for hiding this comment

andy31415 commented Jul 14, 2021

bzbarsky-apple commented Jul 14, 2021

tcarmelveilleux commented Jul 15, 2021

tcarmelveilleux commented Jul 9, 2021 •

edited

Loading

This comment was generated by todo based on a `TODO` comment in `57e6a60` in #8236. cc @tcarmelveilleux.