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

VCEK contains trailerField for RSASSA-PSS-param and (potentially) violates rfc4055 #57

Open
kajinamit opened this issue May 29, 2024 · 13 comments

Comments

@kajinamit
Copy link

kajinamit commented May 29, 2024

Problem statement

It seems the VCEK file obtained by snpguest fetch command is not compliant with RFC. The violation prevents some libraries such as cryptography in python from loading the file.

Steps to reproduce

  1. Download VCEK
$ snpguest fetch vcek pem milan certs attestation_report.bin
  1. Prepare an environment with cryptography
$ virtualenv .venv
$ source .venv/bin/activate
$ pip install cryptography
  1. Run the snippet to load the certificate
$ python
>>> from cryptography import x509
>>> x509.load_pem_x509_certificate(open('certs/vcek.pem', 'rb').read())

Expected result

The certificate can be loaded without any error

Actual result

The certificate can't be loaded. The crypography library contains the following error.

ValueError: error parsing asn1 value: ParseError { kind: EncodedDefault, location: ["Certificate::tbs_cert", "TbsCertificate::signature_alg", "AlgorithmIdentifier::params", "RsaPssParameters::_trailer_field"] }

Environment

  • Ubuntu 22.04
  • snpguest ( hash: 3896114 )
  • Python 3.10.12 + cryptography 42.0.7

Additional information

According to RFC 4055, https://datatracker.ietf.org/doc/html/rfc4055#section-3.1 , it is stated that the trailerField shouldbe omitted by the implementations that perform signature generation.

trailerField

         The trailerField field is an integer.  It provides
         compatibility with IEEE Std 1363a-2004 P1363A.  The value
         MUST be 1, which represents the trailer field with hexadecimal
         value 0xBC.  Other trailer fields, including the trailer field
         composed of HashID concatenated with 0xCC that is specified in
         IEEE Std 1363a, are not supported.  Implementations that
         perform signature generation MUST omit the trailerField field,
         indicating that the default trailer field value was used.
         Implementations that perform signature validation MUST
         recognize both a present trailerField field with value 1 and an
         absent trailerField field.

but the field is present in the certificate downloaded.

    0:d=0  hl=4 l=1357 cons: SEQUENCE
    4:d=1  hl=4 l= 764 cons:  SEQUENCE
    8:d=2  hl=2 l=   3 cons:   cont [ 0 ]
   10:d=3  hl=2 l=   1 prim:    INTEGER           :02
   13:d=2  hl=2 l=   1 prim:   INTEGER           :00
   16:d=2  hl=2 l=  70 cons:   SEQUENCE
   18:d=3  hl=2 l=   9 prim:    OBJECT            :rsassaPss
   29:d=3  hl=2 l=  57 cons:    SEQUENCE
   31:d=4  hl=2 l=  15 cons:     cont [ 0 ]
   33:d=5  hl=2 l=  13 cons:      SEQUENCE
   35:d=6  hl=2 l=   9 prim:       OBJECT            :sha384
   46:d=6  hl=2 l=   0 prim:       NULL
   48:d=4  hl=2 l=  28 cons:     cont [ 1 ]
   50:d=5  hl=2 l=  26 cons:      SEQUENCE
   52:d=6  hl=2 l=   9 prim:       OBJECT            :mgf1
   63:d=6  hl=2 l=  13 cons:       SEQUENCE
   65:d=7  hl=2 l=   9 prim:        OBJECT            :sha384
   76:d=7  hl=2 l=   0 prim:        NULL
   78:d=4  hl=2 l=   3 cons:     cont [ 2 ]
   80:d=5  hl=2 l=   1 prim:      INTEGER           :30
   83:d=4  hl=2 l=   3 cons:     cont [ 3 ]
   85:d=5  hl=2 l=   1 prim:      INTEGER           :01 <============ THIS SHOULD BE OMITTED

Although the description in RFC is quite confusing, I suspect that the trailerField should be omitted in VCEK.

