Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Commit 8b7bdf6

Browse files
stephentoubstephentoub
committed
Merge pull request #2004 from stephentoub/hashcore_checks
Fix potential AVs from poorly written HashAlgorithm usage
2 parents baaa017 + e3bae9f commit 8b7bdf6

File tree

7 files changed

+240
-26
lines changed

7 files changed

+240
-26
lines changed

‎src/System.Security.Cryptography.Hashing.Algorithms/src/Internal/Cryptography/HashProvider.cs

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,28 @@ namespace Internal.Cryptography
1212
internal abstract class HashProvider
1313
{
1414
// Adds new data to be hashed. This can be called repeatedly in order to hash data from incontiguous sources.
15-
public abstract void AppendHashData(byte[] data, int offset, int count);
15+
public void AppendHashData(byte[] data, int offset, int count)
16+
{
17+
// AppendHashData can be called via exposed APIs (e.g. a type that derives from
18+
// HMACSHA1 and calls HashCore) and could be passed bad data from there. It could
19+
// also receive a bad count from HashAlgorithm reading from a Stream that returns
20+
// an invalid number of bytes read. Since our implementations of AppendHashDataCore
21+
// end up using unsafe code, we want to be sure the arguments are valid.
22+
if (data == null)
23+
throw new ArgumentNullException("data", SR.ArgumentNull_Buffer);
24+
if (offset < 0)
25+
throw new ArgumentOutOfRangeException("offset", SR.ArgumentOutOfRange_NeedNonNegNum);
26+
if (count < 0)
27+
throw new ArgumentOutOfRangeException("count", SR.ArgumentOutOfRange_NeedNonNegNum);
28+
if (data.Length - offset < count)
29+
throw new ArgumentException(SR.Argument_InvalidOffLen);
30+
31+
AppendHashDataCore(data, offset, count);
32+
}
33+
34+
// Adds new data to be hashed. This can be called repeatedly in order to hash data from incontiguous sources.
35+
// Argument validation is handled by AppendHashData.
36+
public abstract void AppendHashDataCore(byte[] data, int offset, int count);
1637

1738
// Compute the hash based on the appended data and resets the HashProvider for more hashing.
1839
public abstract byte[] FinalizeHashAndReset();
@@ -25,5 +46,3 @@ internal abstract class HashProvider
2546
}
2647
}
2748

28-
29-

‎src/System.Security.Cryptography.Hashing.Algorithms/src/Internal/Cryptography/HashProviderDispenser.Unix.cs

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -75,12 +75,8 @@ public EvpHashProvider(IntPtr algorithmEvp)
7575
Check(Interop.libcrypto.EVP_DigestInit_ex(_ctx, algorithmEvp, IntPtr.Zero));
7676
}
7777

78-
public sealed override unsafe void AppendHashData(byte[] data, int offset, int count)
78+
public sealed override unsafe void AppendHashDataCore(byte[] data, int offset, int count)
7979
{
80-
Debug.Assert(data != null);
81-
Debug.Assert(offset >= 0 && count >= 0);
82-
Debug.Assert((offset + count) >= 0 && (offset + count) <= data.Length);
83-
8480
fixed (byte* md = data)
8581
{
8682
Check(Interop.libcrypto.EVP_DigestUpdate(_ctx, md + offset, (IntPtr)count));
@@ -135,12 +131,8 @@ public unsafe HmacHashProvider(IntPtr algorithmEvp, byte[] key)
135131
}
136132
}
137133

138-
public sealed override unsafe void AppendHashData(byte[] data, int offset, int count)
134+
public sealed override unsafe void AppendHashDataCore(byte[] data, int offset, int count)
139135
{
140-
Debug.Assert(data != null);
141-
Debug.Assert(offset >= 0 && count >= 0);
142-
Debug.Assert((offset + count) >= 0 && (offset + count) <= data.Length);
143-
144136
fixed (byte* md = data)
145137
{
146138
Check(Interop.libcrypto.HMAC_Update(ref _hmacCtx, md + offset, count));

‎src/System.Security.Cryptography.Hashing.Algorithms/src/Internal/Cryptography/HashProviderDispenser.Windows.cs

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -61,14 +61,11 @@ public HashProviderCng(String hashAlgId, byte[] key)
6161
return;
6262
}
6363

64-
public sealed override void AppendHashData(byte[] rgb, int ib, int cb)
64+
public sealed override unsafe void AppendHashDataCore(byte[] data, int offset, int count)
6565
{
66-
unsafe
66+
fixed (byte* pRgb = data)
6767
{
68-
fixed (byte* pRgb = rgb)
69-
{
70-
_hHash.BCryptHashData(pRgb + ib, cb);
71-
}
68+
_hHash.BCryptHashData(pRgb + offset, count);
7269
}
7370
}
7471

‎src/System.Security.Cryptography.Hashing.Algorithms/src/Resources/Strings.resx

Lines changed: 129 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,129 @@
1+
<?xml version="1.0" encoding="utf-8"?>
2+
<root>
3+
<!--
4+
Microsoft ResX Schema
5+
6+
Version 2.0
7+
8+
The primary goals of this format is to allow a simple XML format
9+
that is mostly human readable. The generation and parsing of the
10+
various data types are done through the TypeConverter classes
11+
associated with the data types.
12+
13+
Example:
14+
15+
... ado.net/XML headers & schema ...
16+
<resheader name="resmimetype">text/microsoft-resx</resheader>
17+
<resheader name="version">2.0</resheader>
18+
<resheader name="reader">System.Resources.ResXResourceReader, System.Windows.Forms, ...</resheader>
19+
<resheader name="writer">System.Resources.ResXResourceWriter, System.Windows.Forms, ...</resheader>
20+
<data name="Name1"><value>this is my long string</value><comment>this is a comment</comment></data>
21+
<data name="Color1" type="System.Drawing.Color, System.Drawing">Blue</data>
22+
<data name="Bitmap1" mimetype="application/x-microsoft.net.object.binary.base64">
23+
<value>[base64 mime encoded serialized .NET Framework object]</value>
24+
</data>
25+
<data name="Icon1" type="System.Drawing.Icon, System.Drawing" mimetype="application/x-microsoft.net.object.bytearray.base64">
26+
<value>[base64 mime encoded string representing a byte array form of the .NET Framework object]</value>
27+
<comment>This is a comment</comment>
28+
</data>
29+
30+
There are any number of "resheader" rows that contain simple
31+
name/value pairs.
32+
33+
Each data row contains a name, and value. The row also contains a
34+
type or mimetype. Type corresponds to a .NET class that support
35+
text/value conversion through the TypeConverter architecture.
36+
Classes that don't support this are serialized and stored with the
37+
mimetype set.
38+
39+
The mimetype is used for serialized objects, and tells the
40+
ResXResourceReader how to depersist the object. This is currently not
41+
extensible. For a given mimetype the value must be set accordingly:
42+
43+
Note - application/x-microsoft.net.object.binary.base64 is the format
44+
that the ResXResourceWriter will generate, however the reader can
45+
read any of the formats listed below.
46+
47+
mimetype: application/x-microsoft.net.object.binary.base64
48+
value : The object must be serialized with
49+
: System.Runtime.Serialization.Formatters.Binary.BinaryFormatter
50+
: and then encoded with base64 encoding.
51+
52+
mimetype: application/x-microsoft.net.object.soap.base64
53+
value : The object must be serialized with
54+
: System.Runtime.Serialization.Formatters.Soap.SoapFormatter
55+
: and then encoded with base64 encoding.
56+
57+
mimetype: application/x-microsoft.net.object.bytearray.base64
58+
value : The object must be serialized into a byte array
59+
: using a System.ComponentModel.TypeConverter
60+
: and then encoded with base64 encoding.
61+
-->
62+
<xsd:schema id="root" xmlns="" xmlns:xsd="http://www.w3.org/2001/XMLSchema" xmlns:msdata="urn:schemas-microsoft-com:xml-msdata">
63+
<xsd:import namespace="http://www.w3.org/XML/1998/namespace" />
64+
<xsd:element name="root" msdata:IsDataSet="true">
65+
<xsd:complexType>
66+
<xsd:choice maxOccurs="unbounded">
67+
<xsd:element name="metadata">
68+
<xsd:complexType>
69+
<xsd:sequence>
70+
<xsd:element name="value" type="xsd:string" minOccurs="0" />
71+
</xsd:sequence>
72+
<xsd:attribute name="name" use="required" type="xsd:string" />
73+
<xsd:attribute name="type" type="xsd:string" />
74+
<xsd:attribute name="mimetype" type="xsd:string" />
75+
<xsd:attribute ref="xml:space" />
76+
</xsd:complexType>
77+
</xsd:element>
78+
<xsd:element name="assembly">
79+
<xsd:complexType>
80+
<xsd:attribute name="alias" type="xsd:string" />
81+
<xsd:attribute name="name" type="xsd:string" />
82+
</xsd:complexType>
83+
</xsd:element>
84+
<xsd:element name="data">
85+
<xsd:complexType>
86+
<xsd:sequence>
87+
<xsd:element name="value" type="xsd:string" minOccurs="0" msdata:Ordinal="1" />
88+
<xsd:element name="comment" type="xsd:string" minOccurs="0" msdata:Ordinal="2" />
89+
</xsd:sequence>
90+
<xsd:attribute name="name" type="xsd:string" use="required" msdata:Ordinal="1" />
91+
<xsd:attribute name="type" type="xsd:string" msdata:Ordinal="3" />
92+
<xsd:attribute name="mimetype" type="xsd:string" msdata:Ordinal="4" />
93+
<xsd:attribute ref="xml:space" />
94+
</xsd:complexType>
95+
</xsd:element>
96+
<xsd:element name="resheader">
97+
<xsd:complexType>
98+
<xsd:sequence>
99+
<xsd:element name="value" type="xsd:string" minOccurs="0" msdata:Ordinal="1" />
100+
</xsd:sequence>
101+
<xsd:attribute name="name" type="xsd:string" use="required" />
102+
</xsd:complexType>
103+
</xsd:element>
104+
</xsd:choice>
105+
</xsd:complexType>
106+
</xsd:element>
107+
</xsd:schema>
108+
<resheader name="resmimetype">
109+
<value>text/microsoft-resx</value>
110+
</resheader>
111+
<resheader name="version">
112+
<value>2.0</value>
113+
</resheader>
114+
<resheader name="reader">
115+
<value>System.Resources.ResXResourceReader, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089</value>
116+
</resheader>
117+
<resheader name="writer">
118+
<value>System.Resources.ResXResourceWriter, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089</value>
119+
</resheader>
120+
<data name="ArgumentOutOfRange_NeedNonNegNum" xml:space="preserve">
121+
<value>Non-negative number required.</value>
122+
</data>
123+
<data name="Argument_InvalidOffLen" xml:space="preserve">
124+
<value>Offset and length were out of bounds for the array or count is greater than the number of elements from index to the end of the source collection.</value>
125+
</data>
126+
<data name="ArgumentNull_Buffer" xml:space="preserve">
127+
<value>Buffer cannot be null.</value>
128+
</data>
129+
</root>

‎src/System.Security.Cryptography.Hashing.Algorithms/tests/InvalidUsageTests.cs

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,80 @@
1+
// Copyright (c) Microsoft. All rights reserved.
2+
// Licensed under the MIT license. See LICENSE file in the project root for full license information.
3+
4+
using System.IO;
5+
using Xunit;
6+
7+
namespace System.Security.Cryptography.Hashing.Algorithms.Tests
8+
{
9+
public class InvalidUsageTests
10+
{
11+
[Fact]
12+
public void InvalidHashCoreArgumentsFromDerivedType()
13+
{
14+
using (var hmac = new DerivedHMACSHA1())
15+
{
16+
Assert.Throws<ArgumentNullException>(() => hmac.ExposedHashCore(null, 0, 0));
17+
Assert.Throws<ArgumentOutOfRangeException>(() => hmac.ExposedHashCore(new byte[1], -1, 1));
18+
Assert.Throws<ArgumentOutOfRangeException>(() => hmac.ExposedHashCore(new byte[1], 0, -1));
19+
Assert.Throws<ArgumentException>(() => hmac.ExposedHashCore(new byte[1], 0, 2));
20+
Assert.Throws<ArgumentException>(() => hmac.ExposedHashCore(new byte[2], 1, 2));
21+
Assert.Throws<ArgumentException>(() => hmac.ExposedHashCore(new byte[1], Int32.MaxValue, Int32.MaxValue));
22+
}
23+
}
24+
25+
[Fact]
26+
public void InvalidHashCoreArgumentsFromStream()
27+
{
28+
using (SHA1 sha1 = SHA1.Create())
29+
{
30+
Assert.Throws<ArgumentException>(
31+
() => sha1.ComputeHash(new BadReadStream(BadReadStream.ErrorCondition.TooLargeValueFromRead)));
32+
sha1.ComputeHash(new BadReadStream(BadReadStream.ErrorCondition.NegativeValueFromRead));
33+
}
34+
}
35+
36+
private sealed class DerivedHMACSHA1 : HMACSHA1
37+
{
38+
public void ExposedHashCore(byte[] rgb, int ib, int cb)
39+
{
40+
HashCore(rgb, ib, cb);
41+
}
42+
}
43+
44+
private sealed class BadReadStream : Stream
45+
{
46+
internal enum ErrorCondition
47+
{
48+
NegativeValueFromRead,
49+
TooLargeValueFromRead
50+
}
51+
52+
private readonly ErrorCondition _condition;
53+
54+
public BadReadStream(ErrorCondition condition)
55+
{
56+
_condition = condition;
57+
}
58+
59+
public override int Read(byte[] buffer, int offset, int count)
60+
{
61+
switch (_condition)
62+
{
63+
case ErrorCondition.NegativeValueFromRead: return -1;
64+
case ErrorCondition.TooLargeValueFromRead: return buffer.Length + 1;
65+
default: return 0;
66+
}
67+
}
68+
69+
public override bool CanRead { get { return true; } }
70+
public override bool CanSeek { get { return false; } }
71+
public override bool CanWrite { get { return false; } }
72+
public override long Length { get { throw new NotSupportedException(); } }
73+
public override long Position { get { throw new NotSupportedException(); } set { throw new NotSupportedException(); } }
74+
public override void Flush() { }
75+
public override long Seek(long offset, SeekOrigin origin) { throw new NotSupportedException(); }
76+
public override void SetLength(long value) { throw new NotSupportedException(); }
77+
public override void Write(byte[] buffer, int offset, int count) { throw new NotSupportedException(); }
78+
}
79+
}
80+
}

‎src/System.Security.Cryptography.Hashing.Algorithms/tests/System.Security.Cryptography.Hashing.Algorithms.Tests.csproj

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
<ItemGroup>
2323
<Compile Include="ByteUtils.cs" />
2424
<Compile Include="HashAlgorithmTest.cs" />
25+
<Compile Include="InvalidUsageTests.cs" />
2526
<Compile Include="HmacSha1Tests.cs" />
2627
<Compile Include="HmacSha256Tests.cs" />
2728
<Compile Include="HmacSha384Tests.cs" />

‎src/System.Security.Cryptography.Hashing/src/System/Security/Cryptography/HashAlgorithm.cs

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -58,14 +58,10 @@ public byte[] ComputeHash(Stream inputStream)
5858
// Default the buffer size to 4K.
5959
byte[] buffer = new byte[4096];
6060
int bytesRead;
61-
do
61+
while ((bytesRead = inputStream.Read(buffer, 0, buffer.Length)) > 0)
6262
{
63-
bytesRead = inputStream.Read(buffer, 0, 4096);
64-
if (bytesRead > 0)
65-
{
66-
HashCore(buffer, 0, bytesRead);
67-
}
68-
} while (bytesRead > 0);
63+
HashCore(buffer, 0, bytesRead);
64+
}
6965
return CaptureHashCodeAndReinitialize();
7066
}
7167

0 commit comments

Comments
 (0)