Skip to content

Commit cf34ef0

Browse files
carlossanlopcarlossanlop
authored
Merge pull request #96709 from vseanreesermsft/internal-merge-8.0-2024-01-09-1136
Merging internal commits for release/8.0
2 parents ae7fac5 + bf5e279 commit cf34ef0

10 files changed

Lines changed: 202 additions & 60 deletions

File tree

‎src/libraries/Common/tests/TestUtilities/System/AssertExtensions.cs‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -397,6 +397,20 @@ public static void GreaterThanOrEqualTo<T>(T actual, T greaterThanOrEqualTo, str
397397
throw new XunitException(AddOptionalUserMessage($"Expected: {actual} to be greater than or equal to {greaterThanOrEqualTo}", userMessage));
398398
}
399399

400+
/// <summary>
401+
/// Validate that a given enum value has the expected flag set.
402+
/// </summary>
403+
/// <typeparam name="T">The enum type.</typeparam>
404+
/// <param name="expected">The flag which should be present in <paramref name="actual"/>.</param>
405+
/// <param name="actual">The value which should contain the flag <paramref name="expected"/>.</param>
406+
public static void HasFlag<T>(T expected, T actual, string userMessage = null) where T : Enum
407+
{
408+
if (!actual.HasFlag(expected))
409+
{
410+
throw new XunitException(AddOptionalUserMessage($"Expected: Value {actual} (of enum type {typeof(T).FullName}) to have the flag {expected} set.", userMessage));
411+
}
412+
}
413+
400414
// NOTE: Consider using SequenceEqual below instead, as it will give more useful information about what
401415
// the actual differences are, especially for large arrays/spans.
402416
/// <summary>

‎src/libraries/System.Security.Cryptography.Pkcs/tests/SignedCms/SignedCmsTests.netcoreapp.cs‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -399,7 +399,10 @@ public static void AddSigner_RSA_EphemeralKey()
399399
{
400400
ContentInfo content = new ContentInfo(new byte[] { 1, 2, 3 });
401401
SignedCms cms = new SignedCms(content, false);
402-
CmsSigner signer = new CmsSigner(certWithEphemeralKey);
402+
CmsSigner signer = new CmsSigner(certWithEphemeralKey)
403+
{
404+
IncludeOption = X509IncludeOption.EndCertOnly
405+
};
403406
cms.ComputeSignature(signer);
404407
}
405408
}
@@ -429,7 +432,8 @@ public static void AddSigner_DSA_EphemeralKey()
429432
SignedCms cms = new SignedCms(content, false);
430433
CmsSigner signer = new CmsSigner(certWithEphemeralKey)
431434
{
432-
DigestAlgorithm = new Oid(Oids.Sha1, Oids.Sha1)
435+
DigestAlgorithm = new Oid(Oids.Sha1, Oids.Sha1),
436+
IncludeOption = X509IncludeOption.EndCertOnly
433437
};
434438
cms.ComputeSignature(signer);
435439
}
@@ -458,7 +462,10 @@ public static void AddSigner_ECDSA_EphemeralKey()
458462
{
459463
ContentInfo content = new ContentInfo(new byte[] { 1, 2, 3 });
460464
SignedCms cms = new SignedCms(content, false);
461-
CmsSigner signer = new CmsSigner(certWithEphemeralKey);
465+
CmsSigner signer = new CmsSigner(certWithEphemeralKey)
466+
{
467+
IncludeOption = X509IncludeOption.EndCertOnly
468+
};
462469
cms.ComputeSignature(signer);
463470
}
464471
}

‎src/libraries/System.Security.Cryptography/src/Resources/Strings.resx‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -834,4 +834,7 @@
834834
<data name="Cryptography_X509_PfxWithoutPassword_ProblemFound" xml:space="preserve">
835835
<value>There was a problem with the PKCS12 (PFX) without a supplied password. See https://go.microsoft.com/fwlink/?linkid=2233907 for more information.</value>
836836
</data>
837+
<data name="Cryptography_X509_ChainBuildingFailed" xml:space="preserve">
838+
<value>An unknown chain building error occurred.</value>
839+
</data>
837840
</root>

‎src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/OidLookup.cs‎

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,8 @@ internal static partial class OidLookup
1212
private static readonly ConcurrentDictionary<string, string> s_lateBoundOidToFriendlyName =
1313
new ConcurrentDictionary<string, string>();
1414

15-
private static readonly ConcurrentDictionary<string, string?> s_lateBoundFriendlyNameToOid =
16-
new ConcurrentDictionary<string, string?>(StringComparer.OrdinalIgnoreCase);
15+
private static readonly ConcurrentDictionary<string, string> s_lateBoundFriendlyNameToOid =
16+
new ConcurrentDictionary<string, string>(StringComparer.OrdinalIgnoreCase);
1717

1818
//
1919
// Attempts to map a friendly name to an OID. Returns null if not a known name.
@@ -77,19 +77,13 @@ internal static partial class OidLookup
7777

7878
mappedOid = NativeFriendlyNameToOid(friendlyName, oidGroup, fallBackToAllGroups);
7979

80-
if (shouldUseCache)
80+
if (shouldUseCache && mappedOid != null)
8181
{
8282
s_lateBoundFriendlyNameToOid.TryAdd(friendlyName, mappedOid);
8383

8484
// Don't add the reverse here. Friendly Name => OID is a case insensitive search,
8585
// so the casing provided as input here may not be the 'correct' one. Just let
8686
// ToFriendlyName capture the response and cache it itself.
87-
88-
// Also, mappedOid could be null here if the lookup failed. Allowing storing null
89-
// means we're able to cache that a lookup failed so we don't repeat it. It's
90-
// theoretically possible, however, the failure could have been intermittent, e.g.
91-
// if the call was forced to follow an AD fallback path and the relevant servers
92-
// were offline.
9387
}
9488

9589
return mappedOid;

‎src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/LocalAppContextSwitches.cs‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,8 @@ internal static partial class LocalAppContextSwitches
1111

1212
internal static long Pkcs12UnspecifiedPasswordIterationLimit { get; } = InitializePkcs12UnspecifiedPasswordIterationLimit();
1313

14+
internal static bool X509ChainBuildThrowOnInternalError { get; } = InitializeX509ChainBuildThrowOnInternalError();
15+
1416
private static long InitializePkcs12UnspecifiedPasswordIterationLimit()
1517
{
1618
object? data = AppContext.GetData("System.Security.Cryptography.Pkcs12UnspecifiedPasswordIterationLimit");
@@ -29,5 +31,12 @@ private static long InitializePkcs12UnspecifiedPasswordIterationLimit()
2931
return DefaultPkcs12UnspecifiedPasswordIterationLimit;
3032
}
3133
}
34+
35+
private static bool InitializeX509ChainBuildThrowOnInternalError()
36+
{
37+
// n.b. the switch defaults to TRUE if it has not been explicitly set.
38+
return AppContext.TryGetSwitch("System.Security.Cryptography.ThrowOnX509ChainBuildInternalError", out bool isEnabled)
39+
? isEnabled : true;
40+
}
3241
}
3342
}

‎src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/UnixPkcs12Reader.cs‎

Lines changed: 8 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ internal abstract class UnixPkcs12Reader : IDisposable
2424
private ContentInfoAsn[]? _safeContentsValues;
2525
private CertAndKey[]? _certs;
2626
private int _certCount;
27-
private PointerMemoryManager<byte>? _tmpManager;
27+
private Memory<byte> _tmpMemory;
2828
private bool _allowDoubleBind;
2929

3030
protected abstract ICertificatePalCore ReadX509Der(ReadOnlyMemory<byte> data);
@@ -39,19 +39,13 @@ protected void ParsePkcs12(ReadOnlySpan<byte> data)
3939

4040
// Windows compatibility: Ignore trailing data.
4141
ReadOnlySpan<byte> encodedData = reader.PeekEncodedValue();
42+
byte[] dataWithoutTrailing = GC.AllocateUninitializedArray<byte>(encodedData.Length, pinned: true);
43+
encodedData.CopyTo(dataWithoutTrailing);
44+
_tmpMemory = MemoryMarshal.CreateFromPinnedArray(dataWithoutTrailing, 0, dataWithoutTrailing.Length);
4245

43-
unsafe
44-
{
45-
IntPtr tmpPtr = Marshal.AllocHGlobal(encodedData.Length);
46-
Span<byte> tmpSpan = new Span<byte>((byte*)tmpPtr, encodedData.Length);
47-
encodedData.CopyTo(tmpSpan);
48-
_tmpManager = new PointerMemoryManager<byte>((void*)tmpPtr, encodedData.Length);
49-
}
50-
51-
ReadOnlyMemory<byte> tmpMemory = _tmpManager.Memory;
52-
reader = new AsnValueReader(tmpMemory.Span, AsnEncodingRules.BER);
46+
reader = new AsnValueReader(_tmpMemory.Span, AsnEncodingRules.BER);
5347

54-
PfxAsn.Decode(ref reader, tmpMemory, out PfxAsn pfxAsn);
48+
PfxAsn.Decode(ref reader, _tmpMemory, out PfxAsn pfxAsn);
5549

5650
if (pfxAsn.AuthSafe.ContentType != Oids.Pkcs7Data)
5751
{
@@ -112,26 +106,8 @@ internal IEnumerable<CertAndKey> EnumerateAll()
112106

113107
public void Dispose()
114108
{
115-
// Generally, having a MemoryManager cleaned up in a Dispose is a bad practice.
116-
// In this case, the UnixPkcs12Reader is only ever created in a using statement,
117-
// never accessed by a second thread, and there isn't a manual call to Dispose
118-
// mixed in anywhere.
119-
if (_tmpManager != null)
120-
{
121-
unsafe
122-
{
123-
Span<byte> tmp = _tmpManager.GetSpan();
124-
CryptographicOperations.ZeroMemory(tmp);
125-
126-
fixed (byte* ptr = tmp)
127-
{
128-
Marshal.FreeHGlobal((IntPtr)ptr);
129-
}
130-
}
131-
132-
((IDisposable)_tmpManager).Dispose();
133-
_tmpManager = null;
134-
}
109+
CryptographicOperations.ZeroMemory(_tmpMemory.Span);
110+
_tmpMemory = Memory<byte>.Empty;
135111

136112
ContentInfoAsn[]? rentedContents = Interlocked.Exchange(ref _safeContentsValues, null);
137113
CertAndKey[]? rentedCerts = Interlocked.Exchange(ref _certs, null);

‎src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/X509Chain.cs‎

Lines changed: 51 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -137,26 +137,62 @@ internal bool Build(X509Certificate2 certificate, bool throwOnException)
137137
chainPolicy.UrlRetrievalTimeout,
138138
chainPolicy.DisableCertificateDownloads);
139139

140-
if (_pal == null)
141-
return false;
142-
143-
_chainElements = new X509ChainElementCollection(_pal.ChainElements!);
144-
145-
Exception? verificationException;
146-
bool? verified = _pal.Verify(chainPolicy.VerificationFlags, out verificationException);
147-
if (!verified.HasValue)
140+
bool success = false;
141+
if (_pal is not null)
148142
{
149-
if (throwOnException)
150-
{
151-
throw verificationException!;
152-
}
153-
else
143+
_chainElements = new X509ChainElementCollection(_pal.ChainElements!);
144+
145+
Exception? verificationException;
146+
bool? verified = _pal.Verify(chainPolicy.VerificationFlags, out verificationException);
147+
if (!verified.HasValue)
154148
{
155-
verified = false;
149+
if (throwOnException)
150+
{
151+
throw verificationException!;
152+
}
153+
else
154+
{
155+
verified = false;
156+
}
156157
}
158+
159+
success = verified.Value;
160+
}
161+
162+
// There are two reasons success might be false here.
163+
//
164+
// The most common reason is that we built the chain but the chain appears to run
165+
// afoul of policy. This is represented by BuildChain returning a non-null object
166+
// and storing potential policy violations in the chain structure. The public Build
167+
// method returns false to the caller, and the caller can inspect the ChainStatus
168+
// and ChainElements properties and evaluate the failure reason against app-level
169+
// policies. If the caller does not care about these policy violations, they can
170+
// choose to ignore them and to treat chain building as successful.
171+
//
172+
// The other type of failure is that BuildChain simply can't build the chain at all.
173+
// Perhaps something within the certificate is not valid or is unsupported, or perhaps
174+
// there's an internal failure within the OS layer we're invoking, etc. Whatever the
175+
// reason, we're not meaningfully able to initialize the ChainStatus property, which
176+
// means callers may observe a non-empty list of policy violations. Depending on the
177+
// caller's logic, they might incorrectly interpret this as there being no policy
178+
// violations at all, which means they might treat this condition as success.
179+
//
180+
// To avoid callers misinterpeting this latter condition as success, we'll throw an
181+
// exception, which matches general .NET API behavior when a method cannot complete
182+
// its objective. A compat switch is provided to normalize this back to a 'false'
183+
// return value for callers who cannot handle an exception here. If throwOnException
184+
// is false, it means the caller explicitly wants to suppress exceptions and normalize
185+
// them to a false return value.
186+
187+
if (!success
188+
&& throwOnException
189+
&& _pal?.ChainStatus is not { Length: > 0 }
190+
&& LocalAppContextSwitches.X509ChainBuildThrowOnInternalError)
191+
{
192+
throw new CryptographicException(SR.Cryptography_X509_ChainBuildingFailed);
157193
}
158194

159-
return verified.Value;
195+
return success;
160196
}
161197
}
162198

‎src/libraries/System.Security.Cryptography/tests/X509Certificates/ChainTests.cs‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1269,6 +1269,58 @@ public static void BuildChainForSelfSignedSha3Certificate()
12691269
}
12701270
}
12711271

1272+
[Fact]
1273+
public static void BuildChainForSelfSignedCertificate_WithSha256RsaSignature()
1274+
{
1275+
using (ChainHolder chainHolder = new ChainHolder())
1276+
using (X509Certificate2 cert = new X509Certificate2(TestData.SelfSignedCertSha256RsaBytes))
1277+
{
1278+
X509Chain chain = chainHolder.Chain;
1279+
chain.ChainPolicy.RevocationMode = X509RevocationMode.NoCheck;
1280+
chain.ChainPolicy.VerificationTime = cert.NotBefore.AddHours(2);
1281+
1282+
// No custom root of trust store means that this self-signed cert will at
1283+
// minimum be marked UntrustedRoot.
1284+
1285+
Assert.False(chain.Build(cert));
1286+
AssertExtensions.HasFlag(X509ChainStatusFlags.UntrustedRoot, chain.AllStatusFlags());
1287+
}
1288+
}
1289+
1290+
[Fact]
1291+
public static void BuildChainForSelfSignedCertificate_WithUnknownOidSignature()
1292+
{
1293+
using (ChainHolder chainHolder = new ChainHolder())
1294+
using (X509Certificate2 cert = new X509Certificate2(TestData.SelfSignedCertDummyOidBytes))
1295+
{
1296+
X509Chain chain = chainHolder.Chain;
1297+
chain.ChainPolicy.RevocationMode = X509RevocationMode.NoCheck;
1298+
chain.ChainPolicy.VerificationTime = cert.NotBefore.AddHours(2);
1299+
1300+
// This tests a self-signed cert whose signature block contains a garbage signing alg OID.
1301+
// Some platforms return NotSignatureValid to indicate that they cannot understand the
1302+
// signature block. Other platforms return PartialChain to indicate that they think the
1303+
// bad signature block might correspond to some unknown, untrusted signer. Yet other
1304+
// platforms simply fail the operation; e.g., Windows's CertGetCertificateChain API returns
1305+
// NTE_BAD_ALGID, which we bubble up as CryptographicException.
1306+
1307+
if (PlatformDetection.UsesAppleCrypto)
1308+
{
1309+
Assert.False(chain.Build(cert));
1310+
AssertExtensions.HasFlag(X509ChainStatusFlags.PartialChain, chain.AllStatusFlags());
1311+
}
1312+
else if (PlatformDetection.IsOpenSslSupported)
1313+
{
1314+
Assert.False(chain.Build(cert));
1315+
AssertExtensions.HasFlag(X509ChainStatusFlags.NotSignatureValid, chain.AllStatusFlags());
1316+
}
1317+
else
1318+
{
1319+
Assert.ThrowsAny<CryptographicException>(() => chain.Build(cert));
1320+
}
1321+
}
1322+
}
1323+
12721324
internal static X509ChainStatusFlags AllStatusFlags(this X509Chain chain)
12731325
{
12741326
return chain.ChainStatus.Aggregate(

0 commit comments

Comments
 (0)