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

[AndroidMessageHandler] ReadAsByteArrayAsync occasionally returning incorrect byte array #8740

Closed
tipa opened this issue Feb 20, 2024 · 2 comments · Fixed by #8753
Closed
Assignees
Labels
Area: Mono Runtime Mono-related issues: BCL bugs, AOT issues, etc.

Comments

@tipa
Copy link

tipa commented Feb 20, 2024

Android application type

.NET Android (net8.0-android)

Affected platform version

34.0.79/8.0.100

Description

My app is syncing contents to and from users personal cloud storage (e.g. OneDrive, Google Drive, & Dropbox), using the respective official HTTP APIs. The files are encrypted and after downloading, my app decrypts them using Aes.DecryptCbc. Now, occasionally, this method throws an exception:

System.Security.Cryptography.CryptographicException: Cryptography_PartialBlock

System.Security.Cryptography.UniversalCryptoOneShot.OneShotDecrypt(ILiteSymmetricCipher , PaddingMode , ReadOnlySpan`1 , Span`1 , Int32& )
System.Security.Cryptography.AesImplementation.TryDecryptCbcCore(ReadOnlySpan`1 , ReadOnlySpan`1 , Span`1 , PaddingMode , Int32& )
System.Security.Cryptography.SymmetricAlgorithm.DecryptCbc(ReadOnlySpan`1 , ReadOnlySpan`1 , PaddingMode )

It appears that an incorrect byte array has been returned by HttpContent.ReadAsByteArrayAsync() and the decryption fails due to invalid padding.

The following reasons make me believe that this is a bug in AndroidMessageHandler rather than my apps code or the servers returning wrong data:

  • when the CryptographicException mentioned above occurs, I am retrying the download of the same file & running the decryption again: it always works and doesn't throw an exception any more
  • the issue only occurs for Android, but not on iOS, macOS or Windows (see how I configured the HttpClientHandlers below)
  • the issue only occurs for requests to OneDrive and Google Drive, which do return gzip-compressed data when using Accept-Encoding: gzip,deflate in the http request. Dropbox on the other hand does not compress the http body when downloading files even when the Accept-Encoding header is present.

Therefore it appears to me that the problem is somehow connected to how the AndroidMessageHandler does decrypts the gzipped data before returning it to the application

Steps to Reproduce

I'd expect this problem very hard to reproduce. According to Google Console, my app does approx. a million requests to Google Drive & OneDrive every month and according to AppCenter, this issue occured ~100 times in the last 28 days. I did not yet observe the problem when downloading a file through the Dropbox API

As I am using the same HttpClient instance across the app and multiple requests are sent simultaneously, perhaps the issue is caused by some race condition.

public static AndroidMessageHandler HttpMessageHandler => new() { AutomaticDecompression = DecompressionMethods.Deflate | DecompressionMethods.GZip };
public static HttpClient HttpClient = new(HttpMessageHandler);

On iOS/macOS I use NSUrlSessionHandler. Note: I did not specify AutomaticDecompression as NSUrlSessionHandler is using gzip by default

public static NSUrlSessionHandler HttpMessageHandler => new()};

On Windows/UWP:

public static HttpClientHandler HttpMessageHandler => new() { AutomaticDecompression = DecompressionMethods.Deflate | DecompressionMethods.GZip };

And this is essentially how the request is sent

using var requestMessage = new HttpRequestMessage(HttpMethod.Get, url);
requestMessage.Headers.Authorization = new AuthenticationHeaderValue("Bearer", access_token);
var response = await HttpClient.SendAsync(requestMessage, cancelToken).ConfigureAwait(false);
var bytes = await response.Content.ReadAsByteArrayAsync().ConfigureAwait(false);

Did you find any workaround?

I will attempt to use another static HttpClient that is not configured with AutomaticDecompression that I will use for downloading binary data, so that the servers will not return gzipped data. As the data I am syncing is mostly .jpgs or .mp4s, gzip-compressing that data doesn't shrink the size any further, I might even save some CPU cycles and network bandwidth...

@tipa tipa added Area: App Runtime Issues in `libmonodroid.so`. needs-triage Issues that need to be assigned. labels Feb 20, 2024
@grendello grendello added Area: Mono Runtime Mono-related issues: BCL bugs, AOT issues, etc. and removed Area: App Runtime Issues in `libmonodroid.so`. needs-triage Issues that need to be assigned. labels Feb 21, 2024
@tipa
Copy link
Author

tipa commented Feb 22, 2024

Here's an example project that helps reproducing the issue: androidmessagehandler.zip

The returned data should be an 8 byte long byte array (string "dGVzdA==").
When the wrong byte array is returned, the byte array is 28 bytes long. It appears that these 28 bytes are the compressed bytes that were sent over the network, so the gzip decompression simply didn't take place in AndroidMessageHandler.

In my tests, the problem occured 3x after ~1.800 HTTP requests on Android. I confirmed via Fiddler that in fact there was always the exact same byte array sent over the network each time.
I also ran the same code on UWP, where the issue did not occur in 12.000 requests, after which I stopped the test.

@simonrozsival
Copy link
Member

@tipa thanks for the repro project.

@grendello This looks like a race condition to me. The AndroidMessageHandler has a bool decompress_here field that we briefly set to false whenever we're setting up a request:

https://github.com/xamarin/xamarin-android/blob/87d8914eef013eda16836ddc1532f13796bb91d2/src/Mono.Android/Xamarin.Android.Net/AndroidMessageHandler.cs#L1086-L1104

If at the same time we're processing a response in another thread, we might return raw compressed data instead of decompressing them:
https://github.com/xamarin/xamarin-android/blob/87d8914eef013eda16836ddc1532f13796bb91d2/src/Mono.Android/Xamarin.Android.Net/AndroidMessageHandler.cs#L733-L735

If I understand the code correctly, decompress_here should be false when the AndroidMessageHandler instance is created and it should change only when AutomaticDecompression changes. Even in that case, we should make sure that this change doesn't affect any request that is in flight at the time.

grendello pushed a commit that referenced this issue Mar 5, 2024
Fixes: #8740

There is a race condition in `AndroidMessageHandler` (see #8740 (comment) 
for more information). 

The solution is fairly simple: we should determine the decompression strategy only during the setup, when `AutomaticDecompression` is set. 
We don't need to do any more work during the request.

To prevent changing the decompression strategy while there are requests mid-flight, the setter will only accept values until the first request 
is sent. This is exactly what the `SocketsHttpHandler` does. I didn't want to touch any code unrelated to the race condition in this PR so if we 
want to prevent other properties from being mutated while HTTP requests are being sent, we should do it in a follow-up PR.
jonathanpeppers pushed a commit that referenced this issue Mar 11, 2024
Fixes: #8740

There is a race condition in `AndroidMessageHandler` (see #8740 (comment) 
for more information). 

The solution is fairly simple: we should determine the decompression strategy only during the setup, when `AutomaticDecompression` is set. 
We don't need to do any more work during the request.

To prevent changing the decompression strategy while there are requests mid-flight, the setter will only accept values until the first request 
is sent. This is exactly what the `SocketsHttpHandler` does. I didn't want to touch any code unrelated to the race condition in this PR so if we 
want to prevent other properties from being mutated while HTTP requests are being sent, we should do it in a follow-up PR.
@github-actions github-actions bot locked and limited conversation to collaborators Apr 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Area: Mono Runtime Mono-related issues: BCL bugs, AOT issues, etc.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants