Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add DangerousAcceptAnyServerCertificateValidator property to HttpClient #19908

Merged
merged 3 commits into from May 18, 2017

Conversation

@stephentoub
Copy link
Member

stephentoub commented May 17, 2017

It's a common pattern in certain situations (in particular in tests) to use HttpClient to connect to a server with a certificate that shouldn't be validated, e.g. a self-signed cert. This is commonly achieved with HttpClientHandler by setting the ServerCertificateCustomValidationCallback property to a delegate that always returns true (returning true means that the cert passed validation):

handler.ServerCertificateCustomValidationCallback = delegate { return true; };

However, we can't support this callback on all implementations of HttpClientHandler. For example, on macOS, the default libcurl built against SecureTransport doesn't provide the callback we'd need to properly implement the callback, and as such, today trying to set up such a callback results in a PlatformNotSupportedException. This blocks some important scenarios; that includes testing (e.g. ASP.NET had to move away from HttpClient because of this, aspnet/MetaPackages#100) but also higher level libraries and tools that provide similar functionality (e.g. PowerShell's Invoke-WebRequest -SkipCertificateCheck is blocked by this PowerShell/PowerShell#3648).

This PR adds an approved API to address this case:

public static Func<HttpRequestMessage, X509Certificate2, X509Chain, SslPolicyErrors, bool> DangerousAcceptAnyServerCertificateValidator { get; }

The actual implementation of this property is trivial, simply returning a cached always-return-true delegate:

public static Func<HttpRequestMessage, X509Certificate2, X509Chain, SslPolicyErrors, bool> DangerousAcceptAnyServerCertificateValidator { get; } = delegate { return true; };

so developers can just substitute it for their custom delegate:

handler.ServerCertificateCustomValidationCallback = HttpClientHandler.DangerousAcceptAnyServerCertificateValidator;

The benefit of that is it gives HttpClientHandler implementations a known object reference identity that can be compared against to understand the developer's intention: if the object stored in ServerCertificateCustomValidationCallback is reference equals to DangerousAcceptAnyServerCertificateValidator, then we know that it will always return true, and on platforms where we'd otherwise throw a PlatformNotSupportedException because we can't hook up a callback, we can still entirely disable validation. This enables such scenarios to work. (As a side benefit, such a property means developers can use this property and give tools an easier opportunity for flagging the danger of such disabling of validation, making it easier for developers to avoid shipping insecure applications.)

This PR:

  • Adds the property and the singleton delegate implementation.
  • Adds some tests to ensure it behaves correctly.
  • On Unix, if we would throw a PNSE because a callback was supplied, we special-case DangerousAcceptAnyServerCertificateValidator to avoid throwing. No changes were made to the implementation on Windows.

In the future, we could choose to go further and avoid hooking up the delegate at all on both Windows and Unix even if callbacks are supported, as doing so may save some overhead. However, as this is not meant to be a common case in production, I've only modified code necessary to get these scenarios working.

Fixes #19709
Related to #19718

cc: @bartonjs, @davidsh, @CIPop, @Priya91, @wfurt, @geoffkizer

@davidsh davidsh added this to the 2.0.0 milestone May 17, 2017

@davidsh

This comment has been minimized.

Copy link
Member

davidsh commented May 17, 2017

As a side benefit, such a property means developers can use this property and give tools an easier opportunity for flagging the danger of such disabling of validation, making it easier for developers to avoid shipping insecure applications

We should do two things to support the above:

  • Modify FxCop security rules to flag this API use as insecure.
  • Modify UWP WACK tools to flag this API and prevent submittal to the Windows App Store.

@morganbr - what is the process for handling FxCop changes?

The UWP WACK tool modification will be handled separately via a Windows Platform change process

@stephentoub stephentoub force-pushed the stephentoub:ssl_callback branch from e596085 to fe0179c May 17, 2017

@stephentoub

This comment has been minimized.

Copy link
Member Author

stephentoub commented May 17, 2017

Modify UWP WACK tools to flag this API and prevent submittal to the Windows App Store.

For the record, I still don't think we should do this. All it does is add friction to developers without actually preventing anything. But I'll leave it at that.

@davidsh
Copy link
Member

davidsh left a comment

LGTM

@stephentoub stephentoub force-pushed the stephentoub:ssl_callback branch from fe0179c to c104df0 May 17, 2017

@CIPop
Copy link
Member

CIPop left a comment

Left a few questions.

{
EventSourceTrace("Disabling peer and host verification per {0}", nameof(HttpClientHandler.DangerousAcceptAnyServerCertificateValidator), easy: easy);
easy.SetCurlOption(Interop.Http.CURLoption.CURLOPT_SSL_VERIFYPEER, 0); // don't verify the peer
easy.SetCurlOption(Interop.Http.CURLoption.CURLOPT_SSL_VERIFYHOST, 0); // don't verify the peer cert's hostname

This comment has been minimized.

Copy link
@CIPop

CIPop May 17, 2017

Member

Should CRL also be deactivated here?

This comment has been minimized.

Copy link
@stephentoub

stephentoub May 17, 2017

Author Member

It is by default. Nothing need be changed. In fact we throw PNSE if you try to enable it and can't.

switch (answer)
{
case CURLcode.CURLE_OK:
// We successfully registered. If we'll be invoking a user-provided callback to verify the server

This comment has been minimized.

Copy link
@CIPop

CIPop May 17, 2017

Member

I'm a bit confused by the comment: I thought the callback will be invoked internally regardless if the user provided a callback or not (if the public API is null) to perform the chain verification then the name verification.

Because if this isn't true: wouldn't we keep the host name validation enabled?

This comment has been minimized.

Copy link
@stephentoub

stephentoub May 17, 2017

Author Member

We do the validation in our callback: if libcurl does it and fails, we never get the callback.

This comment has been minimized.

Copy link
@CIPop

CIPop May 17, 2017

Member

That's what I thought. The comment seems out of sync: it should not have that "If".
Maybe something on the lines of "we'll disable libcurl's verification of the host name and perform it within the callback" would be clearer.

This comment has been minimized.

Copy link
@stephentoub

stephentoub May 18, 2017

Author Member

I'll fix the comment. Thanks.

@@ -85,6 +85,7 @@ public partial class HttpClientHandler : System.Net.Http.HttpMessageHandler
public System.Security.Cryptography.X509Certificates.X509CertificateCollection ClientCertificates { get { throw null; } }
public System.Net.CookieContainer CookieContainer { get { throw null; } set { } }
public System.Net.ICredentials Credentials { get { throw null; } set { } }
public static System.Func<System.Net.Http.HttpRequestMessage, System.Security.Cryptography.X509Certificates.X509Certificate2, System.Security.Cryptography.X509Certificates.X509Chain, System.Net.Security.SslPolicyErrors, bool> DangerousAcceptAnyServerCertificateValidator { get { throw null; } }

This comment has been minimized.

Copy link
@CIPop

CIPop May 17, 2017

Member

We don't need to add ifdefs here to limit to core right? (I remember a time when we used to do that in ref but I'm not yet caught up with recent changes.)

This comment has been minimized.

Copy link
@stephentoub

stephentoub May 17, 2017

Author Member

Correct.

}

[Theory]
[InlineData(SslProtocols.Tls, false)]

This comment has been minimized.

Copy link
@CIPop

CIPop May 17, 2017

Member

I'm not against coverage but I don't understand why this test is varying the protocol. If there is a particular reason a comment would be useful (we should have different tests varying the protocol already).

This comment has been minimized.

Copy link
@stephentoub

stephentoub May 18, 2017

Author Member

I can cull it back and add a comment. It was mainly to ensure we're appropriately setting version constraints even when the callback is applied (since I know in the implementation that has to be done in particular places).

@morganbr

This comment has been minimized.

Copy link
Contributor

morganbr commented May 17, 2017

@davidsh This could be a good rule for https://github.com/dotnet/roslyn-analyzers . I'm not sure if new FXCop rules are still being added. I wouldn't be in favor of adding it to the WACK, however. The WACK prevents using code that won't function or would be dangerous to UWP sandboxing. It doesn't prevent developers from having poorly chosen patterns.

@@ -293,12 +290,12 @@
<value>Can not access a closed Stream.</value>
</data>
<data name="net_http_libcurl_callback_notsupported" xml:space="preserve">
<value>The handler does not support custom handling of certificates with this combination of libcurl ({0}) and its SSL backend ("{1}").</value>
<value>The handler does not support custom validation of certificates with this combination of libcurl ({0}) and its SSL backend ("{1}"). A libcurl built with OpenSSL is required.</value>

This comment has been minimized.

Copy link
@bartonjs

bartonjs May 17, 2017

Member

When I made these three messages I was very much explicitly trying to avoid saying OpenSSL. I don't quite remember why, but that was my goal.

This comment has been minimized.

Copy link
@bartonjs

bartonjs May 17, 2017

Member

(Maybe because #19718 was still several thought experiments away?)

This comment has been minimized.

Copy link
@stephentoub

stephentoub May 18, 2017

Author Member

I think we should be explicit about it. Without saying OpenSSL, we don't give the developer guidance on how to avoid the problem. By adding the phrase, we're not only telling them what's not supported and why, we're telling them how to fix it.

This comment has been minimized.

Copy link
@bartonjs

bartonjs May 18, 2017

Member

Right now on macOS using libcurl+openssl gains nothing. And even if I can get my gedankenexperiment working today the clientcerts message will still be misleading (since it will make callback work, but not clientcerts).

So mainly I'm concerned with macOS users saying "it didn't work, and I followed the exception's instructions, and it gave me the exact same error".

This comment has been minimized.

Copy link
@stephentoub

stephentoub May 18, 2017

Author Member

Hmm, ok, that's fair. I guess we could have different messages for macOS and Linux.

{
EventSourceTrace("Disabling peer and host verification per {0}", nameof(HttpClientHandler.DangerousAcceptAnyServerCertificateValidator), easy: easy);
easy.SetCurlOption(Interop.Http.CURLoption.CURLOPT_SSL_VERIFYPEER, 0); // don't verify the peer
easy.SetCurlOption(Interop.Http.CURLoption.CURLOPT_SSL_VERIFYHOST, 0); // don't verify the peer cert's hostname

This comment has been minimized.

Copy link
@bartonjs

bartonjs May 17, 2017

Member

darwinssl, at least, uses VERIFYHOST=1 as "should I enable SNI". So it would be good if we can leave this on. But maybe that causes it to still reject the self-signed cert if the hostname doesn't match, in which case... cutting SNI is a sad side effect that we may need to write down. (Though I don't know what the state of SNI is for us on Linux or Windows... so maybe this is "a nice feature of macOS might get turned off"))

This comment has been minimized.

Copy link
@CIPop

CIPop May 17, 2017

Member

Though I don't know what the state of SNI is for us on Linux or Windows

SNI should work on Windows for the client side in all components (SslStream, HttpClient, etc)

This comment has been minimized.

Copy link
@stephentoub

stephentoub May 18, 2017

Author Member

I'll double check, but I believe when I tested it with the default of CURLOPT_SSL_VERIFYHOST==1, it still failed if the hostname didn't match, in which case we'll need to set it to 0. If that means SNI doesn't work when this is used, we can doc it.

This comment has been minimized.

Copy link
@stephentoub

stephentoub May 18, 2017

Author Member

