Use new Bouncy Castle for tagged objects and fix bug in version parsing

pom.xml

@@ -46,8 +46,7 @@
         <java.runtime.version>11</java.runtime.version>
 
         <net.ripe.ipresource.version>1.52</net.ripe.ipresource.version>
-        <!-- TODO: Use 1.77 and patch API changes -->
-        <bouncycastle.version>1.74</bouncycastle.version>
+        <bouncycastle.version>1.77</bouncycastle.version>
         <guava.version>33.0.0-jre</guava.version>
         <joda-time.version>2.12.7</joda-time.version>
         <xstream.version>1.4.20</xstream.version>

src/main/java/net/ripe/rpki/commons/crypto/IllegalAsn1StructureException.java

@@ -0,0 +1,14 @@
+package net.ripe.rpki.commons.crypto;
+
+/**
+ * Encountered an ASN.1 structure that was not expected (e.g. implicit instead of explicit tags).
+ */
+public class IllegalAsn1StructureException extends IllegalArgumentException {
+    public IllegalAsn1StructureException(String message) {
+        super(message);
+    }
+
+    public IllegalAsn1StructureException(String message, Throwable cause) {
+        super(message, cause);
+    }
+}

src/main/java/net/ripe/rpki/commons/crypto/cms/RpkiSignedObjectParser.java

@@ -1,5 +1,6 @@
 package net.ripe.rpki.commons.crypto.cms;
 
+import net.ripe.rpki.commons.crypto.IllegalAsn1StructureException;
 import net.ripe.rpki.commons.crypto.util.BouncyCastleUtil;
 import net.ripe.rpki.commons.crypto.x509cert.AbstractX509CertificateWrapperException;
 import net.ripe.rpki.commons.crypto.x509cert.X509ResourceCertificate;
@@ -373,18 +374,34 @@ private void verifyUnsignedAttributes(SignerInformation signer) {
         validationResult.rejectIfFalse(signer.getUnsignedAttributes() == null, UNSIGNED_ATTRS_OMITTED);
     }
 
