Skip to content

Commit

Permalink
Patch #1 for 2.3
Browse files Browse the repository at this point in the history 
- TLS: fix timing side-channel for RSA key exchange
- fix method Write(ReadOnlySpan<byte>) in LimitedBuffer
- ASN.1: Limit OID contents to 4096 bytes
- EdDSA: fix verification infinite loop
- EC: restrict m value in F2m curves
  • Loading branch information
@peterdettman
peterdettman committed May 7, 2024
1 parent cf1829b commit 45c6b99
Show file tree
Hide file tree
Showing 16 changed files with 2,456 additions and 232 deletions.
6 changes: 3 additions & 3 deletions crypto/Contributors.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -268,9 +268,9 @@ <h3>Code Contributors:</h3>
<li>
<p>Matthew Sitton (https://github.com/mdsitton) - Addition of missing ALPN Protocol names.</p>
</li>
<li>
<p>Jozef Gajdo&scaron; (https://github.com/harrison314) - Time constructor optimization, RevokedStatus fix, improved thread-safe singleton code (e.g. X509Certificate/X509Crl cached encoding), SubjectPublicKeyInfo support in OpenSsl.PemWriter.</p>
</li>
<li>
<p>Jozef Gajdo&scaron; (https://github.com/harrison314) - Time constructor optimization, RevokedStatus fix, improved thread-safe singleton code (e.g. X509Certificate/X509Crl cached encoding), SubjectPublicKeyInfo support in OpenSsl.PemWriter, fixed PSS raw signing over spans.</p>
</li>
<li>
<p>Ben Adams (https://github.com/benaadams) - Performance optimization for AES-NI.</p>
</li>
Expand Down
23 changes: 23 additions & 0 deletions crypto/Readme.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ <h3><a class="mozTocH3" name="mozTocId685176"></a>Contents:<br /></h3>
<li>
<a href="#mozTocId3413">Notes:</a>
<ol>
<li>
<a href="#mozTocId85332">Release 2.3.1</a>
<li>
<a href="#mozTocId85331">Release 2.3.0</a>
<li>
Expand Down Expand Up @@ -329,6 +331,27 @@ <h3><a class="mozTocH3" name="mozTocId358608"></a>For first time users.</h3>
<hr style="WIDTH: 100%; HEIGHT: 2px">
<h3><a class="mozTocH3" name="mozTocId3413"></a>Notes:</h3>

<h4><a class="mozTocH4" name="mozTocId85332"></a>Release 2.3.1, Tuesday May 7, 2024</h4>
<h5>Defects Fixed</h5>
<ul>
<li>TLS: Fixed timing side-channel for RSA key exchange ("The Marvin Attack").</li>
<li>PSS: Fixed regression in 2.3.0 when updating signer from a span.</li>
<li>EdDSA: Fixed verification infinite loop (regression in 2.1.0)
- see <a href="https://github.com/bcgit/bc-java/issues/1599">corresponding bc-java issue</a>.</li>
</ul>
<h5>Additional Features and Functionality</h5>
<ul>
<li>ASN.1: Limited OID contents to 4096 bytes.</li>
<li>EC: Restricted m value in F2m curves.</li>
</ul>
<h5>Additional Notes</h5>
<ul>
<li>
See the (cumulative) list of GitHub pull requests that we have accepted at
<a href="https://github.com/bcgit/bc-csharp/pulls?q=is%3Apr+is%3Aclosed">bcgit/bc-csharp</a>.
</li>
</ul>

<h4><a class="mozTocH4" name="mozTocId85331"></a>Release 2.3.0, Monday February 5, 2024</h4>
<h5>Defects Fixed</h5>
<ul>
Expand Down
11 changes: 9 additions & 2 deletions crypto/src/asn1/Asn1InputStream.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -377,7 +377,9 @@ internal static Asn1Object CreatePrimitiveDerObject(int tagNo, DefiniteLengthInp
switch (tagNo)
{
case Asn1Tags.BmpString:
{
return CreateDerBmpString(defIn);
}
case Asn1Tags.Boolean:
{
GetBuffer(defIn, tmpBuffers, out var contents);
Expand All @@ -390,9 +392,16 @@ internal static Asn1Object CreatePrimitiveDerObject(int tagNo, DefiniteLengthInp
}
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);
}
}

byte[] bytes = defIn.ToArray();
Expand Down Expand Up @@ -421,8 +430,6 @@ internal static Asn1Object CreatePrimitiveDerObject(int tagNo, DefiniteLengthInp
return Asn1OctetString.CreatePrimitive(bytes);
case Asn1Tags.PrintableString:
return DerPrintableString.CreatePrimitive(bytes);
case Asn1Tags.RelativeOid:
return Asn1RelativeOid.CreatePrimitive(bytes, false);
case Asn1Tags.T61String:
return DerT61String.CreatePrimitive(bytes);
case Asn1Tags.UniversalString:
Expand Down
84 changes: 54 additions & 30 deletions crypto/src/asn1/Asn1RelativeOid.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,16 @@ internal override Asn1Object FromImplicitPrimitive(DerOctetString octetString)
}
}

/// <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)
Expand Down Expand Up @@ -68,14 +78,18 @@ public static bool TryFromID(string identifier, out Asn1RelativeOid oid)
{
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;
Expand All @@ -85,31 +99,13 @@ public static bool TryFromID(string identifier, out Asn1RelativeOid oid)

public Asn1RelativeOid(string identifier)
{
if (identifier == null)
throw new ArgumentNullException("identifier");
if (!IsValidIdentifier(identifier, 0))
throw new FormatException("string " + identifier + " not a relative OID");

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;
}
CheckIdentifier(identifier);

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)
Expand All @@ -120,7 +116,14 @@ private Asn1RelativeOid(byte[] contents, string identifier)

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()
Expand Down Expand Up @@ -165,9 +168,30 @@ internal sealed override DerEncoding GetEncodingDerImplicit(int tagClass, int ta
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(nameof(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)
{
return new Asn1RelativeOid(contents, clone);
CheckContentsLength(contents.Length);

if (!IsValidContents(contents))
throw new ArgumentException("invalid relative OID contents", nameof(contents));

return new Asn1RelativeOid(clone ? Arrays.Clone(contents) : contents, identifier: null);
}

internal static bool IsValidContents(byte[] contents)
Expand Down
78 changes: 53 additions & 25 deletions crypto/src/asn1/DerObjectIdentifier.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,16 @@ internal override Asn1Object FromImplicitPrimitive(DerOctetString octetString)
}
}

/// <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)
Expand Down Expand Up @@ -86,14 +96,18 @@ public static bool TryFromID(string identifier, out DerObjectIdentifier oid)
{
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;
Expand All @@ -105,22 +119,13 @@ public static bool TryFromID(string identifier, out DerObjectIdentifier oid)

public DerObjectIdentifier(string identifier)
{
if (identifier == null)
throw new ArgumentNullException("identifier");
if (!IsValidIdentifier(identifier))
throw new FormatException("string " + identifier + " not an OID");
CheckIdentifier(identifier);

m_contents = ParseIdentifier(identifier);
m_identifier = identifier;
}
byte[] contents = ParseIdentifier(identifier);
CheckContentsLength(contents.Length);

private DerObjectIdentifier(byte[] contents, bool clone)
{
if (!Asn1RelativeOid.IsValidContents(contents))
throw new ArgumentException("invalid OID contents", nameof(contents));

m_contents = clone ? Arrays.Clone(contents) : contents;
m_identifier = null;
m_contents = contents;
m_identifier = identifier;
}

private DerObjectIdentifier(byte[] contents, string identifier)
Expand All @@ -131,11 +136,13 @@ private DerObjectIdentifier(byte[] contents, string identifier)

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);
}

Expand Down Expand Up @@ -195,9 +202,27 @@ internal sealed override DerEncoding GetEncodingDerImplicit(int tagClass, int ta
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(nameof(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)
{
int index = Arrays.GetHashCode(contents);
CheckContentsLength(contents.Length);

uint index = (uint)Arrays.GetHashCode(contents);

index ^= index >> 20;
index ^= index >> 10;
Expand All @@ -207,7 +232,10 @@ internal static DerObjectIdentifier CreatePrimitive(byte[] contents, bool clone)
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)
Expand All @@ -228,7 +256,7 @@ private static bool IsValidIdentifier(string identifier)
if (first < '0' || first > '2')
return false;

if (!Asn1RelativeOid.IsValidIdentifier(identifier, 2))
if (!Asn1RelativeOid.IsValidIdentifier(identifier, from: 2))
return false;

if (first == '2')
Expand Down
Loading

0 comments on commit 45c6b99

Please sign in to comment.