I tried removing the CURLOPT_SSL_VERIFYHOST=0 setting. With an NSS backend, VERIFYPEER=0 automatically implies VERIFYHOST=0 (and there's no way to override that), so it didn't affect the CentOS/Fedora/RHEL VMs with have in CI, as they have an NSS backend. With a GnuTLS backend on Ubuntu 16.10, and without VERIFYHOST=0, and connecting to a site with an incorrect host name, we get:

  [Microsoft-System-Net-Http-12] (65, 64, CurlDebugFunction, CURLINFO_TEXT: subjectAltName does not match wrong.host.badssl.com).
  [Microsoft-System-Net-Http-12] (65, 64, CurlDebugFunction, CURLINFO_TEXT: SSL: no alternative certificate subject name matches target host name 'wrong.host.badssl.com').

which then fails the request with:

SSL peer certificate or SSH remote key was not OK

Similar error for all of our localhost ssl tests. So I at least need to leave the VERIFYHOST=0 part in for the Linux CurlHandler.SslProvider.cs. However, CI passed on macOS without setting VERIFYHOST, so maybe we could get away without it? But I'm not sure why this didn't fail. With VERIFYHOST at its default value, libcurl should be calling Apple's SSLSetPeerDomainName function:
https://github.com/curl/curl/blob/master/lib/vtls/darwinssl.c#L1536
and that should cause the connection to fail if the hostname doesn't match.

@CIPop

CIPop approved these changes May 17, 2017

@CIPop

This comment has been minimized.

Copy link
Member

CIPop commented May 17, 2017

@davidsh This could be a good rule for https://github.com/dotnet/roslyn-analyzers . I'm not sure if new FXCop rules are still being added. I wouldn't be in favor of adding it to the WACK, however. The WACK prevents using code that won't function or would be dangerous to UWP sandboxing. It doesn't prevent developers from having poorly chosen patterns.

@morganbr @davidsh I've too raised some questions about WACK. Wouldn't it be simpler to just continue the discussion in #19709

I've thought the matter was closed and both Dev, API review board and Security signed off on @stephentoub's proposal. My signoff above was made with that assumption.

@stephentoub stephentoub force-pushed the stephentoub:ssl_callback branch 2 times, most recently from f32ae4a to 483a9d9 May 18, 2017

@gkhanna79

This comment has been minimized.

Copy link
Member

gkhanna79 commented May 18, 2017

@stephentoub Can you PTAL at the outerloop failures?

@stephentoub

This comment has been minimized.

Copy link
Member Author

stephentoub commented May 18, 2017

@gkhanna79, the failure is unrelated: it's #19702, a fix for which is up at #19920. I'm going to run a few more legs, though.

stephentoub added some commits May 17, 2017

Special-case DangerousAcceptAnyServerCertificateValidator on Unix
In situations where setting ServerCertificateValidationCallback would result in a PlatformNotSupportedException, if it was set to DangerousAcceptAnyServerCertificateValidator instead simply disable CURLOPT_SSL_VERIFYPEER and CURLOPT_SSL_VERIFYHOST.  This enables a common case of `delegate { return true; }` to work on libcurls with non-SSL backends, such as the default on macOS.

@stephentoub stephentoub force-pushed the stephentoub:ssl_callback branch from 483a9d9 to 513426e May 18, 2017

@stephentoub

This comment has been minimized.

Copy link
Member Author

stephentoub commented May 18, 2017

@dotnet-bot test outerloop netcoreapp Windows_NT Debug
@dotnet-bot test outerloop netcoreapp Ubuntu14.04 Debug
@dotnet-bot test outerloop netcoreapp Ubuntu16.10 Debug
@dotnet-bot test outerloop netcoreapp OSX10.12 Debug
@dotnet-bot test outerloop netcoreapp RHEL7.2 Debug

@stephentoub stephentoub merged commit ea706ea into dotnet:master May 18, 2017

25 checks passed

CROSS Check Build finished.
Details
Innerloop CentOS7.1 Debug x64 Build and Test Build finished.
Details
Innerloop CentOS7.1 Release x64 Build and Test Build finished.
Details
Innerloop OSX10.12 Debug x64 Build and Test Build finished.
Details
Innerloop OSX10.12 Release x64 Build and Test Build finished.
Details
Innerloop PortableLinux Debug x64 Build and Test Build finished.
Details
Innerloop PortableLinux Release x64 Build and Test Build finished.
Details
Innerloop Tizen armel Debug Cross Build Build finished.
Details
Innerloop Tizen armel Release Cross Build Build finished.
Details
Innerloop Ubuntu14.04 Debug x64 Build and Test Build finished.
Details
Innerloop Ubuntu14.04 Release x64 Build and Test Build finished.
Details
Innerloop Ubuntu14.04 arm Release Cross Build Build finished.
Details
Innerloop Ubuntu16.04 arm Debug Cross Build Build finished.
Details
Innerloop Windows_NT Debug x86 Build and Test Build finished.
Details
Innerloop Windows_NT Release x64 Build and Test Build finished.
Details
Innerloop netfx Windows_NT Debug x86 Build and Test Build finished.
Details
Innerloop netfx Windows_NT Release x64 Build and Test Build finished.
Details
OuterLoop netcoreapp OSX10.12 Debug x64 Build finished.
Details
OuterLoop netcoreapp RHEL7.2 Debug x64 Build finished.
Details
OuterLoop netcoreapp Ubuntu14.04 Debug x64 Build finished.
Details
OuterLoop netcoreapp Ubuntu16.10 Debug x64 Build finished.
Details
OuterLoop netcoreapp Windows_NT Debug x64 Build finished.
Details
Vertical uap Build Build finished.
Details
Vertical uapaot Build Build finished.
Details
Windows_NT Debug AllConfigurations Build Build finished.
Details

@stephentoub stephentoub deleted the stephentoub:ssl_callback branch May 18, 2017

@karelz karelz added this to the 2.1.0 milestone May 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.