ClientWebSocket options are potentially silently ignored in case an invoker is passed

[CarnaViire]

Also discovered while looking at ClientWebSocket code.

If the user both passes a custom HttpMessageInvoker and sets security options like Credentials or RemoteCertificateValidationCallback, they seem to be just silently ignored, as the passed HttpMessageInvoker will be used without checking the options.

runtime/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/WebSocketHandle.Managed.cs

Line 51 in 3824cb2

invoker ??= new HttpMessageInvoker(SetupHandler(options, out disposeHandler));

This might result in security issues.

Unless I miss something and there are checks somewhere that I haven't found, I believe we should forbid setting the security options in case a custom invoker is passed.

cc @greenEkatherine

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Also discovered while looking at ClientWebSocket code.

If the user both passes a custom HttpMessageInvoker and sets security options like Credentials or RemoteCertificateValidationCallback, they seem to be just silently ignored, as the passed HttpMessageInvoker will be used without checking the options.

runtime/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/WebSocketHandle.Managed.cs

Line 51 in 3824cb2

invoker ??= new HttpMessageInvoker(SetupHandler(options, out disposeHandler));

This might result in security issues.

Unless I miss something and there are checks somewhere that I haven't found, I believe we should forbid setting the security options in case a custom invoker is passed.

cc @greenEkatherine

Author:	CarnaViire
Assignees:	-
Labels:	area-System.Net
Milestone:	-

[greenEkatherine]

Security options can be modified before invoker is passed. Should we throw in the case if they have different options?

[CarnaViire]

1. If it's possible to check whether all the options are same on the passed invoker, we should do that.
2. If it's not possible, I think we should throw in case they are set and user is passing an invoker.

cc @wfurt @stephentoub
It is not enough to check that the following options are the same on the passed invoker, is it? There are things like LocalCertificateSelectionCallback that can override whatever is set in ClientCertificates, and it is always set by default, if I remember correctly, so would that mean we have only second option (throw without checking) available?

runtime/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/WebSocketHandle.Managed.cs

Lines 269 to 291 in 3824cb2

	handler.CookieContainer = options.Cookies;
	handler.UseCookies = options.Cookies != null;
	handler.SslOptions.RemoteCertificateValidationCallback = options.RemoteCertificateValidationCallback;
	handler.Credentials = options.UseDefaultCredentials ?
	CredentialCache.DefaultCredentials :
	options.Credentials;
	if (options.Proxy == null)
	{
	handler.UseProxy = false;
	}
	else if (options.Proxy != DefaultWebProxy.Instance)
	{
	handler.Proxy = options.Proxy;
	}
	if (options._clientCertificates?.Count > 0) // use field to avoid lazily initializing the collection
	{
	Debug.Assert(handler.SslOptions.ClientCertificates == null);
	handler.SslOptions.ClientCertificates = new X509Certificate2Collection();
	handler.SslOptions.ClientCertificates.AddRange(options.ClientCertificates);
	}

[MihaZupan]

The invoker hides access to the underlying handler. Outside of reflection, you can't inspect its settings.

[CarnaViire]

Right, then the only option is to throw

[karelz]

Triage: This is new feature where we do not warn developer that they did something wrong and we might use unexpected credentials or wrong cookies, proxy, etc. That may have potentially security impact on poorly written user code.
Given that, we should try to address it in 7.0.