ASN.1: Limit OID contents to 4096 bytes

3 files changed, 108 insertions, 54 deletions

diff --git a/crypto/src/asn1/Asn1InputStream.cs b/crypto/src/asn1/Asn1InputStream.cs
index a1bc987cc..3b5eaaa95 100644
--- a/crypto/src/asn1/Asn1InputStream.cs
+++ b/crypto/src/asn1/Asn1InputStream.cs
@@ -392,11 +392,13 @@ namespace Org.BouncyCastle.Asn1
             }
             case Asn1Tags.ObjectIdentifier:
             {
+                DerObjectIdentifier.CheckContentsLength(defIn.Remaining);
                 bool usedBuffer = GetBuffer(defIn, tmpBuffers, out var contents);
                 return DerObjectIdentifier.CreatePrimitive(contents, clone: usedBuffer);
             }
             case Asn1Tags.RelativeOid:
             {
+                Asn1RelativeOid.CheckContentsLength(defIn.Remaining);
                 bool usedBuffer = GetBuffer(defIn, tmpBuffers, out var contents);
                 return Asn1RelativeOid.CreatePrimitive(contents, clone: usedBuffer);
             }
diff --git a/crypto/src/asn1/Asn1RelativeOid.cs b/crypto/src/asn1/Asn1RelativeOid.cs
index 3e2b54c9e..f1b9bc274 100644
--- a/crypto/src/asn1/Asn1RelativeOid.cs
+++ b/crypto/src/asn1/Asn1RelativeOid.cs
@@ -23,6 +23,16 @@ namespace Org.BouncyCastle.Asn1
             }
         }
 
+        /// <summary>Implementation limit on the length of the contents octets for a Relative OID.</summary>
+        /// <remarks>
+        /// We adopt the same value used by OpenJDK for Object Identifier. In theory there is no limit on the length of
+        /// the contents, or the number of subidentifiers, or the length of individual subidentifiers. In practice,
+        /// supporting arbitrary lengths can lead to issues, e.g. denial-of-service attacks when attempting to convert a
+        /// parsed value to its (decimal) string form.
+        /// </remarks>
+        private const int MaxContentsLength = 4096;
+        private const int MaxIdentifierLength = MaxContentsLength * 4 - 1;
+
         public static Asn1RelativeOid FromContents(byte[] contents)
         {
             if (contents == null)
@@ -69,14 +79,18 @@ namespace Org.BouncyCastle.Asn1
         {
             if (identifier == null)
                 throw new ArgumentNullException(nameof(identifier));
-            if (!IsValidIdentifier(identifier, 0))
+            if (identifier.Length <= MaxIdentifierLength && IsValidIdentifier(identifier, from: 0))
             {
-                oid = default;
-                return false;
+                byte[] contents = ParseIdentifier(identifier);
+                if (contents.Length <= MaxContentsLength)
+                {
+                    oid = new Asn1RelativeOid(contents, identifier);
+                    return true;
+                }
             }
 
-            oid = new Asn1RelativeOid(ParseIdentifier(identifier), identifier);
-            return true;
+            oid = default;
+            return false;
         }
 
         private const long LongLimit = (long.MaxValue >> 7) - 0x7F;
@@ -88,31 +102,13 @@ namespace Org.BouncyCastle.Asn1
 
         public Asn1RelativeOid(string identifier)
         {
-            if (identifier == null)
-                throw new ArgumentNullException("identifier");
-            if (!IsValidIdentifier(identifier, 0))
-                throw new FormatException("string " + identifier + " not a relative OID");
+            CheckIdentifier(identifier);
 
-            m_contents = ParseIdentifier(identifier);
-            m_identifier = identifier;
-        }
-
-        private Asn1RelativeOid(Asn1RelativeOid oid, string branchID)
-        {
-            if (!IsValidIdentifier(branchID, 0))
-                throw new FormatException("string " + branchID + " not a valid relative OID branch");
-
-            m_contents = Arrays.Concatenate(oid.m_contents, ParseIdentifier(branchID));
-            m_identifier = oid.GetID() + "." + branchID;
-        }
-
-        private Asn1RelativeOid(byte[] contents, bool clone)
-        {
-            if (!IsValidContents(contents))
-                throw new ArgumentException("invalid relative OID contents", nameof(contents));
+            byte[] contents = ParseIdentifier(identifier);
+            CheckContentsLength(contents.Length);
 
-            m_contents = clone ? Arrays.Clone(contents) : contents;
-            m_identifier = null;
+            m_contents = contents;
+            m_identifier = identifier;
         }
 
         private Asn1RelativeOid(byte[] contents, string identifier)
@@ -123,7 +119,14 @@ namespace Org.BouncyCastle.Asn1
 
         public virtual Asn1RelativeOid Branch(string branchID)
         {
-            return new Asn1RelativeOid(this, branchID);
+            CheckIdentifier(branchID);
+
+            byte[] branchContents = ParseIdentifier(branchID);
+            CheckContentsLength(m_contents.Length + branchContents.Length);
+
+            return new Asn1RelativeOid(
+                contents: Arrays.Concatenate(m_contents, branchContents),
+                identifier: GetID() + "." + branchID);
         }
 
         public string GetID()
@@ -168,8 +171,26 @@ namespace Org.BouncyCastle.Asn1
             return new PrimitiveDerEncoding(tagClass, tagNo, m_contents);
         }
 
+        internal static void CheckContentsLength(int contentsLength)
+        {
+            if (contentsLength > MaxContentsLength)
+                throw new ArgumentException("exceeded relative OID contents length limit");
+        }
+
+        internal static void CheckIdentifier(string identifier)
+        {
+            if (identifier == null)
+                throw new ArgumentNullException("identifier");
+            if (identifier.Length > MaxIdentifierLength)
+                throw new ArgumentException("exceeded relative OID contents length limit");
+            if (!IsValidIdentifier(identifier, from: 0))
+                throw new FormatException("string " + identifier + " not a valid relative OID");
+        }
+
         internal static Asn1RelativeOid CreatePrimitive(byte[] contents, bool clone)
         {
+            CheckContentsLength(contents.Length);
+
             uint index = (uint)Arrays.GetHashCode(contents);
 
             index ^= index >> 24;
@@ -181,7 +202,10 @@ namespace Org.BouncyCastle.Asn1
             if (originalEntry != null && Arrays.AreEqual(contents, originalEntry.m_contents))
                 return originalEntry;
 
-            var newEntry = new Asn1RelativeOid(contents, clone);
+            if (!IsValidContents(contents))
+                throw new ArgumentException("invalid relative OID contents", nameof(contents));
+
+            var newEntry = new Asn1RelativeOid(clone ? Arrays.Clone(contents) : contents, identifier: null);
 
             var exchangedEntry = Interlocked.CompareExchange(ref Cache[index], newEntry, originalEntry);
             if (exchangedEntry != originalEntry)
diff --git a/crypto/src/asn1/DerObjectIdentifier.cs b/crypto/src/asn1/DerObjectIdentifier.cs
index 51acd232c..4a68d541c 100644
--- a/crypto/src/asn1/DerObjectIdentifier.cs
+++ b/crypto/src/asn1/DerObjectIdentifier.cs
@@ -23,6 +23,16 @@ namespace Org.BouncyCastle.Asn1
             }
         }
 
+        /// <summary>Implementation limit on the length of the contents octets for an Object Identifier.</summary>
+        /// <remarks>
+        /// We adopt the same value used by OpenJDK. In theory there is no limit on the length of the contents, or the
+        /// number of subidentifiers, or the length of individual subidentifiers. In practice, supporting arbitrary
+        /// lengths can lead to issues, e.g. denial-of-service attacks when attempting to convert a parsed value to its
+        /// (decimal) string form.
+        /// </remarks>
+        private const int MaxContentsLength = 4096;
+        private const int MaxIdentifierLength = MaxContentsLength * 4 + 1;
+
         public static DerObjectIdentifier FromContents(byte[] contents)
         {
             if (contents == null)
@@ -86,14 +96,18 @@ namespace Org.BouncyCastle.Asn1
         {
             if (identifier == null)
                 throw new ArgumentNullException(nameof(identifier));
-            if (!IsValidIdentifier(identifier))
+            if (identifier.Length <= MaxIdentifierLength && IsValidIdentifier(identifier))
             {
-                oid = default;
-                return false;
+                byte[] contents = ParseIdentifier(identifier);
+                if (contents.Length <= MaxContentsLength)
+                {
+                    oid = new DerObjectIdentifier(contents, identifier);
+                    return true;
+                }
             }
 
-            oid = new DerObjectIdentifier(ParseIdentifier(identifier), identifier);
-            return true;
+            oid = default;
+            return false;
         }
 
         private const long LongLimit = (long.MaxValue >> 7) - 0x7F;
@@ -105,22 +119,13 @@ namespace Org.BouncyCastle.Asn1
 
         public DerObjectIdentifier(string identifier)
         {
-            if (identifier == null)
-                throw new ArgumentNullException("identifier");
-            if (!IsValidIdentifier(identifier))
-                throw new FormatException("string " + identifier + " not an OID");
-
-            m_contents = ParseIdentifier(identifier);
-            m_identifier = identifier;
-        }
+            CheckIdentifier(identifier);
 
-        private DerObjectIdentifier(byte[] contents, bool clone)
-        {
-            if (!Asn1RelativeOid.IsValidContents(contents))
-                throw new ArgumentException("invalid OID contents", nameof(contents));
+            byte[] contents = ParseIdentifier(identifier);
+            CheckContentsLength(contents.Length);
 
-            m_contents = clone ? Arrays.Clone(contents) : contents;
-            m_identifier = null;
+            m_contents = contents;
+            m_identifier = identifier;
         }
 
         private DerObjectIdentifier(byte[] contents, string identifier)
@@ -131,11 +136,13 @@ namespace Org.BouncyCastle.Asn1
 
         public virtual DerObjectIdentifier Branch(string branchID)
         {
-            if (!Asn1RelativeOid.IsValidIdentifier(branchID, 0))
-                throw new FormatException("string " + branchID + " not a valid OID branch");
+            Asn1RelativeOid.CheckIdentifier(branchID);
+
+            byte[] branchContents = Asn1RelativeOid.ParseIdentifier(branchID);
+            CheckContentsLength(m_contents.Length + branchContents.Length);
 
             return new DerObjectIdentifier(
-                contents: Arrays.Concatenate(m_contents, Asn1RelativeOid.ParseIdentifier(branchID)),
+                contents: Arrays.Concatenate(m_contents, branchContents),
                 identifier: GetID() + "." + branchID);
         }
 
@@ -195,8 +202,26 @@ namespace Org.BouncyCastle.Asn1
             return new PrimitiveDerEncoding(tagClass, tagNo, m_contents);
         }
 
+        internal static void CheckContentsLength(int contentsLength)
+        {
+            if (contentsLength > MaxContentsLength)
+                throw new ArgumentException("exceeded OID contents length limit");
+        }
+
+        internal static void CheckIdentifier(string identifier)
+        {
+            if (identifier == null)
+                throw new ArgumentNullException("identifier");
+            if (identifier.Length > MaxIdentifierLength)
+                throw new ArgumentException("exceeded OID contents length limit");
+            if (!IsValidIdentifier(identifier))
+                throw new FormatException("string " + identifier + " not a valid OID");
+        }
+
         internal static DerObjectIdentifier CreatePrimitive(byte[] contents, bool clone)
         {
+            CheckContentsLength(contents.Length);
+
             uint index = (uint)Arrays.GetHashCode(contents);
 
             index ^= index >> 20;
@@ -207,7 +232,10 @@ namespace Org.BouncyCastle.Asn1
             if (originalEntry != null && Arrays.AreEqual(contents, originalEntry.m_contents))
                 return originalEntry;
 
-            var newEntry = new DerObjectIdentifier(contents, clone);
+            if (!Asn1RelativeOid.IsValidContents(contents))
+                throw new ArgumentException("invalid OID contents", nameof(contents));
+
+            var newEntry = new DerObjectIdentifier(clone ? Arrays.Clone(contents) : contents, identifier: null);
 
             var exchangedEntry = Interlocked.CompareExchange(ref Cache[index], newEntry, originalEntry);
             if (exchangedEntry != originalEntry)
@@ -228,7 +256,7 @@ namespace Org.BouncyCastle.Asn1
             if (first < '0' || first > '2')
                 return false;
 
-            if (!Asn1RelativeOid.IsValidIdentifier(identifier, 2))
+            if (!Asn1RelativeOid.IsValidIdentifier(identifier, from: 2))
                 return false;
 
             if (first == '2')