fishsoupisgood commented 3 years ago

TL;DR as of Tuesday Oct 13 Microsoft's wintrust.dll is being very strict in its parsing of DER encoded PKCS7 data and is rejecting signatures made by openssl.

The root cause is that, in the DER encoding, the SET OF certificates in openssl generated PKCS7 signed-data isn't correctly sorted.

RFC2315 §9.1 says:

SignedData ::= SEQUENCE { [...] [0] IMPLICIT ExtendedCertificatesAndCertificates OPTIONAL,

where (§6.6)

ExtendedCertificatesAndCertificates ::= SET OF ExtendedCertificateOrCertificate

ITU X.690 §11.6 states:

11.6 Set-of components The encodings of the component values of a set-of value shall appear in ascending order, the encodings being compared as octet strings with the shorter components being padded at their trailing end with 0-octets.

Yet the attached test program generates the following asn.1

    0:d=0  hl=4 l=1709 cons: SEQUENCE
    4:d=1  hl=2 l=   9 prim:  OBJECT            :pkcs7-signedData
[...]  
   45:d=3  hl=4 l=1340 cons:    cont [ 0 ]
   49:d=4  hl=4 l= 666 cons:     SEQUENCE
   53:d=5  hl=4 l= 386 cons:      SEQUENCE
   57:d=6  hl=2 l=   1 prim:       INTEGER           :00
[...]
   79:d=8  hl=2 l=  15 cons:         SEQUENCE
   81:d=9  hl=2 l=   3 prim:          OBJECT            :commonName
   86:d=9  hl=2 l=   8 prim:          UTF8STRING        :Zebraaaa
[...]
  719:d=4  hl=4 l= 666 cons:     SEQUENCE
  723:d=5  hl=4 l= 386 cons:      SEQUENCE
  727:d=6  hl=2 l=   1 prim:       INTEGER           :00
[...]
  749:d=8  hl=2 l=  15 cons:         SEQUENCE
  751:d=9  hl=2 l=   3 prim:          OBJECT            :commonName
  756:d=9  hl=2 l=   8 prim:          UTF8STRING        :Aardvark

which violates the X.690 §11.6 requirement.

test.c.txt asn1.txt

fishsoupisgood commented 3 years ago

13143 fixes this.

kaduk commented 2 years ago

From reading the history of #13143 it seems that the mechancial change to use ASN1_IMP_SET_OF instead of ASN1_IMP_SEQUENCE_OF is straightforward and trivial, and that the actual framing of the encoded bits is the same, so that an encoded SET OF can be parsed as SEQUENCE of with no issue. The reverse direction (taking something encoded as SEQUENCE OF and parsing it as SET OF) will only sometimes succeed, since the DER rules for SET require the elements to be sorted by tag order, and a sequence will not necessarily have that property. The history of the PR suggests that the simple change caused some unit tests to fail, which may be due to their reliance on the order of entries being preserved with a round trip through the en/decoder, or it may be due to old existing files created using the sequence encoding failing to parse as sets (I don't remember if our parser is strict about checking the sorted nature). However, the actual CI jobs are not visible anymore so I don't have any direct insight there. In either case, more code than just the trivial structure definition replacement will be needed to produce a workable solution.

davidben commented 2 years ago

so that an encoded SET OF can be parsed as SEQUENCE of with no issue

Yup. This isn't true in general, but it's true here because this particular SET OF type is implicitly-tagged. That meant the underlying type's tag never appeared in the encoding, but was still relevant for other encoding rules.

or it may be due to old existing files created using the sequence encoding failing to parse as sets (I don't remember if our parser is strict about checking the sorted nature)

I don't believe OpenSSL's parser enforces this. It's pretty lax and doesn't enforce most things, especially DER-only checks.

nhorman commented 2 weeks ago

Marking as inactive, to be closed at the end of 3.4 dev barring further input.

openssl / openssl

Incorrect DER encoding in PKCS7 signed data objects #13142

13143 fixes this.