pyca/cryptography#11037 contains the relevant discussion.
(note: Title is incorrect but it's about the same topic)

@kajinamit
Copy link
Author

I've not yet identified at which layer these NULL values are inserted. Is it possible that the source VCEK file contains these ?
Also, I tried DER format but the same problem is reproduced so it's not relevant to the certificate encoding.

@sarutak
Copy link

sarutak commented May 29, 2024

The issue was caused by the certificate which contains NULL for signature algorithm parameter though it should be omitted according to RFC (See https://datatracker.ietf.org/doc/html/rfc5758#section-3.1 ).

Section 3.2 is the correct part isn't it?

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.

@kajinamit
Copy link
Author

@sarutak Oh. That's a very good point.

Re-reading RFC I noticed that we should probably apply the spec for hash function here, instead,

https://datatracker.ietf.org/doc/html/rfc5758#section-2

and this states ;

   When one of these OIDs appears in an AlgorithmIdentifier, all
   implementations MUST accept both NULL and absent parameters as legal
   and equivalent encodings.

This makes me feel that this is a bug in cryptography side... Let me open an issue for the library and here their thoughts...

@kajinamit
Copy link
Author

After further investigation I'm concluding this is a regression in cryptography... I'm closing this issue now and track down the problem in pyca/cryptography#11037 .
Sorry for the noise !

@kajinamit kajinamit changed the title VCEK contains NULL for signature algorithm parameter which violates RFC VCEK contains trailerField for RSASSA-PSS-param and violates RFC8017 May 30, 2024
@kajinamit kajinamit changed the title VCEK contains trailerField for RSASSA-PSS-param and violates RFC8017 VCEK contains trailerField for RSASSA-PSS-param and violates rfc5758 May 30, 2024
@kajinamit kajinamit changed the title VCEK contains trailerField for RSASSA-PSS-param and violates rfc5758 VCEK contains trailerField for RSASSA-PSS-param and violates rfc4055 May 30, 2024
@kajinamit kajinamit reopened this May 30, 2024
@kajinamit
Copy link
Author

Reopening this issue because we have identified the field which is causing the problem (I've corrected the descriptions). Although the description in RFC is not very clear enough, I'm wondering if we can hear some thoughts here.

@kajinamit kajinamit changed the title VCEK contains trailerField for RSASSA-PSS-param and violates rfc4055 VCEK contains trailerField for RSASSA-PSS-param and (potentially) violates rfc4055 May 30, 2024
@tylerfanelli
Copy link
Member

@kajinamit AMD maintains the VCEK certificates in their Key Distribution Server (KDS). snpguest fetch uses the attestation report values and SEV-SNP generation to build the URL string to fetch a specific VCEK.

With that said, it seems that this is an issue with the VCEK certificates maintained on the KDS, rather than the snpguest fetch command itself, correct?

@kajinamit
Copy link
Author

kajinamit commented May 30, 2024

@tylerfanelli We checked the VCEK file downloaded from https://kdsintf.amd.com directly and confirmed the file contains the trailerField field, too. So the issue is not in snpguest(or snphost) but in the content maintained by AMD, I think.

I wonder if anyone knows any good contact to ask AMD to check the VCEK content by their end. I checked a few sites but failed to find the one which looks appropriate.

@tylerfanelli
Copy link
Member

@tylerfanelli We checked the VCEK file downloaded from https://kdsintf.amd.com directly and confirmed the file contains the trailerField field, too. So the issue is not in snpguest(or snphost) but in the content maintained by AMD, I think.

I wonder if anyone knows any good contact to ask AMD to check the VCEK content by their end. I checked a few sites but failed to find the one which looks appropriate.

@larrydewey Would you be able to help here?

@kajinamit
Copy link
Author

Another thing I noticed is that VCEK does not contain the extensions such as Authority Key Identifier and Subject Key Identifier. This is causing problems with libraries (like cryptography) which strictly require compliance with RFC 5280 .

https://datatracker.ietf.org/doc/html/rfc5280#section-4.2.1.1

@kajinamit
Copy link
Author

Another thing I noticed is that VCEK does not contain the extensions such as Authority Key Identifier and Subject Key Identifier. This is causing problems with libraries (like cryptography) which strictly require compliance with RFC 5280 .

https://datatracker.ietf.org/doc/html/rfc5280#section-4.2.1.1

# openssl verify -CAfile certs/ark.pem -untrusted certs/ask.pem -x509_strict certs/vcek.pem
OU = Engineering, C = US, L = Santa Clara, ST = CA, O = Advanced Micro Devices, CN = SEV-VCEK
error 85 at 0 depth lookup: Missing Authority Key Identifier
error certs/vcek.pem: verification failed

@larrydewey
Copy link
Contributor

@kajinamit a patch was just released to the KDS system which should mitigate this issue. Can you try to re-pull the certificate and let me know if it comes back correctly, now?

@kajinamit
Copy link
Author

kajinamit commented Aug 15, 2024

Hi @larrydewey . Thanks for the update ! Sorry for my delayed response. I took some days off.

I confirmed the trailerField field has been removed.

However the latest VCEK still lacks the required extension fields such as Authority Key Identifier and Subject Key Identifier in its X509v3 extensions.
Could you please check if these fields can be added to VCEK ?

$ openssl x509 -in ./vcek.pem -noout -text
Certificate:
    Data:
        ...
        X509v3 extensions:
            1.3.6.1.4.1.3704.1.1:
                ...
            1.3.6.1.4.1.3704.1.2:
                ..Milan-B0
            1.3.6.1.4.1.3704.1.3.1:
                ...
            1.3.6.1.4.1.3704.1.3.2:
                ...
            1.3.6.1.4.1.3704.1.3.4:
                ...
            1.3.6.1.4.1.3704.1.3.5:
                ...
            1.3.6.1.4.1.3704.1.3.6:
                ...
            1.3.6.1.4.1.3704.1.3.7:
                ...
            1.3.6.1.4.1.3704.1.3.3:
                ...
            1.3.6.1.4.1.3704.1.3.8:
                ....
            1.3.6.1.4.1.3704.1.4:
                t-.}..e..=.V..-.[.'X..c.../...N.g..U...N'.e~c#(.d..h...Vi.y.....
    Signature Algorithm: rsassaPss
    ...

I see ask contains these fields.

$ openssl x509 -in ./ask.pem -noout -text
Certificate:
    Data:
        ...
        X509v3 extensions:
            X509v3 Subject Key Identifier:
                3B:C6:6E:18:2A:C3:FD:3D:62:64:48:9B:E3:B7:47:2C:B4:FC:BF:F8
            X509v3 Authority Key Identifier:
                85:AC:1A:D1:43:F7:C8:AC:55:D4:C5:1D:41:48:AB:D5:78:4A:D4:53
            X509v3 Basic Constraints: critical
                CA:TRUE, pathlen:0
            X509v3 Key Usage: critical
                Certificate Sign
            X509v3 CRL Distribution Points:
                Full Name:
                  URI:https://kdsintf.amd.com/vcek/v1/Milan/crl
    Signature Algorithm: rsassaPss
    ...

@kajinamit
Copy link
Author

@larrydewey By any chance could you check my previous comment ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants