Harden X509Certificate collections

justinvp · justinvp · commit 1acb8f44664f · 2015-08-17T17:05:26.000-07:00
This commit introduces minor behavior changes in order to harden
the X509Certificate collections in .NET Core:

1. Using X509CertificateCollection or X509Certificate2Collection as
an IList allows adding/inserting non-X509Certificate objects into the
collection. Subsequent calls to enumerate or access these items
results in InvalidCastException being thrown. This matches the full
.NET Framework (desktop) behavior. Clearly this is not the intended
behavior of these collections, which are meant to hold
X509Certificate and X509Certificate2 objects. This commit changes
X509CertificateCollection and X509Certificate2Collection to throw
ArgumentException when non-X509Certificate objects are attempted to
be added/inserted, via List&lt;X509Certificate&gt;. As part of this,
cleanup calling the non-generic IList implementations and null
checking.

2. X509CertificateEnumerator has a public constructor with a
collection parameter that it uses without first verifying the
parameter is non-null, which could result in NullReferenceExceptions.
This commit adds a null check, throwing ArgumentNullException if the
parameter is null.
diff --git a/src/System.Security.Cryptography.X509Certificates/src/System/Security/Cryptography/X509Certificates/X509Certificate2Collection.cs b/src/System.Security.Cryptography.X509Certificates/src/System/Security/Cryptography/X509Certificates/X509Certificate2Collection.cs
@@ -30,14 +30,11 @@ public X509Certificate2Collection(X509Certificate2Collection certificates)
         {
             get
             {
-                return (X509Certificate2)(List[index]);
+                return (X509Certificate2)(base[index]);
             }
             set
             {
-                if (value == null)
-                    throw new ArgumentNullException("value");
-
-                List[index] = value;
+                base[index] = value;
             }
         }
 
@@ -46,7 +43,7 @@ public int Add(X509Certificate2 certificate)
             if (certificate == null)
                 throw new ArgumentNullException("certificate");
 
-            return List.Add(certificate);
+            return base.Add(certificate);
         }
 
         public void AddRange(X509Certificate2[] certificates)
@@ -100,7 +97,7 @@ public bool Contains(X509Certificate2 certificate)
             if (certificate == null)
                 throw new ArgumentNullException("certificate");
 
-            return List.Contains(certificate);
+            return base.Contains(certificate);
         }
 
         public byte[] Export(X509ContentType contentType)
@@ -171,15 +168,15 @@ public void Insert(int index, X509Certificate2 certificate)
             if (certificate == null)
                 throw new ArgumentNullException("certificate");
 
-            List.Insert(index, certificate);
+            base.Insert(index, certificate);
         }
 
         public void Remove(X509Certificate2 certificate)
         {
             if (certificate == null)
                 throw new ArgumentNullException("certificate");
 
-            List.Remove(certificate);
+            base.Remove(certificate);
         }
 
         public void RemoveRange(X509Certificate2[] certificates)
diff --git a/src/System.Security.Cryptography.X509Certificates/src/System/Security/Cryptography/X509Certificates/X509Certificate2Enumerator.cs b/src/System.Security.Cryptography.X509Certificates/src/System/Security/Cryptography/X509Certificates/X509Certificate2Enumerator.cs
@@ -10,7 +10,7 @@ namespace System.Security.Cryptography.X509Certificates
     public sealed class X509Certificate2Enumerator : IEnumerator
     {
         // This is a mutable struct enumerator, so don't mark it as readonly.
-        private List<object>.Enumerator _enumerator;
+        private List<X509Certificate>.Enumerator _enumerator;
 
         internal X509Certificate2Enumerator(X509Certificate2Collection collection)
         {
diff --git a/src/System.Security.Cryptography.X509Certificates/src/System/Security/Cryptography/X509Certificates/X509CertificateCollection.cs b/src/System.Security.Cryptography.X509Certificates/src/System/Security/Cryptography/X509Certificates/X509CertificateCollection.cs
@@ -8,11 +8,11 @@ namespace System.Security.Cryptography.X509Certificates
 {
     public partial class X509CertificateCollection : ICollection, IEnumerable, IList
     {
-        private readonly List<object> _list;
+        private readonly List<X509Certificate> _list;
 
         public X509CertificateCollection()
         {
-            _list = new List<object>();
+            _list = new List<X509Certificate>();
         }
 
         public X509CertificateCollection(X509Certificate[] value)
@@ -39,7 +39,7 @@ bool ICollection.IsSynchronized
 
         object ICollection.SyncRoot
         {
-            get { return ((ICollection)_list).SyncRoot; }
+            get { return NonGenericList.SyncRoot; }
         }
 
         bool IList.IsFixedSize
@@ -56,33 +56,40 @@ object IList.this[int index]
         {
             get
             {
-                return _list[index];
+                return NonGenericList[index];
             }
             set
             {
                 if (value == null)
                     throw new ArgumentNullException("value");
 
-                _list[index] = value;
+                NonGenericList[index] = value;
             }
         }
 
         public X509Certificate this[int index]
         {
             get
             {
-                // Note: If a non-X509Certificate was inserted at this position, the result InvalidCastException is the defined behavior.
-                return (X509Certificate)(List[index]);
+                return _list[index];
             }
             set
             {
-                List[index] = value;
+                if (value == null)
+                    throw new ArgumentNullException("value");
+
+                _list[index] = value;
             }
         }
 
         public int Add(X509Certificate value)
         {
-            return List.Add(value);
+            if (value == null)
+                throw new ArgumentNullException("value");
+
+            int index = _list.Count;
+            _list.Add(value);
+            return index;
         }
 
         public void AddRange(X509Certificate[] value)
@@ -119,7 +126,7 @@ public bool Contains(X509Certificate value)
 
         public void CopyTo(X509Certificate[] array, int index)
         {
-            List.CopyTo(array, index);
+            _list.CopyTo(array, index);
         }
 
         public X509CertificateEnumerator GetEnumerator()
@@ -144,17 +151,23 @@ public override int GetHashCode()
 
         public int IndexOf(X509Certificate value)
         {
-            return List.IndexOf(value);
+            return _list.IndexOf(value);
         }
 
         public void Insert(int index, X509Certificate value)
         {
-            List.Insert(index, value);
+            if (value == null)
+                throw new ArgumentNullException("value");
+
+            _list.Insert(index, value);
         }
 
         public void Remove(X509Certificate value)
         {
-            List.Remove(value);
+            if (value == null)
+                throw new ArgumentNullException("value");
+
+            _list.Remove(value);
         }
 
         public void RemoveAt(int index)
@@ -164,51 +177,49 @@ public void RemoveAt(int index)
 
         void ICollection.CopyTo(Array array, int index)
         {
-            ((ICollection)_list).CopyTo(array, index);
+            NonGenericList.CopyTo(array, index);
         }
 
         int IList.Add(object value)
         {
             if (value == null)
                 throw new ArgumentNullException("value");
 
-            int index = _list.Count;
-            _list.Add(value);
-            return index;
+            return NonGenericList.Add(value);
         }
 
         bool IList.Contains(object value)
         {
-            return _list.Contains(value);
+            return NonGenericList.Contains(value);
         }
 
         int IList.IndexOf(object value)
         {
-            return _list.IndexOf(value);
+            return NonGenericList.IndexOf(value);
         }
 
         void IList.Insert(int index, object value)
         {
             if (value == null)
                 throw new ArgumentNullException("value");
 
-            _list.Insert(index, value);
+            NonGenericList.Insert(index, value);
         }
 
         void IList.Remove(object value)
         {
             if (value == null)
                 throw new ArgumentNullException("value");
 
-            _list.Remove(value);
+            NonGenericList.Remove(value);
         }
 
-        internal IList List
+        private IList NonGenericList
         {
-            get { return this; }
+            get { return _list; }
         }
 
-        internal void GetEnumerator(out List<object>.Enumerator enumerator)
+        internal void GetEnumerator(out List<X509Certificate>.Enumerator enumerator)
         {
             enumerator = _list.GetEnumerator();
         }
diff --git a/src/System.Security.Cryptography.X509Certificates/src/System/Security/Cryptography/X509Certificates/X509CertificateEnumerator.cs b/src/System.Security.Cryptography.X509Certificates/src/System/Security/Cryptography/X509Certificates/X509CertificateEnumerator.cs
@@ -11,10 +11,13 @@ public partial class X509CertificateCollection : ICollection, IEnumerable, IList
         public class X509CertificateEnumerator : IEnumerator
         {
             // This is a mutable struct enumerator, so don't mark it as readonly.
-            private List<object>.Enumerator _enumerator;
+            private List<X509Certificate>.Enumerator _enumerator;
 
             public X509CertificateEnumerator(X509CertificateCollection mappings)
             {
+                if (mappings == null)
+                    throw new ArgumentNullException("mappings");
+
                 mappings.GetEnumerator(out _enumerator);
             }
 
diff --git a/src/System.Security.Cryptography.X509Certificates/tests/CollectionTests.cs b/src/System.Security.Cryptography.X509Certificates/tests/CollectionTests.cs
@@ -111,6 +111,8 @@ public static void X509CertificateCollectionThrowsArgumentNullException()
                 Assert.Throws<ArgumentNullException>(() => ilist.Insert(0, null));
                 Assert.Throws<ArgumentNullException>(() => ilist.Remove(null));
             }
+
+            Assert.Throws<ArgumentNullException>(() => new X509CertificateCollection.X509CertificateEnumerator(null));
         }
 
         [Fact]
@@ -273,11 +275,10 @@ public static void X509CertificateCollectionAsIList()
                 IList il = cc;
                 Assert.Throws<ArgumentNullException>(() => il[0] = null);
 
-                il[0] = "Bogus";
-                Assert.Equal("Bogus", il[0]);
-
-                object ignored;
-                Assert.Throws<InvalidCastException>(() => ignored = cc[0]);
+                string bogus = "Bogus";
+                Assert.Throws<ArgumentException>(() => il[0] = bogus);
+                Assert.Throws<ArgumentException>(() => il.Add(bogus));
+                Assert.Throws<ArgumentException>(() => il.Insert(0, bogus));
             }
         }
 

Original file line number	Diff line number	Diff line change
`@@ -30,14 +30,11 @@ public X509Certificate2Collection(X509Certificate2Collection certificates)`
`30`	`30`	`{`
`31`	`31`	`get`
`32`	`32`	`{`
`33`		`- return (X509Certificate2)(List[index]);`
	`33`	`+ return (X509Certificate2)(base[index]);`
`34`	`34`	`}`
`35`	`35`	`set`
`36`	`36`	`{`
`37`		`- if (value == null)`
`38`		`- throw new ArgumentNullException("value");`
`39`		`-`
`40`		`- List[index] = value;`
	`37`	`+ base[index] = value;`
`41`	`38`	`}`
`42`	`39`	`}`
`43`	`40`
`@@ -46,7 +43,7 @@ public int Add(X509Certificate2 certificate)`
`46`	`43`	`if (certificate == null)`
`47`	`44`	`throw new ArgumentNullException("certificate");`
`48`	`45`
`49`		`- return List.Add(certificate);`
	`46`	`+ return base.Add(certificate);`
`50`	`47`	`}`
`51`	`48`
`52`	`49`	`public void AddRange(X509Certificate2[] certificates)`
`@@ -100,7 +97,7 @@ public bool Contains(X509Certificate2 certificate)`
`100`	`97`	`if (certificate == null)`
`101`	`98`	`throw new ArgumentNullException("certificate");`
`102`	`99`
`103`		`- return List.Contains(certificate);`
	`100`	`+ return base.Contains(certificate);`
`104`	`101`	`}`
`105`	`102`
`106`	`103`	`public byte[] Export(X509ContentType contentType)`
`@@ -171,15 +168,15 @@ public void Insert(int index, X509Certificate2 certificate)`
`171`	`168`	`if (certificate == null)`
`172`	`169`	`throw new ArgumentNullException("certificate");`
`173`	`170`
`174`		`- List.Insert(index, certificate);`
	`171`	`+ base.Insert(index, certificate);`
`175`	`172`	`}`
`176`	`173`
`177`	`174`	`public void Remove(X509Certificate2 certificate)`
`178`	`175`	`{`
`179`	`176`	`if (certificate == null)`
`180`	`177`	`throw new ArgumentNullException("certificate");`
`181`	`178`
`182`		`- List.Remove(certificate);`
	`179`	`+ base.Remove(certificate);`
`183`	`180`	`}`
`184`	`181`
`185`	`182`	`public void RemoveRange(X509Certificate2[] certificates)`
Original file line number	Diff line number	Diff line change
`@@ -10,7 +10,7 @@ namespace System.Security.Cryptography.X509Certificates`
`10`	`10`	`public sealed class X509Certificate2Enumerator : IEnumerator`
`11`	`11`	`{`
`12`	`12`	`// This is a mutable struct enumerator, so don't mark it as readonly.`
`13`		`- private List<object>.Enumerator _enumerator;`
	`13`	`+ private List<X509Certificate>.Enumerator _enumerator;`
`14`	`14`
`15`	`15`	`internal X509Certificate2Enumerator(X509Certificate2Collection collection)`
`16`	`16`	`{`
Original file line number	Diff line number	Diff line change
`@@ -8,11 +8,11 @@ namespace System.Security.Cryptography.X509Certificates`
`8`	`8`	`{`
`9`	`9`	`public partial class X509CertificateCollection : ICollection, IEnumerable, IList`
`10`	`10`	`{`
`11`		`- private readonly List<object> _list;`
	`11`	`+ private readonly List<X509Certificate> _list;`
`12`	`12`
`13`	`13`	`public X509CertificateCollection()`
`14`	`14`	`{`
`15`		`- _list = new List<object>();`
	`15`	`+ _list = new List<X509Certificate>();`
`16`	`16`	`}`
`17`	`17`
`18`	`18`	`public X509CertificateCollection(X509Certificate[] value)`
`@@ -39,7 +39,7 @@ bool ICollection.IsSynchronized`
`39`	`39`
`40`	`40`	`object ICollection.SyncRoot`
`41`	`41`	`{`
`42`		`- get { return ((ICollection)_list).SyncRoot; }`
	`42`	`+ get { return NonGenericList.SyncRoot; }`
`43`	`43`	`}`
`44`	`44`
`45`	`45`	`bool IList.IsFixedSize`
`@@ -56,33 +56,40 @@ object IList.this[int index]`
`56`	`56`	`{`
`57`	`57`	`get`
`58`	`58`	`{`
`59`		`- return _list[index];`
	`59`	`+ return NonGenericList[index];`
`60`	`60`	`}`
`61`	`61`	`set`
`62`	`62`	`{`
`63`	`63`	`if (value == null)`
`64`	`64`	`throw new ArgumentNullException("value");`
`65`	`65`
`66`		`- _list[index] = value;`
	`66`	`+ NonGenericList[index] = value;`
`67`	`67`	`}`
`68`	`68`	`}`
`69`	`69`
`70`	`70`	`public X509Certificate this[int index]`
`71`	`71`	`{`
`72`	`72`	`get`
`73`	`73`	`{`
`74`		`- // Note: If a non-X509Certificate was inserted at this position, the result InvalidCastException is the defined behavior.`
`75`		`- return (X509Certificate)(List[index]);`
	`74`	`+ return _list[index];`
`76`	`75`	`}`
`77`	`76`	`set`
`78`	`77`	`{`
`79`		`- List[index] = value;`
	`78`	`+ if (value == null)`
	`79`	`+ throw new ArgumentNullException("value");`
	`80`	`+`
	`81`	`+ _list[index] = value;`
`80`	`82`	`}`
`81`	`83`	`}`
`82`	`84`
`83`	`85`	`public int Add(X509Certificate value)`
`84`	`86`	`{`
`85`		`- return List.Add(value);`
	`87`	`+ if (value == null)`
	`88`	`+ throw new ArgumentNullException("value");`
	`89`	`+`
	`90`	`+ int index = _list.Count;`
	`91`	`+ _list.Add(value);`
	`92`	`+ return index;`
`86`	`93`	`}`
`87`	`94`
`88`	`95`	`public void AddRange(X509Certificate[] value)`
`@@ -119,7 +126,7 @@ public bool Contains(X509Certificate value)`
`119`	`126`
`120`	`127`	`public void CopyTo(X509Certificate[] array, int index)`
`121`	`128`	`{`
`122`		`- List.CopyTo(array, index);`
	`129`	`+ _list.CopyTo(array, index);`
`123`	`130`	`}`
`124`	`131`
`125`	`132`	`public X509CertificateEnumerator GetEnumerator()`
`@@ -144,17 +151,23 @@ public override int GetHashCode()`
`144`	`151`
`145`	`152`	`public int IndexOf(X509Certificate value)`
`146`	`153`	`{`
`147`		`- return List.IndexOf(value);`
	`154`	`+ return _list.IndexOf(value);`
`148`	`155`	`}`
`149`	`156`
`150`	`157`	`public void Insert(int index, X509Certificate value)`
`151`	`158`	`{`
`152`		`- List.Insert(index, value);`
	`159`	`+ if (value == null)`
	`160`	`+ throw new ArgumentNullException("value");`
	`161`	`+`
	`162`	`+ _list.Insert(index, value);`
`153`	`163`	`}`
`154`	`164`
`155`	`165`	`public void Remove(X509Certificate value)`
`156`	`166`	`{`
`157`		`- List.Remove(value);`
	`167`	`+ if (value == null)`
	`168`	`+ throw new ArgumentNullException("value");`
	`169`	`+`
	`170`	`+ _list.Remove(value);`
`158`	`171`	`}`
`159`	`172`
`160`	`173`	`public void RemoveAt(int index)`
`@@ -164,51 +177,49 @@ public void RemoveAt(int index)`
`164`	`177`
`165`	`178`	`void ICollection.CopyTo(Array array, int index)`
`166`	`179`	`{`
`167`		`- ((ICollection)_list).CopyTo(array, index);`
	`180`	`+ NonGenericList.CopyTo(array, index);`
`168`	`181`	`}`
`169`	`182`
`170`	`183`	`int IList.Add(object value)`
`171`	`184`	`{`
`172`	`185`	`if (value == null)`
`173`	`186`	`throw new ArgumentNullException("value");`
`174`	`187`
`175`		`- int index = _list.Count;`
`176`		`- _list.Add(value);`
`177`		`- return index;`
	`188`	`+ return NonGenericList.Add(value);`
`178`	`189`	`}`
`179`	`190`
`180`	`191`	`bool IList.Contains(object value)`
`181`	`192`	`{`
`182`		`- return _list.Contains(value);`
	`193`	`+ return NonGenericList.Contains(value);`
`183`	`194`	`}`
`184`	`195`
`185`	`196`	`int IList.IndexOf(object value)`
`186`	`197`	`{`
`187`		`- return _list.IndexOf(value);`
	`198`	`+ return NonGenericList.IndexOf(value);`
`188`	`199`	`}`
`189`	`200`
`190`	`201`	`void IList.Insert(int index, object value)`
`191`	`202`	`{`
`192`	`203`	`if (value == null)`
`193`	`204`	`throw new ArgumentNullException("value");`
`194`	`205`
`195`		`- _list.Insert(index, value);`
	`206`	`+ NonGenericList.Insert(index, value);`
`196`	`207`	`}`
`197`	`208`
`198`	`209`	`void IList.Remove(object value)`
`199`	`210`	`{`
`200`	`211`	`if (value == null)`
`201`	`212`	`throw new ArgumentNullException("value");`
`202`	`213`
`203`		`- _list.Remove(value);`
	`214`	`+ NonGenericList.Remove(value);`
`204`	`215`	`}`
`205`	`216`
`206`		`- internal IList List`
	`217`	`+ private IList NonGenericList`
`207`	`218`	`{`
`208`		`- get { return this; }`
	`219`	`+ get { return _list; }`
`209`	`220`	`}`
`210`	`221`
`211`		`- internal void GetEnumerator(out List<object>.Enumerator enumerator)`
	`222`	`+ internal void GetEnumerator(out List<X509Certificate>.Enumerator enumerator)`
`212`	`223`	`{`
`213`	`224`	`enumerator = _list.GetEnumerator();`
`214`	`225`	`}`
Original file line number	Diff line number	Diff line change
`@@ -11,10 +11,13 @@ public partial class X509CertificateCollection : ICollection, IEnumerable, IList`
`11`	`11`	`public class X509CertificateEnumerator : IEnumerator`
`12`	`12`	`{`
`13`	`13`	`// This is a mutable struct enumerator, so don't mark it as readonly.`
`14`		`- private List<object>.Enumerator _enumerator;`
	`14`	`+ private List<X509Certificate>.Enumerator _enumerator;`
`15`	`15`
`16`	`16`	`public X509CertificateEnumerator(X509CertificateCollection mappings)`
`17`	`17`	`{`
	`18`	`+ if (mappings == null)`
	`19`	`+ throw new ArgumentNullException("mappings");`
	`20`	`+`
`18`	`21`	`mappings.GetEnumerator(out _enumerator);`
`19`	`22`	`}`
`20`	`23`
Original file line number	Diff line number	Diff line change
`@@ -111,6 +111,8 @@ public static void X509CertificateCollectionThrowsArgumentNullException()`
`111`	`111`	`Assert.Throws<ArgumentNullException>(() => ilist.Insert(0, null));`
`112`	`112`	`Assert.Throws<ArgumentNullException>(() => ilist.Remove(null));`
`113`	`113`	`}`
	`114`	`+`
	`115`	`+ Assert.Throws<ArgumentNullException>(() => new X509CertificateCollection.X509CertificateEnumerator(null));`
`114`	`116`	`}`
`115`	`117`
`116`	`118`	`[Fact]`
`@@ -273,11 +275,10 @@ public static void X509CertificateCollectionAsIList()`
`273`	`275`	`IList il = cc;`
`274`	`276`	`Assert.Throws<ArgumentNullException>(() => il[0] = null);`
`275`	`277`
`276`		`- il[0] = "Bogus";`
`277`		`- Assert.Equal("Bogus", il[0]);`
`278`		`-`
`279`		`- object ignored;`
`280`		`- Assert.Throws<InvalidCastException>(() => ignored = cc[0]);`
	`278`	`+ string bogus = "Bogus";`
	`279`	`+ Assert.Throws<ArgumentException>(() => il[0] = bogus);`
	`280`	`+ Assert.Throws<ArgumentException>(() => il.Add(bogus));`
	`281`	`+ Assert.Throws<ArgumentException>(() => il.Insert(0, bogus));`
`281`	`282`	`}`
`282`	`283`	`}`
`283`	`284`