Skip to content

Commit 7a97ad4

Browse files
rzikmvcsjonescarlossanlop
authored
[release/7.0] Fix server-side OCSP stapling on Linux (#96808)
* Recover from failed OCSP download. (#96448) * Recover from failed OCSP check. * Add 5s back-off after failed OCSP querry * Do not OCSP staple invalid OCSP responses * Add entire issuer chain to trusted X509_STORE when validating OCSP_Response Code review feedback More code review feedback Update src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Linux.cs Co-authored-by: Jeremy Barton <jbarton@microsoft.com> Fix compilation Always include root certificate * Fix compilation * Don't shorten OCSP expriation on failed server OCSP fetch (#96972) * Don't shorten OCSP expriation on failed server OCSP fetch * Code review feedback --------- Co-authored-by: Kevin Jones <kevin@vcsjones.com> Co-authored-by: Carlos Sánchez López <1175054+carlossanlop@users.noreply.github.com>
1 parent a314c5b commit 7a97ad4

File tree

8 files changed

+162
-28
lines changed

8 files changed

+162
-28
lines changed

‎src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OCSP.cs

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -29,27 +29,30 @@ private static unsafe partial int CryptoNative_X509DecodeOcspToExpiration(
2929
int len,
3030
SafeOcspRequestHandle req,
3131
IntPtr subject,
32-
IntPtr issuer,
32+
IntPtr* issuers,
33+
int issuersLen,
3334
ref long expiration);
3435

3536
internal static unsafe bool X509DecodeOcspToExpiration(
3637
ReadOnlySpan<byte> buf,
3738
SafeOcspRequestHandle request,
3839
IntPtr x509Subject,
39-
IntPtr x509Issuer,
40+
ReadOnlySpan<IntPtr> x509Issuers,
4041
out DateTimeOffset expiration)
4142
{
4243
long timeT = 0;
4344
int ret;
4445

4546
fixed (byte* pBuf = buf)
47+
fixed (IntPtr* pIssuers = x509Issuers)
4648
{
4749
ret = CryptoNative_X509DecodeOcspToExpiration(
4850
pBuf,
4951
buf.Length,
5052
request,
5153
x509Subject,
52-
x509Issuer,
54+
pIssuers,
55+
x509Issuers.Length,
5356
ref timeT);
5457
}
5558

‎src/libraries/Common/tests/System/Security/Cryptography/X509Certificates/RevocationResponder.cs

Lines changed: 29 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,9 @@ internal sealed class RevocationResponder : IDisposable
1616
private static readonly bool s_traceEnabled =
1717
Environment.GetEnvironmentVariable("TRACE_REVOCATION_RESPONSE") != null;
1818

19+
private static readonly byte[] s_invalidResponse =
20+
"<html><marquee>The server is down for maintenence.</marquee></html>"u8.ToArray();
21+
1922
private readonly HttpListener _listener;
2023

2124
private readonly Dictionary<string, CertificateAuthority> _aiaPaths =
@@ -29,7 +32,7 @@ private readonly Dictionary<string, CertificateAuthority> _crlPaths
2932

3033
public string UriPrefix { get; }
3134

32-
public bool RespondEmpty { get; set; }
35+
public RespondKind RespondKind { get; set; }
3336
public AiaResponseKind AiaResponseKind { get; set; }
3437

3538
public TimeSpan ResponseDelay { get; set; }
@@ -183,7 +186,12 @@ private void HandleRequest(HttpListenerContext context, ref bool responded)
183186
Thread.Sleep(ResponseDelay);
184187
}
185188

186-
byte[] certData = RespondEmpty ? Array.Empty<byte>() : GetCertDataForAiaResponseKind(AiaResponseKind, authority);
189+
byte[] certData = RespondKind switch
190+
{
191+
RespondKind.Empty => Array.Empty<byte>(),
192+
RespondKind.Invalid => s_invalidResponse,
193+
_ => GetCertDataForAiaResponseKind(AiaResponseKind, authority),
194+
};
187195

188196
responded = true;
189197
context.Response.StatusCode = 200;
@@ -201,7 +209,12 @@ private void HandleRequest(HttpListenerContext context, ref bool responded)
201209
Thread.Sleep(ResponseDelay);
202210
}
203211

204-
byte[] crl = RespondEmpty ? Array.Empty<byte>() : authority.GetCrl();
212+
byte[] crl = RespondKind switch
213+
{
214+
RespondKind.Empty => Array.Empty<byte>(),
215+
RespondKind.Invalid => s_invalidResponse,
216+
_ => authority.GetCrl(),
217+
};
205218

206219
responded = true;
207220
context.Response.StatusCode = 200;
@@ -236,7 +249,12 @@ private void HandleRequest(HttpListenerContext context, ref bool responded)
236249
return;
237250
}
238251

239-
byte[] ocspResponse = RespondEmpty ? Array.Empty<byte>() : authority.BuildOcspResponse(certId, nonce);
252+
byte[] ocspResponse = RespondKind switch
253+
{
254+
RespondKind.Empty => Array.Empty<byte>(),
255+
RespondKind.Invalid => s_invalidResponse,
256+
_ => authority.BuildOcspResponse(certId, nonce),
257+
};
240258

241259
if (DelayedActions.HasFlag(DelayedActionsFlag.Ocsp))
242260
{
@@ -468,4 +486,11 @@ public enum AiaResponseKind
468486
Cert = 0,
469487
Pkcs12 = 1,
470488
}
489+
490+
public enum RespondKind
491+
{
492+
Normal = 0,
493+
Empty = 1,
494+
Invalid = 2,
495+
}
471496
}

‎src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Linux.cs

Lines changed: 47 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -25,14 +25,31 @@ public partial class SslStreamCertificateContext
2525
private byte[]? _ocspResponse;
2626
private DateTimeOffset _ocspExpiration;
2727
private DateTimeOffset _nextDownload;
28+
// Private copy of the intermediate certificates, in case the user decides to dispose the
29+
// instances reachable through IntermediateCertificates property.
30+
private X509Certificate2[] _privateIntermediateCertificates;
31+
private X509Certificate2? _rootCertificate;
2832
private Task<byte[]?>? _pendingDownload;
2933
private List<string>? _ocspUrls;
30-
private X509Certificate2? _ca;
3134

3235
private SslStreamCertificateContext(X509Certificate2 target, X509Certificate2[] intermediates, SslCertificateTrust? trust)
3336
{
3437
Certificate = target;
3538
IntermediateCertificates = intermediates;
39+
if (intermediates.Length > 0)
40+
{
41+
_privateIntermediateCertificates = new X509Certificate2[intermediates.Length];
42+
43+
for (int i = 0; i < intermediates.Length; i++)
44+
{
45+
_privateIntermediateCertificates[i] = new X509Certificate2(intermediates[i]);
46+
}
47+
}
48+
else
49+
{
50+
_privateIntermediateCertificates = Array.Empty<X509Certificate2>();
51+
}
52+
3653
Trust = trust;
3754
SslContexts = new ConcurrentDictionary<SslProtocols, SafeSslContextHandle>();
3855

@@ -54,7 +71,7 @@ private SslStreamCertificateContext(X509Certificate2 target, X509Certificate2[]
5471
}
5572
}
5673

57-
if (KeyHandle== null)
74+
if (KeyHandle == null)
5875
{
5976
throw new NotSupportedException(SR.net_ssl_io_no_server_cert);
6077
}
@@ -75,15 +92,8 @@ partial void SetNoOcspFetch(bool noOcspFetch)
7592

7693
partial void AddRootCertificate(X509Certificate2? rootCertificate, ref bool transferredOwnership)
7794
{
78-
if (IntermediateCertificates.Length == 0)
79-
{
80-
_ca = rootCertificate;
81-
transferredOwnership = true;
82-
}
83-
else
84-
{
85-
_ca = IntermediateCertificates[0];
86-
}
95+
_rootCertificate = rootCertificate;
96+
transferredOwnership = rootCertificate != null;
8797

8898
if (!_staplingForbidden)
8999
{
@@ -148,7 +158,7 @@ partial void AddRootCertificate(X509Certificate2? rootCertificate, ref bool tran
148158
return new ValueTask<byte[]?>(pending);
149159
}
150160

151-
if (_ocspUrls is null && _ca is not null)
161+
if (_ocspUrls is null && _rootCertificate is not null)
152162
{
153163
foreach (X509Extension ext in Certificate.Extensions)
154164
{
@@ -191,7 +201,9 @@ partial void AddRootCertificate(X509Certificate2? rootCertificate, ref bool tran
191201

192202
private async Task<byte[]?> FetchOcspAsync()
193203
{
194-
X509Certificate2? caCert = _ca;
204+
Debug.Assert(_rootCertificate != null);
205+
X509Certificate2? caCert = _privateIntermediateCertificates.Length > 0 ? _privateIntermediateCertificates[0] : _rootCertificate;
206+
195207
Debug.Assert(_ocspUrls is not null);
196208
Debug.Assert(_ocspUrls.Count > 0);
197209
Debug.Assert(caCert is not null);
@@ -210,6 +222,13 @@ partial void AddRootCertificate(X509Certificate2? rootCertificate, ref bool tran
210222
return null;
211223
}
212224

225+
IntPtr[] issuerHandles = ArrayPool<IntPtr>.Shared.Rent(_privateIntermediateCertificates.Length + 1);
226+
for (int i = 0; i < _privateIntermediateCertificates.Length; i++)
227+
{
228+
issuerHandles[i] = _privateIntermediateCertificates[i].Handle;
229+
}
230+
issuerHandles[_privateIntermediateCertificates.Length] = _rootCertificate.Handle;
231+
213232
using (SafeOcspRequestHandle ocspRequest = Interop.Crypto.X509BuildOcspRequest(subject, issuer))
214233
{
215234
byte[] rentedBytes = ArrayPool<byte>.Shared.Rent(Interop.Crypto.GetOcspRequestDerSize(ocspRequest));
@@ -226,8 +245,9 @@ partial void AddRootCertificate(X509Certificate2? rootCertificate, ref bool tran
226245

227246
if (ret is not null)
228247
{
229-
if (!Interop.Crypto.X509DecodeOcspToExpiration(ret, ocspRequest, subject, issuer, out DateTimeOffset expiration))
248+
if (!Interop.Crypto.X509DecodeOcspToExpiration(ret, ocspRequest, subject, issuerHandles.AsSpan(0, _privateIntermediateCertificates.Length + 1), out DateTimeOffset expiration))
230249
{
250+
ret = null;
231251
continue;
232252
}
233253

@@ -245,15 +265,27 @@ partial void AddRootCertificate(X509Certificate2? rootCertificate, ref bool tran
245265
_ocspResponse = ret;
246266
_ocspExpiration = expiration;
247267
_nextDownload = nextCheckA < nextCheckB ? nextCheckA : nextCheckB;
248-
_pendingDownload = null;
249268
break;
250269
}
251270
}
252271

272+
issuerHandles.AsSpan().Clear();
273+
ArrayPool<IntPtr>.Shared.Return(issuerHandles);
253274
ArrayPool<byte>.Shared.Return(rentedBytes);
254275
ArrayPool<char>.Shared.Return(rentedChars.Array!);
255276
GC.KeepAlive(Certificate);
277+
GC.KeepAlive(_privateIntermediateCertificates);
278+
GC.KeepAlive(_rootCertificate);
256279
GC.KeepAlive(caCert);
280+
281+
_pendingDownload = null;
282+
if (ret == null)
283+
{
284+
// All download attempts failed, don't try again for 5 seconds.
285+
// This backoff will be applied only if the OCSP staple is not expired.
286+
// If it is expired, we will force-refresh it during next GetOcspResponseAsync call.
287+
_nextDownload = DateTimeOffset.UtcNow.AddSeconds(5);
288+
}
257289
return ret;
258290
}
259291
}

‎src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamCertificateContextTests.cs

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
// Licensed to the .NET Foundation under one or more agreements.
2+
// The .NET Foundation licenses this file to you under the MIT license.
3+
4+
using System.Reflection;
5+
using System.Security.Cryptography.X509Certificates;
6+
using System.Security.Cryptography.X509Certificates.Tests.Common;
7+
using System.Threading.Tasks;
8+
using Xunit;
9+
10+
namespace System.Net.Security.Tests
11+
{
12+
public static class SslStreamCertificateContextTests
13+
{
14+
[Fact]
15+
[OuterLoop("Subject to resource contention and load.")]
16+
[PlatformSpecific(TestPlatforms.Linux)]
17+
public static async Task Create_OcspDoesNotReturnOrCacheInvalidStapleData()
18+
{
19+
string serverName = $"{nameof(Create_OcspDoesNotReturnOrCacheInvalidStapleData)}.example";
20+
21+
CertificateAuthority.BuildPrivatePki(
22+
PkiOptions.EndEntityRevocationViaOcsp | PkiOptions.CrlEverywhere,
23+
out RevocationResponder responder,
24+
out CertificateAuthority rootAuthority,
25+
out CertificateAuthority[] intermediateAuthorities,
26+
out X509Certificate2 serverCert,
27+
intermediateAuthorityCount: 1,
28+
subjectName: serverName,
29+
keySize: 2048,
30+
extensions: TestHelper.BuildTlsServerCertExtensions(serverName));
31+
32+
using (responder)
33+
using (rootAuthority)
34+
using (CertificateAuthority intermediateAuthority = intermediateAuthorities[0])
35+
using (serverCert)
36+
using (X509Certificate2 rootCert = rootAuthority.CloneIssuerCert())
37+
using (X509Certificate2 issuerCert = intermediateAuthority.CloneIssuerCert())
38+
{
39+
responder.RespondKind = RespondKind.Invalid;
40+
41+
SslStreamCertificateContext context = SslStreamCertificateContext.Create(
42+
serverCert,
43+
additionalCertificates: new X509Certificate2Collection { issuerCert },
44+
offline: false);
45+
46+
MethodInfo fetchOcspAsyncMethod = typeof(SslStreamCertificateContext).GetMethod(
47+
"DownloadOcspAsync",
48+
BindingFlags.Instance | BindingFlags.NonPublic);
49+
FieldInfo ocspResponseField = typeof(SslStreamCertificateContext).GetField(
50+
"_ocspResponse",
51+
BindingFlags.Instance | BindingFlags.NonPublic);
52+
53+
Assert.NotNull(fetchOcspAsyncMethod);
54+
Assert.NotNull(ocspResponseField);
55+
56+
byte[] ocspFetch = await (ValueTask<byte[]>)fetchOcspAsyncMethod.Invoke(context, Array.Empty<object>());
57+
Assert.Null(ocspFetch);
58+
59+
byte[] ocspResponseValue = (byte[])ocspResponseField.GetValue(context);
60+
Assert.Null(ocspResponseValue);
61+
}
62+
}
63+
}
64+
}

‎src/libraries/System.Net.Security/tests/FunctionalTests/System.Net.Security.Tests.csproj

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
<Compile Include="ServerAsyncAuthenticateTest.cs" />
3232
<Compile Include="ServerNoEncryptionTest.cs" />
3333
<Compile Include="ServerRequireEncryptionTest.cs" />
34+
<Compile Include="SslStreamCertificateContextTests.cs" />
3435
<Compile Include="SslStreamConformanceTests.cs" />
3536
<Compile Include="SslStreamStreamToStreamTest.cs" />
3637
<Compile Include="SslStreamNetworkStreamTest.cs" />

‎src/libraries/System.Security.Cryptography.X509Certificates/tests/RevocationTests/AiaTests.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ public static void EmptyAiaResponseIsIgnored()
3333
using (endEntity)
3434
using (X509Certificate2 intermediate2Cert = intermediate2.CloneIssuerCert())
3535
{
36-
responder.RespondEmpty = true;
36+
responder.RespondKind = RespondKind.Empty;
3737

3838
RetryHelper.Execute(() => {
3939
using (ChainHolder holder = new ChainHolder())

‎src/native/libs/System.Security.Cryptography.Native/pal_x509.c

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1280,11 +1280,11 @@ CryptoNative_X509ChainVerifyOcsp(X509_STORE_CTX* storeCtx, OCSP_REQUEST* req, OC
12801280
return X509ChainVerifyOcsp(storeCtx, subject, issuer, req, resp, cachePath);
12811281
}
12821282

1283-
int32_t CryptoNative_X509DecodeOcspToExpiration(const uint8_t* buf, int32_t len, OCSP_REQUEST* req, X509* subject, X509* issuer, int64_t* expiration)
1283+
int32_t CryptoNative_X509DecodeOcspToExpiration(const uint8_t* buf, int32_t len, OCSP_REQUEST* req, X509* subject, X509** issuers, int issuersLen, int64_t* expiration)
12841284
{
12851285
ERR_clear_error();
12861286

1287-
if (buf == NULL || len == 0)
1287+
if (buf == NULL || len == 0 || issuersLen == 0)
12881288
{
12891289
return 0;
12901290
}
@@ -1307,7 +1307,16 @@ int32_t CryptoNative_X509DecodeOcspToExpiration(const uint8_t* buf, int32_t len,
13071307

13081308
if (bag != NULL)
13091309
{
1310-
if (X509_STORE_add_cert(store, issuer) && sk_X509_push(bag, issuer))
1310+
int i;
1311+
for (i = 0; i < issuersLen; i++)
1312+
{
1313+
if (!X509_STORE_add_cert(store, issuers[i]) || !sk_X509_push(bag, issuers[i]))
1314+
{
1315+
break;
1316+
}
1317+
}
1318+
1319+
if (i == issuersLen)
13111320
{
13121321
ctx = X509_STORE_CTX_new();
13131322
}
@@ -1321,7 +1330,7 @@ int32_t CryptoNative_X509DecodeOcspToExpiration(const uint8_t* buf, int32_t len,
13211330
{
13221331
int canCache = 0;
13231332
time_t expiration_t = 0;
1324-
X509VerifyStatusCode code = CheckOcspGetExpiry(req, resp, subject, issuer, ctx, &canCache, &expiration_t);
1333+
X509VerifyStatusCode code = CheckOcspGetExpiry(req, resp, subject, issuers[0], ctx, &canCache, &expiration_t);
13251334

13261335
if (sizeof(time_t) == sizeof(int64_t))
13271336
{

‎src/native/libs/System.Security.Cryptography.Native/pal_x509.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -407,4 +407,4 @@ PALEXPORT int32_t CryptoNative_X509ChainVerifyOcsp(X509_STORE_CTX* storeCtx,
407407
Decode len bytes of buf into an OCSP response, process it against the OCSP request, and return if the bytes were valid.
408408
If the bytes were valid, and the OCSP response had a nextUpdate value, assign it to expiration.
409409
*/
410-
PALEXPORT int32_t CryptoNative_X509DecodeOcspToExpiration(const uint8_t* buf, int32_t len, OCSP_REQUEST* req, X509* subject, X509* issuer, int64_t* expiration);
410+
PALEXPORT int32_t CryptoNative_X509DecodeOcspToExpiration(const uint8_t* buf, int32_t len, OCSP_REQUEST* req, X509* subject, X509** issuers, int issuersLen, int64_t* expiration);

0 commit comments

Comments
 (0)