-    protected static BigInteger getRpkiObjectVersion(ASN1Sequence seq) {
-        ASN1Primitive asn1Version = seq.getObjectAt(0).toASN1Primitive();
-        BigInteger version = null;
-        if (asn1Version instanceof ASN1Integer) {
-            version = ((ASN1Integer) asn1Version).getValue();
-        } else if (asn1Version instanceof DERTaggedObject){
-            final ASN1Primitive o = ((DERTaggedObject) asn1Version).getObject();
-            if (o instanceof ASN1Integer) {
-                version = ((ASN1Integer) o).getValue();
+    /**
+     * Parse the version field in a signed object.
+     * By convention, the version is the first field in the sequence, and it is explicitly tagged.
+     * @param tagNo tag number
+     * @param seq sequence of content
+     * @return version number if present, empty otherwise.
+     */
+    protected static Optional<BigInteger> getTaggedVersion(int tagNo, ASN1Sequence seq) throws IllegalAsn1StructureException {
+        try {
+            ASN1Encodable maybeVersion = seq.getObjectAt(0).toASN1Primitive();
+            if (maybeVersion instanceof DLTaggedObject) {
+                ASN1TaggedObject tagged = (ASN1TaggedObject) maybeVersion;
+                if (tagged.getTagNo() == tagNo) {
+                    var integerVersion = tagged.getExplicitBaseObject();
+                    if (!tagged.isExplicit()) {
+                        throw new IllegalAsn1StructureException("Expected explicit tag for version field");
+                    }
+
+                    if (integerVersion instanceof ASN1Integer) {
+                        return Optional.of(((ASN1Integer) integerVersion).getValue());
+                    }
+                }
             }
+
+            return Optional.empty();
+        } catch (IllegalStateException e) {
+            // for example: version is implicitly tagged instead of explicitly
+            throw new IllegalAsn1StructureException("Error parsing version field", e);
         }
-        return version;
     }
-
 }

src/main/java/net/ripe/rpki/commons/crypto/cms/manifest/ManifestCmsParser.java

@@ -17,6 +17,7 @@
 import java.math.BigInteger;
 import java.text.ParseException;
 import java.util.Map;
+import java.util.Optional;
 import java.util.TreeMap;
 
 import static net.ripe.rpki.commons.crypto.util.Asn1Util.*;
@@ -102,8 +103,13 @@ public void decodeAsn1Content(ASN1Encodable encoded) {
             final int itemCount = seq.size();
             int offset = 0;
             if (itemCount == 6) {
-                BigInteger version = getRpkiObjectVersion(seq);
-                validationResult.rejectIfFalse(BigInteger.ZERO.equals(version), "mf.version", "manifest version must be 0, but is " + version);
+                Optional<BigInteger> optionalVersion = getTaggedVersion(0, seq);
+                optionalVersion.ifPresentOrElse(foundVersion -> {
+                    validationResult.rejectIfFalse(BigInteger.ZERO.equals(foundVersion), MANIFEST_VERSION, optionalVersion.get().toString());
+                    version = 0;
+                }, () -> {
+                    validationResult.error(MANIFEST_VERSION, "missing/not explicitly tagged");
+                });
                 offset++;
             } else if (itemCount == 5) {
                 version = ManifestCms.DEFAULT_VERSION;

src/main/java/net/ripe/rpki/commons/crypto/cms/roa/RoaCmsParser.java

@@ -4,6 +4,7 @@
 import net.ripe.ipresource.IpRange;
 import net.ripe.ipresource.IpResourceSet;
 import net.ripe.ipresource.IpResourceType;
+import net.ripe.rpki.commons.crypto.IllegalAsn1StructureException;
 import net.ripe.rpki.commons.crypto.cms.RpkiSignedObjectInfo;
 import net.ripe.rpki.commons.crypto.cms.RpkiSignedObjectParser;
 import net.ripe.rpki.commons.crypto.rfc3779.AddressFamily;
@@ -17,6 +18,7 @@
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
+import java.util.Optional;
 
 import static net.ripe.rpki.commons.crypto.util.Asn1Util.*;
 import static net.ripe.rpki.commons.validation.ValidationString.*;
@@ -96,11 +98,15 @@ void parseRouteOriginAttestation(ASN1Encodable der) {
 
             final int itemCount = seq.size();
             if (itemCount == 3) {
-                BigInteger version = getRpkiObjectVersion(seq);
-                if (validationResult.rejectIfFalse(BigInteger.ZERO.equals(version), ROA_ATTESTATION_VERSION, "attestation version must be 0, but is " + version)) {
-                    asn = Asn1Util.parseAsId(seq.getObjectAt(1));
-                    prefixes = parseRoaIpAddressFamilySequence(seq.getObjectAt(2));
-                }
+                Optional<BigInteger> maybeVersion = getTaggedVersion(0, seq);
+                maybeVersion.ifPresentOrElse(version -> {
+                    if (validationResult.rejectIfFalse(BigInteger.ZERO.equals(version), ROA_ATTESTATION_VERSION, "attestation version must be 0, but is " + version)) {
+                        asn = Asn1Util.parseAsId(seq.getObjectAt(1));
+                        prefixes = parseRoaIpAddressFamilySequence(seq.getObjectAt(2));
+                    }
+                }, () -> {
+                    validationResult.error(ROA_ATTESTATION_VERSION, "missing/not explicitly tagged");
+                });
             } else if (itemCount == 2) {
                 asn = Asn1Util.parseAsId(seq.getObjectAt(0));
                 prefixes = parseRoaIpAddressFamilySequence(seq.getObjectAt(1));

src/main/java/net/ripe/rpki/commons/crypto/rfc3779/ResourceExtensionParser.java

@@ -1,15 +1,9 @@
 package net.ripe.rpki.commons.crypto.rfc3779;
 
+import com.google.common.base.Preconditions;
 import net.ripe.ipresource.*;
-import org.apache.commons.lang3.Validate;
-import org.bouncycastle.asn1.ASN1Encodable;
-import org.bouncycastle.asn1.ASN1Integer;
-import org.bouncycastle.asn1.ASN1Null;
-import org.bouncycastle.asn1.ASN1OctetString;
-import org.bouncycastle.asn1.ASN1Primitive;
-import org.bouncycastle.asn1.ASN1Sequence;
-import org.bouncycastle.asn1.ASN1TaggedObject;
-import org.bouncycastle.asn1.DERBitString;
+import net.ripe.rpki.commons.crypto.IllegalAsn1StructureException;
+import org.bouncycastle.asn1.*;
 
 import java.security.cert.X509Certificate;
 import java.util.EnumSet;
@@ -80,7 +74,7 @@ public SortedMap<AddressFamily, IpResourceSet> parseIpAddressBlocks(byte[] exten
         }
 
         for (AddressFamily addressFamily : map.keySet()) {
-            Validate.isTrue(!addressFamily.hasSubsequentAddressFamilyIdentifier(), "SAFI not supported");
+            Preconditions.checkArgument(!addressFamily.hasSubsequentAddressFamilyIdentifier(), "SAFI not supported");
         }
 
         return map;
@@ -96,8 +90,8 @@ public IpResourceSet parseAsIdentifiers(byte[] extension) {
         expect(octetString, ASN1OctetString.class);
         ASN1OctetString o = (ASN1OctetString) octetString;
         IpResourceSet[] resources = derToAsIdentifiers(decode(o.getOctets()));
-        Validate.notNull(resources[1], "inheritance of resources has not been implemented yet");
-        Validate.isTrue(resources[1].isEmpty(), "routing domain identifiers (RDI) not supported");
+        Preconditions.checkNotNull(resources[1], "inheritance of resources has not been implemented yet");
+        Preconditions.checkArgument(resources[1].isEmpty(), "routing domain identifiers (RDI) not supported");
         return resources[0];
     }
 
@@ -120,7 +114,7 @@ SortedMap<AddressFamily, IpResourceSet> derToIpAddressBlocks(ASN1Encodable der)
      */
     void derToIpAddressFamily(ASN1Encodable der, SortedMap<AddressFamily, IpResourceSet> map) {
         ASN1Sequence seq = expect(der, ASN1Sequence.class);
-        Validate.isTrue(seq.size() == 2, "IpAddressFamily must have exactly two entries: addressFamily and IpAddressChoice");
+        Preconditions.checkArgument(seq.size() == 2, "IpAddressFamily must have exactly two entries: addressFamily and IpAddressChoice");
 
         AddressFamily addressFamily = AddressFamily.fromDer(seq.getObjectAt(0));
         IpResourceSet resources = derToIpAddressChoice(addressFamily.toIpResourceType(), seq.getObjectAt(1));
@@ -144,14 +138,14 @@ IpResourceSet derToIpAddressChoice(IpResourceType type, ASN1Encodable der) {
                 IpResource current = derToIpAddressOrRange(type, seq.getObjectAt(i));
                 // Check if previous and next are (1) in order and (2) not continuous
                 if (previous != null) {
-                    Validate.isTrue(!previous.adjacent(current), "IP resources in extension MUST NOT be adjacent");
+                    Preconditions.checkArgument(!previous.adjacent(current), "IP resources in extension MUST NOT be adjacent");
                     //    The addressesOrRanges element is a SEQUENCE OF IPAddressOrRange
                     //   types.  The addressPrefix and addressRange elements MUST be sorted
                     //   using the binary representation of:
                     //
                     //      <lowest IP address in range> | <prefix length>
                     UniqueIpResource start = current.getStart();
-                    Validate.isTrue(previous.getEnd().compareTo(start) < 0, "addressOrRanges MUST be sorted");
+                    Preconditions.checkArgument(previous.getEnd().compareTo(start) < 0, "addressOrRanges MUST be sorted");
                 }
 
                 result.add(current);
@@ -182,7 +176,7 @@ IpResource derToIpAddressOrRange(IpResourceType type, ASN1Encodable der) {
      */
     IpResource derToIpRange(IpResourceType type, ASN1Encodable der) {
         ASN1Sequence sequence = expect(der, ASN1Sequence.class);
-        Validate.isTrue(sequence.size() == 2, "IPRange MUST consist of two entries (start and end)");
+        Preconditions.checkArgument(sequence.size() == 2, "IPRange MUST consist of two entries (start and end)");
 
         IpAddress start = parseIpAddress(type, sequence.getObjectAt(0), false);
         IpAddress end = parseIpAddress(type, sequence.getObjectAt(1), true);
@@ -195,7 +189,7 @@ IpResource derToIpRange(IpResourceType type, ASN1Encodable der) {
      */
     IpResourceRange derToAsRange(ASN1Encodable der) {
         ASN1Sequence seq = expect(der, ASN1Sequence.class);
-        Validate.isTrue(seq.size() == 2, "ASN1Sequence with two elements expected");
+        Preconditions.checkArgument(seq.size() == 2, "ASN1Sequence with two elements expected");
         return parseAsId(seq.getObjectAt(0)).upTo(parseAsId(seq.getObjectAt(1)));
     }
 
@@ -232,8 +226,8 @@ IpResourceSet derToAsIdsOrRanges(ASN1Encodable der) {
             if (previous != null) {
                 UniqueIpResource start = current.getStart();
 
-                Validate.isTrue(!start.adjacent(previous.getEnd()), "ASIdOrRange entries MUST NOT be adjacent");
-                Validate.isTrue(start.max(previous.getEnd()).equals(start), "ASIdOrRange entries MUST be sorted by increasing numeric value");
+                Preconditions.checkArgument(!start.adjacent(previous.getEnd()), "ASIdOrRange entries MUST NOT be adjacent");
+                Preconditions.checkArgument(start.max(previous.getEnd()).equals(start), "ASIdOrRange entries MUST be sorted by increasing numeric value");
             }
             result.add(current);
             previous = current;
@@ -267,17 +261,22 @@ IpResourceSet derToAsIdentifierChoice(ASN1Encodable der) {
      */
     IpResourceSet[] derToAsIdentifiers(ASN1Encodable der) {
         expect(der, ASN1Sequence.class);
-        ASN1Sequence seq = (ASN1Sequence) der;
-        Validate.isTrue(seq.size() <= 2, "ASN1Sequence with 2 or fewer elements expected");
-
-        IpResourceSet[] result = {new IpResourceSet(), new IpResourceSet()};
-        for (int i = 0; i < seq.size(); ++i) {
-            expect(seq.getObjectAt(i), ASN1TaggedObject.class);
-            ASN1TaggedObject tagged = (ASN1TaggedObject) seq.getObjectAt(i);
-            Validate.isTrue(tagged.getTagNo() == 0 || tagged.getTagNo() == 1, "unknown tag no: " + tagged.getTagNo());
-            result[tagged.getTagNo()] = derToAsIdentifierChoice(tagged.getObject());
+        try {
+            ASN1Sequence seq = (ASN1Sequence) der;
+            Preconditions.checkArgument(seq.size() <= 2, "ASN1Sequence with 2 or fewer elements expected");
+
+            IpResourceSet[] result = {new IpResourceSet(), new IpResourceSet()};
+            for (int i = 0; i < seq.size(); ++i) {
+                expect(seq.getObjectAt(i), ASN1TaggedObject.class);
+                ASN1TaggedObject tagged = (ASN1TaggedObject) seq.getObjectAt(i);
+                Preconditions.checkArgument(tagged.getTagNo() == 0 || tagged.getTagNo() == 1, "unknown tag no: " + tagged.getTagNo());
+                Preconditions.checkArgument((tagged.getTagClass() & BERTags.CONTEXT_SPECIFIC) != 0, "element tag is context specific.");
+                result[tagged.getTagNo()] = derToAsIdentifierChoice(tagged.getExplicitBaseObject());
+            }
+            return result;
+        } catch (IllegalStateException e) {
+            throw new IllegalAsn1StructureException("Could not parse AsIdentifiers extension", e);
         }
-        return result;
     }
 
 }

src/main/java/net/ripe/rpki/commons/validation/ValidationString.java

@@ -171,6 +171,8 @@ private ValidationString() {
     public static final String MANIFEST_THIS_UPDATE_TIME_BEFORE_NEXT_UPDATE_TIME = "mf.this.update.before.next.update";
     public static final String MANIFEST_ENTRY_FILE_NAME_IS_RELATIVE = "mf.entry.file.name.is.relative";
 
+    public static final String MANIFEST_VERSION = "mf.version";
+
     //ghostbusters
     public static final String GHOSTBUSTERS_RECORD_CONTENT_TYPE = "ghostbusters.record.content.type";
     public static final String GHOSTBUSTERS_RECORD_SINGLE_VCARD = "ghostbusters.record.single.vcard";

src/main/resources/validation_en.properties

@@ -537,8 +537,8 @@ aspa.content.structure.passed=ASPA content structure is valid
 aspa.content.structure.warning=ASPA content structure is invalid
 aspa.content.structure.error=ASPA content structure is invalid
 aspa.version.passed=ASPA ASN.1 structure version is {0}
-aspa.version.warning=ASPA ASN.1 structure seems to have wrong version, should be 1, was {0}
-aspa.version.error=ASPA ASN.1 structure seems to have wrong version, should be 1, was {0}
+aspa.version.warning=ASPA ASN.1 structure has the wrong version, should be 1, was {0}
+aspa.version.error=ASPA ASN.1 structure has the wrong version, should be 1, was {0}
 aspa.provider.as.set.valid.passed=ASPA providers are valid
 aspa.provider.as.set.valid.warning=ASPA providers are invalid: {}
 aspa.provider.as.set.valid.error=ASPA provider set is invalid: {}
@@ -565,6 +565,10 @@ mf.content.type.passed=Content type is Manifest
 mf.content.type.warning=Content type is not Manifest
 mf.content.type.error=Content type is not Manifest
 
+mf.version.passed=Manifest ASN.1 structure version is {0}
+mf.version.warning=Manifest ASN.1 structure has the have wrong version, should be 0, was {0}
+mf.version.error=Manifest ASN.1 structure has the wrong version, should be 0, was {0}
+
 mf.content.size.passed=Manifest content size is valid
 mf.content.size.warning=Manifest content size is invalid
 mf.content.size.error=Manifest content size is invalid

src/test/java/net/ripe/rpki/commons/interop/BBNRoaConformanceTest.java

@@ -3,12 +3,14 @@
 import com.google.common.io.Files;
 import net.ripe.rpki.commons.crypto.cms.roa.RoaCmsParser;
 import net.ripe.rpki.commons.validation.ValidationResult;
-import org.junit.Test;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.CsvSource;
 
 import java.io.File;
 import java.io.IOException;
 
-import static org.junit.Assert.*;
+import static org.assertj.core.api.Assertions.assertThat;
 
 public class BBNRoaConformanceTest {
 
@@ -18,14 +20,32 @@ public class BBNRoaConformanceTest {
     public void shouldParseRoaWithMissingSignature() throws IOException {
         // no signature 6488#2.1.6.6
         boolean hasFailure = parseRoa("root/badCMSSigInfoNoSig.roa");
-        assertTrue(hasFailure);
+        assertThat(hasFailure).isTrue();
     }
 
     @Test
     public void shouldParseRoaWithNoSignerInfo() throws IOException {
         // empty set of SignerInfos 6488#2.1
         boolean hasFailure = parseRoa("root/badCMSNoSigInfo.roa");
-        assertTrue(hasFailure);
+        assertThat(hasFailure).isTrue();
+    }
+
+    /**
+     * Apply a number of test cases for version.
+     *
+     * Note that in these objects version is <emph>implicit</emph> not <emph>explicit</emph> as required.
+     */
+    @CsvSource({
+        "557, VersionV1Explicit, # explicit V1 version (int 0) applied before signature 6482#3",
+        "558, VersionV1ExplicitBadSig, # explicit V1 version (int 0) applied after signature 6482#3",
+        "559, VersionV2, # Version V2 (int 1) 6482#3.1"
+    })
+    @ParameterizedTest(name = "{displayName} - {0} {1} {2}")
+    public void shouldRejectBadRoaObject(String testNumber, String testCaseFile, String testCaseDescription) throws IOException {
+        final String fileName = String.format("root/badROA%s.roa", testCaseFile);
+
+        assertThat(parseRoa(fileName)).isTrue()
+                .withFailMessage("Should reject certificate with " + testCaseDescription + " from " + fileName);
     }
 
     private boolean parseRoa(String roa) throws IOException {