bcgit / bc-java

Bouncy Castle Java Distribution (Mirror)
https://www.bouncycastle.org/java.html
MIT License
2.33k stars 1.14k forks source link

CMS verification fails in Bouncy Castle when signed attributes in the message are not sorted #1484

Open satur9nine opened 1 year ago

satur9nine commented 1 year ago

I have a CMS signed data message that fails to verify with Bouncy Castle 1.76. This particular message is included as an example in the ASC X9 TR-34-2019 standard.

OpenSSL is able to verify this message but Bouncy Castle reports verification failure.

The cause appears to be that although the signed attributes are specified as non-ordered SET they must be processed in the order they appear in the ASN.1 structure to produce the same digest as when they are signed.

RFC 5652 is a bit vague on this but it says in section 5.4:

When the field is present, however, the result is the message
digest of the complete DER encoding of the SignedAttrs value
contained in the signedAttrs field.

I have attached the data and a test demonstrating the issue. I also show how verification can succeed both in openssl 3.0.8 and when the digest is computed manually using the ordered form of the signed attributes.

bc-cms-verify-bug.zip

dghgit commented 1 year ago

Thanks for the report, and thanks for your efforts in putting together the example, however this is the wrong place for the bug report. If you replace:

    if (!csd.verifySignatures(vProv)) {
        throw new SecurityException("BC verification failed");
    }

with:

    Collection<SignerInformation> cs = csd.getSignerInfos().getSigners();
    for (SignerInformation si : cs) {
        if (new MyDLSignerInformation(si).verify(vProv.get(si.getSID())))
        {
            System.err.println("signature not calculated over DER");
        }
    }

where MyDLSignerInformation is defined as:

private static class MyDLSignerInformation
    extends SignerInformation
{
    protected MyDLSignerInformation(SignerInformation baseInfo)
    {
        super(baseInfo);
    }

    public byte[] getEncodedSignedAttributes()
        throws IOException
    {
        return signedAttributeSet.getEncoded(ASN1Encoding.DL);
    }
}

You'll find the signature verifies and the test prints "signature not calculated over DER" This is a shorthand way of dealing with the problem, essentially it does what your additional methods do.

Your interpretation of RFC 5652 section 5.4 is 100% correct though and there is absolutely nothing vague about it. The signature on the sample message is invalid, it is not calculated over DER, but has been calculated using the disordered encoding in the message, with no attention paid to the need to sort the attributes correctly first.

If it would help, I would welcome the opportunity to discuss this with someone in X9 as it appears the test vector is not proving what they hope it does (unless it's actually there to prove the validation is not RFC 5652 compliant which considering the earliest paper about the dangers of not enforcing the structure of high level messages goes back to at least 2006, I would sincerely hope is not the case).

satur9nine commented 1 year ago

Thanks for your response, effectively you are saying the sample provided in X9 TR 34 is not a valid DER because the elements in the SET are not ordered and therefore the bug is in the document, is that correct? I guess openssl is not quite so strictly applying this sorting requirement.

satur9nine commented 1 year ago

I found the particular requirement in X.690-07/2002 Section 9.3

The encodings of the component values of a set value shall appear in an order determined by their tags as specified in 8.6 of ITU-T Rec. X.680 | ISO/IEC 8824-1. Additionally, for the purposes of determining the order in which components are encoded when one or more component is an untagged choice type, each untagged choice type is ordered as though it has a tag equal to that of the smallest tag in that choice type or any untagged choice types nested within.

I guess it's unfortunate overly complex design choice that leaves ample opportunity for incompatible implementations. I have submitted a message to X9 via their website explaining this issue. I suppose the authors of OpenSSL should be similarly contacted since they are out of spec in verifying this signature as is?

peterdettman commented 1 year ago

Just to be clear, it's not an error per se for the SET to be unordered in the document. However the signature generation must be over a DER encoding, which requires sorting amongst other rules. The signer is therefore required to re-encode to DER as necessary.

dghgit commented 1 year ago

Peter's correct about this. That said I'd recommend outputting messages DER sorted - some recipients can't re-encode, but can at least validate the stream as DER, where this limitation applies it will mean the message will still be validated as a valid BER one, while "technically correct" may not be.

Yes, please drop OpenSSL a line. If anyone needs anything further from us to help deal with this, we're happy to help.