Skip to content

ASN.1 structure parsers accept duplicate, out-of-order and unknown context tags in SEQUENCE with OPTIONAL fields #2516

Open

Description

@xipki

Summary

Many getInstance() / constructor implementations for ASN.1 structures parse
SEQUENCE elements by looping over all elements and switching on the tag number.
This does not enforce the ASN.1 definition and accepts encodings that should be rejected.

Example

Definition:

A ::= SEQUENCE {
  x [0] OCTET STRING OPTIONAL,
  y [1] OCTET STRING OPTIONAL
}

The following encoding is accepted, although [0] may appear at most once:

SEQUENCE {
  [0] OCTET STRING
  [0] OCTET STRING
}

Similarly accepted:

  • out-of-order elements, e.g. [1] before [0]
  • unexpected tag numbers, e.g. [5]

Expected behaviour

Elements should be parsed positionally: each OPTIONAL field is consumed at most once,
in definition order, with the tag class checked (e.g. via hasContextTag(n)), and any
remaining element should cause an IllegalArgumentException.

Why it matters

Lenient parsing allows multiple encodings of the same logical value and can create
parser differentials with other ASN.1 implementations (e.g. "first wins" vs "last wins"
on duplicates). In security-relevant structures this class of issue has previously led
to vulnerabilities (e.g. duplicate X.509 extensions, BERserk / CVE-2014-1568).

Suggested fix

Replace the enumerate-and-switch pattern with index-based sequential parsing, e.g.:

int i = 0, n = seq.size();
if (i < n && tagged(seq, i).hasContextTag(0)) {
    x = ...;
    i++; 
}
   
if (i < n && tagged(seq, i).hasContextTag(1)) {
    y = ...;
    i++;
}

if (i != n) {
    throw new IllegalArgumentException("unexpected element at index " + i);
}

Activity

  1. self-assigned this
    on Oct 20, 2026
  2. xipki commented on Oct 22, 2026

    @xipki
    ContributorAuthor

    I have reviewed the current SignerInfo code (after the latest commits), and it still does not check the tag numbers of the context-specific elements.

    RFC 5652 defines SignerInfo as:

    SignerInfo ::= SEQUENCE {
        version             CMSVersion,
        sid                 SignerIdentifier,
        digestAlgorithm     DigestAlgorithmIdentifier,
        signedAttrs         [0] IMPLICIT SignedAttributes OPTIONAL,
        signatureAlgorithm  SignatureAlgorithmIdentifier,
        signature           SignatureValue,
        unsignedAttrs       [1] IMPLICIT UnsignedAttributes OPTIONAL }
    

    The current code identifies these optional fields only by position, not also by tag. It treats any tagged element after digestAlgorithm as signedAttrs, and any tagged element after signature as unsignedAttrs. The tag numbers [0] and [1] are never checked.

    Recommendation

    When decoding SignerInfo, the parser should check the tag numbers strictly:

    The optional element after digestAlgorithm must be [0].
    The optional element after signature must be [1].
    Any other tag, or any extra trailing element, should be rejected as an encoding error.

  3. dghgit commented on Oct 25, 2026

    @dghgit
    Contributor

    Thanks for the report, would you check this again? I think we may have "crossed commits" and you received an old file. It should be correct now. Let me know how you go. Thanks.

  4. xipki commented on Oct 28, 2026

    @xipki
    ContributorAuthor

    4. Thanks for the report, would you check this again? I think we may have "crossed commits" and you received an old file. It should be correct now. Let me know how you go. Thanks.

    You are right. Both SignerInfo classes are fine.

  5. xipki commented on Oct 28, 2026

    @xipki
    ContributorAuthor

    Repro2516.java

    I went through the parsers not touched by the #2516 commits. The ones below still accept malformed encodings. The attached reproducer (Repro2516.java) first parses a well-formed encoding of each class, which must be accepted, and then the malformed variants. Against both 1.86 and d401e82d3f it reports 53 malformed encodings accepted.

    Security-relevant

    • x509.AttributeCertificateInfo
      • Two extensions blocks are accepted and the last one wins, so the duplicate-extension check is bypassed.
      • extensions before issuerUniqueID is accepted.
      • Unknown trailing elements are ignored.
    • sec.ECPrivateKey
      • Fields are looked up lazily by tag, so a duplicate [0] is accepted and the first one wins.
      • [1] before [0], an unknown [5] and trailing elements are all accepted.
      • The deprecated ECPrivateKeyStructure behaves the same way.
    • x509.OtherName: neither the tag number nor the tag class of value [0] EXPLICIT is checked, so [5] and APPLICATION 0 are both accepted.
    • pkcs.CertificationRequestInfo: the tag number of attributes [0] is not checked, and trailing elements are ignored.
    • x509.BasicConstraints: trailing elements are ignored.

    PKCS#12 / PKCS#7

    • pkcs.ContentInfo, SafeBag, CertBag, CRLBag, SecretBag
      • The [0] tag number is not checked. SafeBag also skips the tag-class check.
      • All except SafeBag ignore trailing elements.
    • pkcs.EncryptedData: trailing elements are ignored.

    Others

    • Tag number not checked: cms.ecc.ECCCMSSharedInfo, cms.ecc.MQVuserKeyingMaterial, cryptopro.Gost2814789EncryptedKey, cryptopro.GostR3410TransportParameters, cryptopro.GostR3410KeyTransport, x509.IetfAttrSyntax.
      • Of these, ECCCMSSharedInfo, GostR3410KeyTransport and IetfAttrSyntax also ignore trailing elements.
    • cryptopro.GOST28147Parameters: trailing elements are ignored.
    • Duplicate and out-of-order fields accepted:
      • isismtt.x509.NamingAuthority
      • isismtt.x509.ProfessionInfo
      • x509.sigi.PersonalData
      • isismtt.x509.ProcurationSyntax, which also accepts a missing mandatory signingFor [3].
    • The deprecated x509.X509Extensions accepts the same extension twice.

    CMS streaming parsers

    • SignedDataParser, EnvelopedDataParser, AuthEnvelopedDataParser, AuthenticatedDataParser, ContentInfoParser and EncryptedContentInfoParser never check for elements after the last field. A caller that reads every field never sees them.
    • AuthenticatedDataParser.getDigestAlgorithm() does not check that the tag is [1], so [5] is accepted.

    This list may not be complete. Positional parsers with no upper size bound (or no final pos != count check) silently ignore trailing elements, and finding all of them needs a separate pass.

    While writing the reproducer I also hit a related defect in the AuthenticatedData parsers:

    CMSAuthenticatedDataParser.getUnauthAttrs() always returns null, and a ClassCastException sits behind it. It is not a leniency issue, so I filed it separately as #2532.

  6. dghgit commented on Oct 31, 2026

    @dghgit
    Contributor

    I've done another pass. Updates should be here now, and also up on https://www.bouncycastle.org/betas

  7. xipki commented on Oct 31, 2026

    @xipki
    ContributorAuthor

    I've done another pass. Updates should be here now, and also up on https://www.bouncycastle.org/betas

    Looks good. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions