[Android] Fix app crash when using non-native HttpClientHandler by simonrozsival · Pull Request #69565 · dotnet/runtime

simonrozsival · 2022-05-19T16:55:09Z

HTTP requests done with HttpClient + HttpClientHandler caused Android apps to crash.

For example, a simple HTTP request using the default HttpClient will throw an exception with the following stack trace:

varclient=newHttpClient();try{varresponse=awaitclient.GetAsync("https://microsoft.com");Console.WriteLine(response);}catch(Exceptione){Console.WriteLine(e);}

System.Net.Http.HttpRequestException: The SSL connection could not be established, see inner exception.---> System.NullReferenceException: Object reference not set to an instance of an object
at System.Net.CertificateValidationPal.GetRemoteCertificate(SafeDeleteContextsecurityContext,BooleanretrieveChainCertificates,X509Chain&chain)
at System.Net.CertificateValidationPal.GetRemoteCertificate(SafeDeleteContextsecurityContext)
at System.Net.Security.SslStream.SelectClientCertificate(Boolean&sessionRestartAttempt)
at System.Net.Security.SslStream.AcquireClientCredentials(Byte[]&thumbPrint)
at System.Net.Security.SslStream.GenerateToken(ReadOnlySpan`1inputBuffer,Byte[]&output)
at System.Net.Security.SslStream.NextMessage(ReadOnlySpan`1incomingBuffer)
at System.Net.Security.SslStream.<ForceAuthenticationAsync>d__144`1[[System.Net.Security.AsyncReadWriteAdapter,System.Net.Security,Version=7.0.0.0,Culture=neutral,PublicKeyToken=b03f5f7f11d50a3a]].MoveNext()
at System.Net.Http.ConnectHelper.EstablishSslConnectionAsync(SslClientAuthenticationOptionssslOptions,HttpRequestMessagerequest,Booleanasync,Streamstream,CancellationTokencancellationToken)---End of inner exception stack trace ---at System.Net.Http.ConnectHelper.EstablishSslConnectionAsync(SslClientAuthenticationOptionssslOptions,HttpRequestMessagerequest,Booleanasync,Streamstream,CancellationTokencancellationToken)
at System.Net.Http.HttpConnectionPool.ConnectAsync(HttpRequestMessagerequest,Booleanasync,CancellationTokencancellationToken)
at System.Net.Http.HttpConnectionPool.CreateHttp11ConnectionAsync(HttpRequestMessagerequest,Booleanasync,CancellationTokencancellationToken)
at System.Net.Http.HttpConnectionPool.AddHttp11ConnectionAsync(QueueItemqueueItem)
at System.Threading.Tasks.TaskCompletionSourceWithCancellation`1.<WaitWithCancellationAsync>d__1[[System.Net.Http.HttpConnection,System.Net.Http,Version=7.0.0.0,Culture=neutral,PublicKeyToken=b03f5f7f11d50a3a]].MoveNext()
at System.Net.Http.HttpConnectionPool.GetHttp11ConnectionAsync(HttpRequestMessagerequest,Booleanasync,CancellationTokencancellationToken)
at System.Net.Http.HttpConnectionPool.SendWithVersionDetectionAndRetryAsync(HttpRequestMessagerequest,Booleanasync,BooleandoRequestAuth,CancellationTokencancellationToken)
at System.Net.Http.RedirectHandler.SendAsync(HttpRequestMessagerequest,Booleanasync,CancellationTokencancellationToken)
at System.Net.Http.HttpClient.<SendAsync>g__Core|83_0(HttpRequestMessagerequest,HttpCompletionOptioncompletionOption,CancellationTokenSourcects,BooleandisposeCts,CancellationTokenSourcependingRequestsCts,CancellationTokenoriginalCancellationToken)
at Program.<Main>$(String[]args)in/home/simon/dotnet/runtime/src/mono/sample/Android/Program.cs:line 12

There is a missing null check in CertificateValidationpal.Android.cs which was removed recently (https://github.com/dotnet/runtime/pull/68188/files#diff-5f4278661dbb1331a13f14f1bf94b3a67e7474e8e4c32a21a99860f966ab02aeL66-L68).

Possibly also related to #69557

ghost · 2022-05-19T16:55:15Z

Tagging subscribers to 'arch-android': @steveisok, @akoeplinger
See info in area-owners.md if you want to be subscribed.

Issue Details

HTTP requests done with HttpClient + HttpClientHandler caused Android apps to crash. For example the test added in this PR would crash with this stack trace:

System.Net.Http.HttpRequestException: The SSL connection could not be established, see inner exception.---> System.NullReferenceException: Object reference not set to an instance of an object
at System.Net.CertificateValidationPal.GetRemoteCertificate(SafeDeleteContextsecurityContext,BooleanretrieveChainCertificates,X509Chain&chain)
at System.Net.CertificateValidationPal.GetRemoteCertificate(SafeDeleteContextsecurityContext)
at System.Net.Security.SslStream.SelectClientCertificate(Boolean&sessionRestartAttempt)
at System.Net.Security.SslStream.AcquireClientCredentials(Byte[]&thumbPrint)
at System.Net.Security.SslStream.GenerateToken(ReadOnlySpan`1inputBuffer,Byte[]&output)
at System.Net.Security.SslStream.NextMessage(ReadOnlySpan`1incomingBuffer)
at System.Net.Security.SslStream.<ForceAuthenticationAsync>d__144`1[[System.Net.Security.AsyncReadWriteAdapter,System.Net.Security,Version=7.0.0.0,Culture=neutral,PublicKeyToken=b03f5f7f11d50a3a]].MoveNext()
at System.Net.Http.ConnectHelper.EstablishSslConnectionAsync(SslClientAuthenticationOptionssslOptions,HttpRequestMessagerequest,Booleanasync,Streamstream,CancellationTokencancellationToken)---End of inner exception stack trace ---at System.Net.Http.ConnectHelper.EstablishSslConnectionAsync(SslClientAuthenticationOptionssslOptions,HttpRequestMessagerequest,Booleanasync,Streamstream,CancellationTokencancellationToken)
at System.Net.Http.HttpConnectionPool.ConnectAsync(HttpRequestMessagerequest,Booleanasync,CancellationTokencancellationToken)
at System.Net.Http.HttpConnectionPool.CreateHttp11ConnectionAsync(HttpRequestMessagerequest,Booleanasync,CancellationTokencancellationToken)
at System.Net.Http.HttpConnectionPool.AddHttp11ConnectionAsync(QueueItemqueueItem)
at System.Threading.Tasks.TaskCompletionSourceWithCancellation`1.<WaitWithCancellationAsync>d__1[[System.Net.Http.HttpConnection,System.Net.Http,Version=7.0.0.0,Culture=neutral,PublicKeyToken=b03f5f7f11d50a3a]].MoveNext()
at System.Net.Http.HttpConnectionPool.GetHttp11ConnectionAsync(HttpRequestMessagerequest,Booleanasync,CancellationTokencancellationToken)
at System.Net.Http.HttpConnectionPool.SendWithVersionDetectionAndRetryAsync(HttpRequestMessagerequest,Booleanasync,BooleandoRequestAuth,CancellationTokencancellationToken)
at System.Net.Http.RedirectHandler.SendAsync(HttpRequestMessagerequest,Booleanasync,CancellationTokencancellationToken)
at System.Net.Http.HttpClient.<SendAsync>g__Core|83_0(HttpRequestMessagerequest,HttpCompletionOptioncompletionOption,CancellationTokenSourcects,BooleandisposeCts,CancellationTokenSourcependingRequestsCts,CancellationTokenoriginalCancellationToken)
at Program.<Main>$(String[]args)in/home/simon/dotnet/runtime/src/mono/sample/Android/Program.cs:line 12

There is a missing null check in CertificateValidationpal.Android.cs which was removed recently (https://github.com/dotnet/runtime/pull/68188/files#diff-5f4278661dbb1331a13f14f1bf94b3a67e7474e8e4c32a21a99860f966ab02aeL66-L68).

Possibly also related to #69557

Author:	simonrozsival
Assignees:	-
Labels:	`os-android`
Milestone:	-

simonrozsival · 2022-05-19T16:59:22Z

/azp run runtime-extra-platforms

azure-pipelines · 2022-05-19T16:59:36Z

Azure Pipelines successfully started running 1 pipeline(s).

akoeplinger · 2022-05-19T17:14:50Z

 ref X509Chain? chain)
 {
- SafeSslHandle sslContext = ((SafeDeleteSslContext)securityContext).SslContext;
+ SafeSslHandle? sslContext = ((SafeDeleteSslContext)securityContext)?.SslContext;


this shouldn't happen given that securityContext isn't marked as nullable, which suggests to me something higher up the call chain is doing something unexpected.

The underlaying handle is nullable.
private SafeFreeCredentials? _credentialsHandle; private SafeDeleteSslContext? _securityContext;
But is seems like it is like this for long time
runtime/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Protocol.cs
Line 254 in 3e5517b
remoteCert=CertificateValidationPal.GetRemoteCertificate(_securityContext!);
Making the parameter nullable make sense to me. I think it will be null when we try to select client certificate before the handshake. cc: @rzikm

The nullability annotations seem to be inconsistent across platforms:
in the OSX version there's no ? but there's a null check: https://github.com/dotnet/runtime/blob/main/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.OSX.cs#L55
in the Unix version there's a ? and there's a null check: https://github.com/dotnet/runtime/blob/main/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Unix.cs#L31
in the Windows version there's a ? and there's a null check: https://github.com/dotnet/runtime/blob/main/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Windows.cs#L34

Ok thanks, that sounds like indeed we're expecting a null securityContext here and should make it consistent across platforms.

wfurt · 2022-05-19T18:27:36Z

On a side note, #69527 removes the dual SafeHandle wrapping for Linux. We could do something similar for Android in future.

simonrozsival · 2022-05-20T08:06:55Z

/azp run runtime-extra-platforms

azure-pipelines · 2022-05-20T08:07:11Z

Azure Pipelines successfully started running 1 pipeline(s).

simonrozsival · 2022-05-20T13:28:30Z

/azp run runtime-extra-platforms

azure-pipelines · 2022-05-20T13:28:44Z

Azure Pipelines successfully started running 1 pipeline(s).

…roid-certificate-validation-pal-null-reference-exception

simonrozsival · 2022-05-26T17:30:48Z

/azp run outerloop-mono

azure-pipelines · 2022-05-26T17:30:53Z

No pipelines are associated with this pull request.

simonrozsival · 2022-05-26T17:31:17Z

/azp run runtime-extra-platforms

azure-pipelines · 2022-05-26T17:31:31Z

Azure Pipelines successfully started running 1 pipeline(s).

simonrozsival · 2022-05-26T17:48:51Z

/azp run runtime-libraries-mono outerloop

azure-pipelines · 2022-05-26T17:49:00Z

Azure Pipelines successfully started running 1 pipeline(s).

simonrozsival · 2022-05-26T17:55:29Z

I re-enabled the functional tests for System.Net.Http on Android and disabled all the tests that are failing. The bug that's fixed in this PR fixed several dozen outerloop tests.

simonrozsival · 2022-05-27T14:24:41Z

/azp run runtime-libraries-mono outerloop

azure-pipelines · 2022-05-27T14:24:53Z

Azure Pipelines successfully started running 1 pipeline(s).

simonrozsival · 2022-05-30T07:48:35Z

/azp run runtime-extra-platforms

azure-pipelines · 2022-05-30T07:49:01Z

Azure Pipelines successfully started running 1 pipeline(s).

simonrozsival · 2022-05-31T07:30:00Z

I think this PR is now ready:

the failing runtime test has something to do with Tar on Linux
the failing runtime-extra-platforms tests are unrelated to this PR (Browser, iOS, tvOS, Linux)
the failing runtime-libraries-mono outerloop tests are unrelated to this PR
- the only Android-related leg doesn't have any test failures related to System.Net.Http

simonrozsival added 2 commits May 19, 2022 18:41

Add test
df35e0c

Avoid null reference exception
2c64835

simonrozsival added the os-android label May 19, 2022

ghost added the area-System.Net label May 19, 2022

ghost assigned simonrozsival May 19, 2022

akoeplinger reviewed May 19, 2022
View reviewed changes

steveisok requested a review from wfurt May 19, 2022 17:15

simonrozsival added 3 commits May 20, 2022 09:45

Add missing nullable variable annotations
edfce3e

Update GetRemoteCertificate method nullability annotations
ab4555f

Fix compilation error
a680326

Remove test
8f3ac3c

simonrozsival added 7 commits May 25, 2022 10:38

Enable System.Net.Http functional tests on Android
a4f70da

Disable tests which test unsupported code paths
4f43fb1

Merge branch 'main' ofhttps://github.com/dotnet/runtimeinto fix-and…
493f94b
…roid-certificate-validation-pal-null-reference-exception

Fix accessing the underlying handler in tests
77942c0

Disable failing tests
5a39ba6

Skip tests that do not work on Android
3c676b5

TMP: include android tests in outerloop-mono
48c9f02

marek-safar reviewed May 26, 2022
View reviewed changes

Comment threadsrc/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Android.cs Outdated

simonrozsival added 3 commits May 27, 2022 09:04

Change test attributes
2a78426

Simplify null check
938e026

Fix invalid cast
02b21b8

dotnet deleted a comment from azure-pipelinesBotMay 27, 2022

steveisok self-requested a review May 31, 2022 14:38

steveisok approved these changes Jun 3, 2022
View reviewed changes

simonrozsival merged commit aafe4d4 into dotnet:mainJun 6, 2022

simonrozsival deleted the fix-android-certificate-validation-pal-null-reference-exception branch June 6, 2022 09:08

ghost locked as resolved and limited conversation to collaborators Jul 6, 2022

karelz added this to the 7.0.0 milestone Jul 19, 2022

Uh oh!

Conversation

simonrozsival commented May 19, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented May 19, 2022

Uh oh!

simonrozsival commented May 19, 2022

Uh oh!

azure-pipelinesBot commented May 19, 2022

Uh oh!

akoeplingerMay 19, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wfurtMay 19, 2022

Choose a reason for hiding this comment

Uh oh!

simonrozsivalMay 20, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akoeplingerMay 20, 2022

Choose a reason for hiding this comment

Uh oh!

wfurt commented May 19, 2022

Uh oh!

simonrozsival commented May 20, 2022

Uh oh!

azure-pipelinesBot commented May 20, 2022

Uh oh!

simonrozsival commented May 20, 2022

Uh oh!

azure-pipelinesBot commented May 20, 2022

Uh oh!

simonrozsival commented May 26, 2022

Uh oh!

azure-pipelinesBot commented May 26, 2022

Uh oh!

simonrozsival commented May 26, 2022

Uh oh!

azure-pipelinesBot commented May 26, 2022

Uh oh!

simonrozsival commented May 26, 2022

Uh oh!

azure-pipelinesBot commented May 26, 2022

Uh oh!

simonrozsival commented May 26, 2022

Uh oh!

Uh oh!

simonrozsival commented May 27, 2022

Uh oh!

azure-pipelinesBot commented May 27, 2022

Uh oh!

simonrozsival commented May 30, 2022

Uh oh!

azure-pipelinesBot commented May 30, 2022

Uh oh!

simonrozsival commented May 31, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

simonrozsival commented May 19, 2022•
edited
Loading

akoeplingerMay 19, 2022•
edited
Loading

simonrozsivalMay 20, 2022•
edited
Loading