From 24d60bb782a73f6e1d0419eb764c20d4d7c955ae Mon Sep 17 00:00:00 2001 From: Peter Dettman Date: Sat, 6 Nov 2021 01:09:09 +0700 Subject: TLS: Improve ASN.1 parsing --- crypto/src/tls/CertificateRequest.cs | 6 ++-- crypto/src/tls/CertificateStatus.cs | 16 ++++++--- crypto/src/tls/OcspStatusRequest.cs | 9 +++-- crypto/src/tls/TlsExtensionsUtilities.cs | 9 +++-- crypto/src/tls/TlsUtilities.cs | 41 ++++++++++++++++++----- crypto/src/tls/TrustedAuthority.cs | 6 ++-- crypto/src/tls/crypto/impl/bc/BcTlsCertificate.cs | 3 +- 7 files changed, 67 insertions(+), 23 deletions(-) diff --git a/crypto/src/tls/CertificateRequest.cs b/crypto/src/tls/CertificateRequest.cs index 1abf01aed..8005731f4 100644 --- a/crypto/src/tls/CertificateRequest.cs +++ b/crypto/src/tls/CertificateRequest.cs @@ -263,8 +263,10 @@ namespace Org.BouncyCastle.Tls do { byte[] derEncoding = TlsUtilities.ReadOpaque16(bis, 1); - Asn1Object asn1 = TlsUtilities.ReadDerObject(derEncoding); - certificateAuthorities.Add(X509Name.GetInstance(asn1)); + Asn1Object asn1 = TlsUtilities.ReadAsn1Object(derEncoding); + X509Name ca = X509Name.GetInstance(asn1); + TlsUtilities.RequireDerEncoding(ca, derEncoding); + certificateAuthorities.Add(ca); } while (bis.Position < bis.Length); } diff --git a/crypto/src/tls/CertificateStatus.cs b/crypto/src/tls/CertificateStatus.cs index 61f4336a8..cbf5600f6 100644 --- a/crypto/src/tls/CertificateStatus.cs +++ b/crypto/src/tls/CertificateStatus.cs @@ -137,8 +137,7 @@ namespace Org.BouncyCastle.Tls RequireStatusRequestVersion(1, statusRequestVersion); byte[] derEncoding = TlsUtilities.ReadOpaque24(input, 1); - Asn1Object derObject = TlsUtilities.ReadDerObject(derEncoding); - response = OcspResponse.GetInstance(derObject); + response = ParseOcspResponse(derEncoding); break; } case CertificateStatusType.ocsp_multi: @@ -162,9 +161,7 @@ namespace Org.BouncyCastle.Tls else { byte[] derEncoding = TlsUtilities.ReadFully(length, buf); - Asn1Object derObject = TlsUtilities.ReadDerObject(derEncoding); - OcspResponse ocspResponse = OcspResponse.GetInstance(derObject); - ocspResponseList.Add(ocspResponse); + ocspResponseList.Add(ParseOcspResponse(derEncoding)); } } @@ -210,6 +207,15 @@ namespace Org.BouncyCastle.Tls return true; } + /// + private static OcspResponse ParseOcspResponse(byte[] derEncoding) + { + Asn1Object asn1 = TlsUtilities.ReadAsn1Object(derEncoding); + OcspResponse ocspResponse = OcspResponse.GetInstance(asn1); + TlsUtilities.RequireDerEncoding(ocspResponse, derEncoding); + return ocspResponse; + } + /// private static void RequireStatusRequestVersion(int minVersion, int statusRequestVersion) { diff --git a/crypto/src/tls/OcspStatusRequest.cs b/crypto/src/tls/OcspStatusRequest.cs index b52517e06..8679022ec 100644 --- a/crypto/src/tls/OcspStatusRequest.cs +++ b/crypto/src/tls/OcspStatusRequest.cs @@ -89,7 +89,9 @@ namespace Org.BouncyCastle.Tls do { byte[] derEncoding = TlsUtilities.ReadOpaque16(buf, 1); - ResponderID responderID = ResponderID.GetInstance(TlsUtilities.ReadDerObject(derEncoding)); + Asn1Object asn1 = TlsUtilities.ReadAsn1Object(derEncoding); + ResponderID responderID = ResponderID.GetInstance(asn1); + TlsUtilities.RequireDerEncoding(responderID, derEncoding); responderIDList.Add(responderID); } while (buf.Position < buf.Length); @@ -101,7 +103,10 @@ namespace Org.BouncyCastle.Tls byte[] derEncoding = TlsUtilities.ReadOpaque16(input); if (derEncoding.Length > 0) { - requestExtensions = X509Extensions.GetInstance(TlsUtilities.ReadDerObject(derEncoding)); + Asn1Object asn1 = TlsUtilities.ReadAsn1Object(derEncoding); + X509Extensions extensions = X509Extensions.GetInstance(asn1); + TlsUtilities.RequireDerEncoding(extensions, derEncoding); + requestExtensions = extensions; } } diff --git a/crypto/src/tls/TlsExtensionsUtilities.cs b/crypto/src/tls/TlsExtensionsUtilities.cs index 688fee3c7..5a13d8d2e 100644 --- a/crypto/src/tls/TlsExtensionsUtilities.cs +++ b/crypto/src/tls/TlsExtensionsUtilities.cs @@ -964,8 +964,10 @@ namespace Org.BouncyCastle.Tls while (buf.Position < buf.Length) { byte[] derEncoding = TlsUtilities.ReadOpaque16(buf, 1); - Asn1Object asn1 = TlsUtilities.ReadDerObject(derEncoding); - authorities.Add(X509Name.GetInstance(asn1)); + Asn1Object asn1 = TlsUtilities.ReadAsn1Object(derEncoding); + X509Name ca = X509Name.GetInstance(asn1); + TlsUtilities.RequireDerEncoding(ca, derEncoding); + authorities.Add(ca); } return authorities; } @@ -1111,8 +1113,9 @@ namespace Org.BouncyCastle.Tls while (buf.Position < buf.Length) { byte[] derEncoding = TlsUtilities.ReadOpaque8(buf, 1); - Asn1Object asn1 = TlsUtilities.ReadDerObject(derEncoding); + Asn1Object asn1 = TlsUtilities.ReadAsn1Object(derEncoding); DerObjectIdentifier certificateExtensionOid = DerObjectIdentifier.GetInstance(asn1); + TlsUtilities.RequireDerEncoding(certificateExtensionOid, derEncoding); if (filters.Contains(certificateExtensionOid)) throw new TlsFatalAlert(AlertDescription.illegal_parameter); diff --git a/crypto/src/tls/TlsUtilities.cs b/crypto/src/tls/TlsUtilities.cs index d1e046965..8733fd68f 100644 --- a/crypto/src/tls/TlsUtilities.cs +++ b/crypto/src/tls/TlsUtilities.cs @@ -15,6 +15,7 @@ using Org.BouncyCastle.Asn1.Pkcs; using Org.BouncyCastle.Asn1.Rosstandart; using Org.BouncyCastle.Asn1.X509; using Org.BouncyCastle.Asn1.X9; +using Org.BouncyCastle.Math; using Org.BouncyCastle.Tls.Crypto; using Org.BouncyCastle.Utilities; using Org.BouncyCastle.Utilities.Date; @@ -901,6 +902,8 @@ namespace Org.BouncyCastle.Tls return result; } + /// + [Obsolete("Will be removed. Use ReadAsn1Object in combination with RequireDerEncoding instead")] public static Asn1Object ReadDerObject(byte[] encoding) { /* @@ -908,11 +911,20 @@ namespace Org.BouncyCastle.Tls * canonical, we can check it by re-encoding the result and comparing to the original. */ Asn1Object result = ReadAsn1Object(encoding); - byte[] check = result.GetEncoded(Asn1Encodable.Der); + RequireDerEncoding(result, encoding); + return result; + } + + /// + public static void RequireDerEncoding(Asn1Encodable asn1, byte[] encoding) + { + /* + * NOTE: The current ASN.1 parsing code can't enforce DER-only parsing, but since DER is + * canonical, we can check it by re-encoding the result and comparing to the original. + */ + byte[] check = asn1.GetEncoded(Asn1Encodable.Der); if (!Arrays.AreEqual(check, encoding)) throw new TlsFatalAlert(AlertDescription.decode_error); - - return result; } public static void WriteGmtUnixTime(byte[] buf, int offset) @@ -4479,13 +4491,26 @@ namespace Org.BouncyCastle.Tls byte[] tlsFeatures = serverCertificate.GetCertificateAt(0).GetExtension(TlsObjectIdentifiers.id_pe_tlsfeature); if (tlsFeatures != null) { - foreach (DerInteger tlsExtension in Asn1Sequence.GetInstance(ReadDerObject(tlsFeatures))) + // TODO[tls] Proper ASN.1 type class for this extension? + Asn1Sequence tlsFeaturesSeq = (Asn1Sequence)ReadAsn1Object(tlsFeatures); + for (int i = 0; i < tlsFeaturesSeq.Count; ++i) { - int extensionType = tlsExtension.IntValueExact; - CheckUint16(extensionType); + if (!(tlsFeaturesSeq[i] is DerInteger)) + throw new TlsFatalAlert(AlertDescription.bad_certificate); + } + + RequireDerEncoding(tlsFeaturesSeq, tlsFeatures); - if (clientExtensions.Contains(extensionType) && !serverExtensions.Contains(extensionType)) - throw new TlsFatalAlert(AlertDescription.certificate_unknown); + foreach (DerInteger tlsExtensionElement in tlsFeaturesSeq) + { + BigInteger tlsExtension = tlsExtensionElement.PositiveValue; + if (tlsExtension.BitLength <= 16) + { + int extensionType = tlsExtension.IntValueExact; + + if (clientExtensions.Contains(extensionType) && !serverExtensions.Contains(extensionType)) + throw new TlsFatalAlert(AlertDescription.certificate_unknown); + } } } } diff --git a/crypto/src/tls/TrustedAuthority.cs b/crypto/src/tls/TrustedAuthority.cs index cd564ebfa..b82666450 100644 --- a/crypto/src/tls/TrustedAuthority.cs +++ b/crypto/src/tls/TrustedAuthority.cs @@ -107,8 +107,10 @@ namespace Org.BouncyCastle.Tls case Tls.IdentifierType.x509_name: { byte[] derEncoding = TlsUtilities.ReadOpaque16(input, 1); - Asn1Object asn1 = TlsUtilities.ReadDerObject(derEncoding); - identifier = X509Name.GetInstance(asn1); + Asn1Object asn1 = TlsUtilities.ReadAsn1Object(derEncoding); + X509Name x509Name = X509Name.GetInstance(asn1); + TlsUtilities.RequireDerEncoding(x509Name, derEncoding); + identifier = x509Name; break; } default: diff --git a/crypto/src/tls/crypto/impl/bc/BcTlsCertificate.cs b/crypto/src/tls/crypto/impl/bc/BcTlsCertificate.cs index 9d4157050..d01bd5c51 100644 --- a/crypto/src/tls/crypto/impl/bc/BcTlsCertificate.cs +++ b/crypto/src/tls/crypto/impl/bc/BcTlsCertificate.cs @@ -29,7 +29,8 @@ namespace Org.BouncyCastle.Tls.Crypto.Impl.BC { try { - return X509CertificateStructure.GetInstance(encoding); + Asn1Object asn1 = TlsUtilities.ReadAsn1Object(encoding); + return X509CertificateStructure.GetInstance(asn1); } catch (Exception e) { -- cgit 1.4.1