From 2ce1f9b33c1476ed9e76f98c1f8ff5570d90f7db Mon Sep 17 00:00:00 2001 From: Jeremy Barton Date: Mon, 24 Jun 2019 10:44:37 -0700 Subject: [PATCH] Fix overread when decoding X509KeyUsage extension on Windows CryptDecodeObject on X509_KEY_USAGE always sets cbStructInfo to 32 (or 20 on x86), reporting 8 bytes (both architectures) more than needed for the CRYPT_BIT_BLOB structure (0 bytes more than the structure when transporting pbData=NULL). However, that value is over-representative of the number of bytes actually written (modified) in this case. The CRYPT_BIT_BLOB contains a cbData saying how long the actual payload is (0 (pbData==NULL), 1, or 2 bytes), but DecodeX509KeyUsageExtension didn't read it, just blindly read the value as a DWORD, implicitly counting on the zero-init of arrays (and Win32's choice of values combined with Little Endian layout). By respecting the decoded cbData value (and not over-reading) the helper no longer needs to manually clear the stackalloc. Commit migrated from https://github.com/dotnet/corefx/commit/0a05173f98493547d40b1865808481e76ede1212 --- .../Pal.Windows/Native/Helpers.cs | 88 ++++++++++++++++--- .../Pal.Windows/X509Pal.CustomExtensions.cs | 16 +++- .../tests/ExtensionsTests.cs | 9 ++ 3 files changed, 99 insertions(+), 14 deletions(-) diff --git a/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Windows/Native/Helpers.cs b/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Windows/Native/Helpers.cs index 687a274952c..62d8be40b09 100644 --- a/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Windows/Native/Helpers.cs +++ b/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Windows/Native/Helpers.cs @@ -75,58 +75,122 @@ namespace Internal.Cryptography.Pal.Native public unsafe delegate void DecodedObjectReceiver(void* pvDecodedObject, int cbDecodedObject); - public static void DecodeObject(this byte[] encoded, CryptDecodeObjectStructType lpszStructType, DecodedObjectReceiver receiver) + public static void DecodeObject( + this byte[] encoded, + CryptDecodeObjectStructType lpszStructType, + DecodedObjectReceiver receiver) { unsafe { int cb = 0; - if (!Interop.crypt32.CryptDecodeObjectPointer(CertEncodingType.All, lpszStructType, encoded, encoded.Length, CryptDecodeObjectFlags.None, null, ref cb)) + if (!Interop.crypt32.CryptDecodeObjectPointer( + CertEncodingType.All, + lpszStructType, + encoded, + encoded.Length, + CryptDecodeObjectFlags.None, + null, + ref cb)) + { throw Marshal.GetLastWin32Error().ToCryptographicException(); + } byte* decoded = stackalloc byte[cb]; - new Span(decoded, cb).Clear(); - if (!Interop.crypt32.CryptDecodeObjectPointer(CertEncodingType.All, lpszStructType, encoded, encoded.Length, CryptDecodeObjectFlags.None, (byte*)decoded, ref cb)) + + if (!Interop.crypt32.CryptDecodeObjectPointer( + CertEncodingType.All, + lpszStructType, + encoded, + encoded.Length, + CryptDecodeObjectFlags.None, + decoded, + ref cb)) + { throw Marshal.GetLastWin32Error().ToCryptographicException(); + } receiver(decoded, cb); } } - public static void DecodeObject(this byte[] encoded, string lpszStructType, DecodedObjectReceiver receiver) + public static void DecodeObject( + this byte[] encoded, + string lpszStructType, + DecodedObjectReceiver receiver) { unsafe { int cb = 0; - if (!Interop.crypt32.CryptDecodeObjectPointer(CertEncodingType.All, lpszStructType, encoded, encoded.Length, CryptDecodeObjectFlags.None, null, ref cb)) + if (!Interop.crypt32.CryptDecodeObjectPointer( + CertEncodingType.All, + lpszStructType, + encoded, + encoded.Length, + CryptDecodeObjectFlags.None, + null, + ref cb)) + { throw Marshal.GetLastWin32Error().ToCryptographicException(); + } byte* decoded = stackalloc byte[cb]; - new Span(decoded, cb).Clear(); - if (!Interop.crypt32.CryptDecodeObjectPointer(CertEncodingType.All, lpszStructType, encoded, encoded.Length, CryptDecodeObjectFlags.None, (byte*)decoded, ref cb)) + + if (!Interop.crypt32.CryptDecodeObjectPointer( + CertEncodingType.All, + lpszStructType, + encoded, + encoded.Length, + CryptDecodeObjectFlags.None, + decoded, + ref cb)) + { throw Marshal.GetLastWin32Error().ToCryptographicException(); + } receiver(decoded, cb); } } - public static bool DecodeObjectNoThrow(this byte[] encoded, CryptDecodeObjectStructType lpszStructType, DecodedObjectReceiver receiver) + public static bool DecodeObjectNoThrow( + this byte[] encoded, + CryptDecodeObjectStructType lpszStructType, + DecodedObjectReceiver receiver) { unsafe { int cb = 0; - if (!Interop.crypt32.CryptDecodeObjectPointer(CertEncodingType.All, lpszStructType, encoded, encoded.Length, CryptDecodeObjectFlags.None, null, ref cb)) + if (!Interop.crypt32.CryptDecodeObjectPointer( + CertEncodingType.All, + lpszStructType, + encoded, + encoded.Length, + CryptDecodeObjectFlags.None, + null, + ref cb)) + { return false; + } byte* decoded = stackalloc byte[cb]; - new Span(decoded, cb).Clear(); - if (!Interop.crypt32.CryptDecodeObjectPointer(CertEncodingType.All, lpszStructType, encoded, encoded.Length, CryptDecodeObjectFlags.None, (byte*)decoded, ref cb)) + + if (!Interop.crypt32.CryptDecodeObjectPointer( + CertEncodingType.All, + lpszStructType, + encoded, + encoded.Length, + CryptDecodeObjectFlags.None, + decoded, + ref cb)) + { return false; + } receiver(decoded, cb); } + return true; } } diff --git a/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Windows/X509Pal.CustomExtensions.cs b/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Windows/X509Pal.CustomExtensions.cs index 6f3d1837a3d..9f835f54003 100644 --- a/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Windows/X509Pal.CustomExtensions.cs +++ b/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Windows/X509Pal.CustomExtensions.cs @@ -50,9 +50,21 @@ namespace Internal.Cryptography.Pal Debug.Assert(cbDecoded >= sizeof(CRYPT_BIT_BLOB)); CRYPT_BIT_BLOB* pBlob = (CRYPT_BIT_BLOB*)pvDecoded; keyUsagesAsUint = 0; - if (pBlob->pbData != null) + byte* pbData = pBlob->pbData; + + if (pbData != null) { - keyUsagesAsUint = *(uint*)(pBlob->pbData); + Debug.Assert((uint)pBlob->cbData < 3, "Unexpected length for X509_KEY_USAGE data"); + + switch (pBlob->cbData) + { + case 1: + keyUsagesAsUint = *pbData; + break; + case 2: + keyUsagesAsUint = *(ushort*)(pbData); + break; + } } } ); diff --git a/src/libraries/System.Security.Cryptography.X509Certificates/tests/ExtensionsTests.cs b/src/libraries/System.Security.Cryptography.X509Certificates/tests/ExtensionsTests.cs index 788d708dcae..f286a15c932 100644 --- a/src/libraries/System.Security.Cryptography.X509Certificates/tests/ExtensionsTests.cs +++ b/src/libraries/System.Security.Cryptography.X509Certificates/tests/ExtensionsTests.cs @@ -189,6 +189,15 @@ namespace System.Security.Cryptography.X509Certificates.Tests TestKeyUsageExtension(X509KeyUsageFlags.NonRepudiation, false, "03020640".HexToByteArray()); } + [Fact] + public static void KeyUsageExtension_KeyAgreementAndDecipherOnly() + { + TestKeyUsageExtension( + X509KeyUsageFlags.KeyAgreement | X509KeyUsageFlags.DecipherOnly, + false, + "0303070880".HexToByteArray()); + } + [Fact] public static void KeyUsageExtension_BER() { -- 2.34.1