ASN1ObjectIdentifier.createPrimitive fails to instantiate a CMSSignedData starting from 1.78

[bsanchezb]

Hello,

We have a problem with validation of some signatures and timestamps after the commit 3790993, in particular after introduction of the !ASN1RelativeOID.isValidContents condition in the ASN1ObjectIdentifier.createPrimitive method:

    static ASN1ObjectIdentifier createPrimitive(byte[] contents, boolean clone)
    {
        checkContentsLength(contents.length);
        
        final OidHandle hdl = new OidHandle(contents);
        ASN1ObjectIdentifier oid = pool.get(hdl);
        if (oid != null)
        {
            return oid;
        }

        if (!ASN1RelativeOID.isValidContents(contents))
        {
            throw new IllegalArgumentException("invalid OID contents");
        }

        return new ASN1ObjectIdentifier(clone ? Arrays.clone(contents) : contents, null);
    }

The problem is BouncyCastle not being able to build a CMSSignedData when having a signature or a timestamp with an invalid ASN1ObjectIdentifier's content.

This is critical for us, as it blocks validation of some legacy signatures, as well as failure on new CMSSignedData(InputStream) call makes it impossible to provide any relevant data about nature of the issue to the end-user:

Caused by: org.bouncycastle.cms.CMSException: IOException reading content.
	at org.bouncycastle.cms.CMSUtils.readContentInfo(Unknown Source)
	at org.bouncycastle.cms.CMSUtils.readContentInfo(Unknown Source)
	at org.bouncycastle.cms.CMSSignedData.<init>(Unknown Source)
	... 61 more
Caused by: org.bouncycastle.asn1.ASN1Exception: invalid OID contents
	at org.bouncycastle.asn1.ASN1InputStream.createPrimitiveDERObject(Unknown Source)
	at org.bouncycastle.asn1.ASN1StreamParser.parseImplicitPrimitive(Unknown Source)
	at org.bouncycastle.asn1.ASN1StreamParser.implParseObject(Unknown Source)
	at org.bouncycastle.asn1.ASN1StreamParser.readVector(Unknown Source)
	at org.bouncycastle.asn1.DLSequenceParser.getLoadedObject(Unknown Source)
	at org.bouncycastle.asn1.ASN1StreamParser.readVector(Unknown Source)
	at org.bouncycastle.asn1.DLSetParser.getLoadedObject(Unknown Source)
	at org.bouncycastle.asn1.ASN1StreamParser.readVector(Unknown Source)
	at org.bouncycastle.asn1.DLSequenceParser.getLoadedObject(Unknown Source)
	at org.bouncycastle.asn1.ASN1StreamParser.readVector(Unknown Source)
	at org.bouncycastle.asn1.ASN1StreamParser.loadTaggedDL(Unknown Source)
	at org.bouncycastle.asn1.DLTaggedObjectParser.getLoadedObject(Unknown Source)
	at org.bouncycastle.asn1.ASN1StreamParser.readVector(Unknown Source)
	at org.bouncycastle.asn1.DLSequenceParser.getLoadedObject(Unknown Source)
	at org.bouncycastle.asn1.ASN1StreamParser.readVector(Unknown Source)
	at org.bouncycastle.asn1.DLSetParser.getLoadedObject(Unknown Source)
	at org.bouncycastle.asn1.ASN1StreamParser.readVector(Unknown Source)
	at org.bouncycastle.asn1.BERSequenceParser.parse(Unknown Source)
	at org.bouncycastle.asn1.BERSequenceParser.getLoadedObject(Unknown Source)
	at org.bouncycastle.asn1.ASN1StreamParser.readVector(Unknown Source)
	at org.bouncycastle.asn1.ASN1StreamParser.loadTaggedIL(Unknown Source)
	at org.bouncycastle.asn1.BERTaggedObjectParser.getLoadedObject(Unknown Source)
	at org.bouncycastle.asn1.ASN1StreamParser.readVector(Unknown Source)
	at org.bouncycastle.asn1.BERSequenceParser.parse(Unknown Source)
	at org.bouncycastle.asn1.ASN1InputStream.readObject(Unknown Source)
	... 65 more
Caused by: java.lang.IllegalArgumentException: invalid OID contents
	at org.bouncycastle.asn1.ASN1ObjectIdentifier.createPrimitive(Unknown Source)
	... 90 more

We would prefer to have an exception on extraction of particular data, but failure on CMSSignedData instantiation is a breaking change for us.

Thank you.

[bsanchezb]

Test file:
bc1639test.zip

Unit test:

        File signatureFile = new File("bc1639test.p7m");

        try (InputStream is = Files.newInputStream(signatureFile.toPath())) {
             CMSSignedData cmsSignedData = new CMSSignedData(is);
        }

[dghgit]

Oh dear. I can't say I've ever heard of someone messing up the encoding of an OID, but I guess there's always a first time. So are you asking for a different exception or that the check be disabled? As far as specific exceptions go, the ASN1Exception is really pointing out the data stream is invalid - I'm not sure we can be more specific than that.

[bsanchezb]

In fact we found quite a few test cases with wrongly encoded OIDs. Sometimes in signatures, sometimes in timestamps and (a lot) in certificate policies certificate extension (the easiest to handle).
I believe the fact that issue was not commonly known before is that the processing was silent and no exception has been thrown, so it was never reported.

While I understand, that it may be a wrong encoding, we need to support signatures and timestamps having the issue. Failing to build a CMSSignedData, thus blocking any signature processing is not an option for us, unfortunately.
I would prefer to avoid exception in ASN1ObjectIdentifier.createPrimitive method, or at least make it somehow optional.

[peterdettman]

@bsanchezb For existing data, couldn't you re-encode it using old BC? I assume you are worried that would break a signature/timestamp but have you checked that assumption?

[bsanchezb]

@peterdettman , this option is not viable, unfortunately. For the context, we develop a EU signature creation and validation library DSS, which can be freely used by any party. The demo is available at: https://ec.europa.eu/digital-building-blocks/DSS/webapp-demo/. In most of the cases, developers may not know in advance what signatures will be provided to the validation, thus re-encoding of user's signatures with an old version of BC is not possible in that context.

[dghgit]

I think the only way we can deal with this is to introduce a system property, which isn't great... I'll look into it.

[dghgit]

So the problem OID is here:

Sequence
      ObjectIdentifier(1.2.840.113549.1.9.16.2.4)
      Set
        Sequence
              UTF8String(please ignore this attribute) 
              ObjectIdentifier()

This has to be deliberate... the level of stupidity it would require to do it by accident beggars belief. I've extended the property I added for #1758 to cover this case as well.