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

TC_DA_1_7.py can crash when non-compliant certificates are mirrored from TestNet #26979

Closed
tcarmelveilleux opened this issue May 31, 2023 · 0 comments · Fixed by #26981
Closed
Assignees
Labels
attestation bug Something isn't working tests

Comments

@tcarmelveilleux
Copy link
Contributor

Problem

TC_DA_1_7.py relies on in-repos PAA mirror. Recently when updating the mirror in credentials/development/paa-root-certs (See #26914), a new PAA was added to testnet with a SignatureAlgorithm field non-compliant with RFC5758 sec 3.2:

When the ecdsa-with-SHA224, ecdsa-with-SHA256, ecdsa-with-SHA384, or
ecdsa-with-SHA512 algorithm identifier appears in the algorithm field
as an AlgorithmIdentifier, the encoding MUST omit the parameters
field. That is, the AlgorithmIdentifier SHALL be a SEQUENCE of one
component, the OID ecdsa-with-SHA224, ecdsa-with-SHA256, ecdsa-with-
SHA384, or ecdsa-with-SHA512.

When parsing the certificate (see attached offending_test_cert.der.zip)
, it blows-up with:

MatterTest] 05-31 11:24:01.236 ERROR Exception occurred in test_TC_DA_1_7.
Traceback (most recent call last):
  File "/usr/local/google/home/tennessee/repos/connectedhomeip/.environment/pigweed-venv/lib/python3.10/site-packages/mobly/base_test.py", line 777, in exec_one_test
    test_method()
  File "/usr/local/google/home/tennessee/repos/connectedhomeip/src/python_testing/matter_testing_support.py", line 813, in async_runner
    return asyncio.run(body(*args, **kwargs))
  File "/usr/lib/python3.10/asyncio/runners.py", line 44, in run
    return loop.run_until_complete(main)
  File "/usr/lib/python3.10/asyncio/base_events.py", line 649, in run_until_complete
    return future.result()
  File "/usr/local/google/home/tennessee/repos/connectedhomeip/src/python_testing/TC_DA_1_7.py", line 79, in test_TC_DA_1_7
    pk.append(await self.single_DUT(i, self.matter_test_config.dut_node_ids[i]))
  File "/usr/local/google/home/tennessee/repos/connectedhomeip/src/python_testing/TC_DA_1_7.py", line 89, in single_DUT
    paa_by_skid = load_all_paa(conf.paa_trust_store_path)
  File "/usr/local/google/home/tennessee/repos/connectedhomeip/src/python_testing/TC_DA_1_7.py", line 46, in load_all_paa
    paa_cert = load_der_x509_certificate(paa_der)
  File "/usr/local/google/home/tennessee/repos/connectedhomeip/.environment/pigweed-venv/lib/python3.10/site-packages/cryptography/x509/base.py", line 594, in load_der_x509_certificate
    return rust_x509.load_der_x509_certificate(data)
ValueError: error parsing asn1 value: ParseError { kind: ExtraData, location: ["Certificate::tbs_cert", "TbsCertificate::signature_alg"] }

This is because the ASN.1 has a NULL for parameters, rather than omitted parameters:

<30 0C>
  SEQUENCE {
<06 08>
    OBJECT IDENTIFIER ecdsaWithSHA256 (1 2 840 10045 4 3 2)
      (ANSI X9.62 ECDSA algorithm with SHA256)
<05 00>
    NULL        ################## <----- Offending field
    }
<03 48>
  BIT STRING
    30 45 02 21 00 E0 2B E2 AF AC C3 A8 E6 51 24 78
    A0 74 61 20 C9 E3 E1 40 CF 3D B4 9C 33 B6 42 59
    7E 01 05 35 21 02 20 08 4F E4 B6 E1 42 00 9C 6E
    0E 08 BD E6 4C EA 32 08 C0 7C DD 86 52 7B B7 27
    BE 39 38 8B 81 82 3D
  }

The TC_DA_1_7.py script has to be updated to be more lenient to bad certs in the trust store, and just log on failure.

@tcarmelveilleux tcarmelveilleux added bug Something isn't working attestation tests labels May 31, 2023
@tcarmelveilleux tcarmelveilleux self-assigned this May 31, 2023
tcarmelveilleux pushed a commit to tcarmelveilleux/connectedhomeip that referenced this issue May 31, 2023
- Fix TC_DA_1_7.py not to blow-up on bad PAAs, just log them.
- Fix PAA fetcher to do the same

Fixes project-chip#26979
andy31415 pushed a commit that referenced this issue Jun 1, 2023
* Fix TC_DA_1_7.py PAA parser

- Fix TC_DA_1_7.py not to blow-up on bad PAAs, just log them.
- Fix PAA fetcher to do the same

Fixes #26979

* Update DCL mirror as of May 31, 2023

- Fix NXP DER conversion --> not 100% legal format, but kept
  since only TC_DA_1_7.py library complains
- Updated all from local run.

Commands executed from root of SDK:

```
  pip install click_option_group # somehow missing from requirements
  cd credentials/development
  python ../fetch-paa-certs-from-dcl.py --use-test-net-http
  cd ../production
  python ../fetch-paa-certs-from-dcl.py --use-main-net-http
  git add credentials/
```

Fixes #26424

* Restyled by autopep8

* Update DCL PAAs on May 31, 2023

Commands run from root. Includes temporary NXP fixups

```
cd credentials/development
rm dcld_mirror_*
python ../fetch-paa-certs-from-dcl.py --use-test-net-http
python ../fetch-paa-certs-from-dcl.py --use-main-net-http

openssl x509 -inform pem -in paa-root-certs/dcld_mirror_SERIALNUMBER_63709330400001_CN_NXP_Matter_PAA_O_NXP_Semiconductors_NV_C_NL.pem -outform der -out paa-root-certs/dcld_mirror_SERIALNUMBER_63709330400001_CN_NXP_Matter_PAA_O_NXP_Semiconductors_NV_C_NL.der
openssl x509 -inform pem -in paa-root-certs/dcld_mirror_SERIALNUMBER_63709380400001_CN_NXP_Matter_Test_PAA_O_NXP_Semiconductors_NV_C_NL.pem -outform der -out paa-root-certs/dcld_mirror_SERIALNUMBER_63709380400001_CN_NXP_Matter_Test_PAA_O_NXP_Semiconductors_NV_C_NL.der

cd ../production
rm dcld_mirror_*
python ../fetch-paa-certs-from-dcl.py --use-main-net-http
openssl x509 -inform pem -in paa-root-certs/dcld_mirror_SERIALNUMBER_63709330400001_CN_NXP_Matter_PAA_O_NXP_Semiconductors_NV_C_NL.pem -outform der -out paa-root-certs/dcld_mirror_SERIALNUMBER_63709330400001_CN_NXP_Matter_PAA_O_NXP_Semiconductors_NV_C_NL.der

cd ../..
git add credentials
```

* Add allowlist of skipped PAAs to TC_DA_1_7

* Fix formatting again

* Restyled by autopep8

---------

Co-authored-by: tennessee.carmelveilleux@gmail.com <tennessee@google.com>
Co-authored-by: Restyled.io <commits@restyled.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
attestation bug Something isn't working tests
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant