Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Wrap cert callback leaked exceptions in WinHttpHandler #24107

Merged
merged 1 commit into from
Sep 18, 2017
Merged

Wrap cert callback leaked exceptions in WinHttpHandler #24107

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/Common/src/System/Net/Http/WinHttpException.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,11 @@ public WinHttpException(int error, string message) : base(error, message)
this.HResult = ConvertErrorCodeToHR(error);
}

public WinHttpException(int error, string message, Exception innerException) : base(message, innerException)
{
this.HResult = ConvertErrorCodeToHR(error);
}

public static int ConvertErrorCodeToHR(int error)
{
// This method allows common error detection code to be used by consumers
Expand Down Expand Up @@ -52,6 +57,13 @@ public static WinHttpException CreateExceptionUsingError(int error)
return e;
}

public static WinHttpException CreateExceptionUsingError(int error, Exception innerException)
{
var e = new WinHttpException(error, GetErrorMessage(error), innerException);
ExceptionStackTrace.AddCurrentStack(e);
return e;
}

public static string GetErrorMessage(int error)
{
// Look up specific error message in WINHTTP.DLL since it is not listed in default system resources
Expand Down
19 changes: 13 additions & 6 deletions src/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpRequestCallback.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,7 @@ private static void OnRequestSendingRequest(WinHttpRequestState state)

X509Chain chain = null;
SslPolicyErrors sslPolicyErrors;
bool result = false;

try
{
Expand All @@ -281,16 +282,16 @@ private static void OnRequestSendingRequest(WinHttpRequestState state)
out chain,
out sslPolicyErrors);

bool result = state.ServerCertificateValidationCallback(
result = state.ServerCertificateValidationCallback(
state.RequestMessage,
serverCertificate,
chain,
sslPolicyErrors);
if (!result)
{
throw WinHttpException.CreateExceptionUsingError(
(int)Interop.WinHttp.ERROR_WINHTTP_SECURE_FAILURE);
}
}
catch (Exception ex)
{
throw WinHttpException.CreateExceptionUsingError(
(int)Interop.WinHttp.ERROR_WINHTTP_SECURE_FAILURE, ex);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The description of ERROR_WINHTTP_SECURE_FAILURE is "One or more errors were found in the Secure Sockets Layer (SSL) certificate sent by the server."... is that the right error code to be using when it's the caller-supplied callback that's throwing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no other WinHttp specific error to return here and I didn't want to invent a new one. From WinHttp's point of view, the validation failed due to "errors" about the certificate validation process. So, it's not perfect but we use it.

FWIW, WinHttp returns this error even if there is nothing wrong with the certificate itself, but rather the SSL handshake has a problem (cipher suites mismatch, TLS protocol version mismatch etc.). So, the error number is really right....it's just that the error message string from WinHttp.dll isn't as descriptive as it should be.

}
finally
{
Expand All @@ -301,6 +302,12 @@ private static void OnRequestSendingRequest(WinHttpRequestState state)

serverCertificate.Dispose();
}

if (!result)
{
throw WinHttpException.CreateExceptionUsingError(
(int)Interop.WinHttp.ERROR_WINHTTP_SECURE_FAILURE);
}
}
}

Expand Down
6 changes: 4 additions & 2 deletions src/System.Net.Http.WinHttpHandler/tests/FunctionalTests/ServerCertificateTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,14 +111,16 @@ public async Task UseCallback_CallbackReturnsFailure_ThrowsInnerSecurityFailureE

[OuterLoop] // TODO: Issue #11345
[Fact]
public async Task UseCallback_CallbackThrowsSpecificException_ThrowsInnerSpecificException()
public async Task UseCallback_CallbackThrowsSpecificException_SpecificExceptionPropagatesAsBaseException()
{
var handler = new WinHttpHandler();
handler.ServerCertificateValidationCallback = CustomServerCertificateValidationCallback;
using (var client = new HttpClient(handler))
{
_validationCallbackHistory.ThrowException = true;
await Assert.ThrowsAsync<CustomException>(() => client.GetAsync(System.Net.Test.Common.Configuration.Http.SecureRemoteEchoServer));
HttpRequestException ex = await Assert.ThrowsAsync<HttpRequestException>(() =>
client.GetAsync(System.Net.Test.Common.Configuration.Http.SecureRemoteEchoServer));
Assert.True(ex.GetBaseException() is CustomException);
}
}

Expand Down
12 changes: 8 additions & 4 deletions src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.ServerCertificates.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,14 +204,18 @@ public async Task UseCallback_CallbackReturnsFailure_ThrowsException()
}
}

[ActiveIssue(21904, ~TargetFrameworkMonikers.Uap)]
[OuterLoop] // TODO: Issue #11345
[ConditionalFact(nameof(BackendSupportsCustomCertificateHandling))]
public async Task UseCallback_CallbackThrowsException_ExceptionPropagatesAsInnerException()
public async Task UseCallback_CallbackThrowsException_ExceptionPropagatesAsBaseException()
{
if (ManagedHandlerTestHelpers.IsEnabled)
{
return; // TODO #21904: ManagedHandler is not properly wrapping exception.
}

if (BackendDoesNotSupportCustomCertificateHandling) // can't use [Conditional*] right now as it's evaluated at the wrong time for the managed handler
{
Console.WriteLine($"Skipping {nameof(UseCallback_CallbackThrowsException_ExceptionPropagatesAsInnerException)}()");
Console.WriteLine($"Skipping {nameof(UseCallback_CallbackThrowsException_ExceptionPropagatesAsBaseException)}()");
return;
}

Expand All @@ -222,7 +226,7 @@ public async Task UseCallback_CallbackThrowsException_ExceptionPropagatesAsInner
handler.ServerCertificateCustomValidationCallback = delegate { throw e; };

HttpRequestException ex = await Assert.ThrowsAsync<HttpRequestException>(() => client.GetAsync(Configuration.Http.SecureRemoteEchoServer));
Assert.Same(e, ex.InnerException);
Assert.Same(e, ex.GetBaseException());
}
}

Expand Down