Android SslStream splits messages over 2048 bytes in multiple TLS records (breaks MS-TDS protocol) #95295

awakecoding · 2023-11-27T21:07:30Z

Description

The Android SslStream implementation using the Android SSLEngine APIs splits application data over 2048 bytes in multiple TLS records, something which normally isn't important, except for SQL Server which expects TDS RPC requests (query messages) to be sent in a single TLS record, breaking Microsoft.Data.SqlClient on Android. This issue only affects SslStream Android implementation, all other platforms resize internal buffers to send larger TLS records that can be safely fragmented at the transport level instead.

Reproduction Steps

Write a simple Android project that makes a TLS connection to a service and send a message of size larger than 2048 bytes - if it is encoded in multiple TLS records, the issue is successfully reproduced. As for reproducing the issue in a context where it matters enough to cause SQL Server to disconnect the client, a test project is available in the original SqlClient issue. From there, all you need is to connect to SQL Server with TLS configured and enforced and make a query using a string longer than 956 characters (UTF-16 doubles the byte size + header overhead), resulting in a TDS RPC request longer than 2048 bytes over the wire. Upon receiving the message split in two TLS records, SQL Server drops the connection:

Expected behavior

SslStream.Write should send input buffers in the smallest number of output TLS records by default, resizing internal buffers accordingly, and taking into account the maximum TLS record data size (16384). This means that calling SslStream.Write with a buffer of 10000 bytes should result in a single TLS record even if internal buffer sizes were initialized to a smaller size. As long as we try sending the input buffers as complete TLS records instead of splitting them, edge cases like MS-TDS RPC requests sent to SQL Server should work.

Actual behavior

SslStream.Write with input buffers larger than 2048 bytes are automatically split and sent in multiple TLS records, breaking long SQL queries made with SqlClient on Android.

Regression?

This used to work in the old Xamarin.Android runtime prior to .NET Core (yes, the ancient one!), but since we've been waiting for .NET 6,7,8 for other SslStream-related Android issues to be fixed, we only noticed this particular regression after finally switching to .NET 8.

Known Workarounds

Increasing the internal initial buffer size from 2048 to 4096 is sufficient to work around the issue, and we have managed to inject a patched System.Net.Security.dll in our builds as a temporary solution. However, it's far from ideal and an upstream fix is needed in the .NET runtime, even if it means just increasing the initial buffer size upstream as a quick fix.

# https://github.com/dotnet/SqlClient/issues/2141
$SystemNetSecurityDllZipUrl = "https://github.com/dotnet/SqlClient/files/13468922/System.Net.Security.dll.zip"
Invoke-WebRequest -Uri $SystemNetSecurityDllZipUrl -OutFile "System.Net.Security.dll.zip"
Expand-Archive "System.Net.Security.dll.zip" -DestinationPath .
$AndroidRuntimePrefix = "C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Runtime.Mono.android"
Get-Item "${AndroidRuntimePrefix}-*\8.0.0\runtimes\android-*\lib\net8.0\System.Net.Security.dll" | % {
  $BackupFile = ($_.FullName + ".bak")
  Move-Item $_ $BackupFile -Force
  Copy-Item "System.Net.Security.dll" $_ -Force
  @($_, $BackupFile) | % { Get-FileHash $_ } | Format-List
}

Configuration

.NET 8 for Android - both x64 and ARM64. The same code using SqlClient on all other platforms (iOS, Windows, macOS, Linux) is unaffected by this issue.

Other information

A lot of additional information is present in the original SqlClient issue.

The text was updated successfully, but these errors were encountered:

ghost · 2023-11-27T21:07:37Z

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

Issue Details

Description

The Android SslStream implementation using the Android SSLEngine APIs splits application data over 2048 bytes in multiple TLS records, something which normally isn't important, except for SQL Server which expects TDS RPC requests (query messages) to be sent in a single TLS record, breaking Microsoft.Data.SqlClient on Android. This issue only affects SslStream Android implementation, all other platforms resize internal buffers to send larger TLS records that can be safely fragmented at the transport level instead.

Reproduction Steps

Write a simple Android project that makes a TLS connection to a service and send a message of size larger than 2048 bytes - if it is encoded in multiple TLS records, the issue is successfully reproduced. As for reproducing the issue in a context where it matters enough to cause SQL Server to disconnect the client, a test project is available in the original SqlClient issue. From there, all you need is to connect to SQL Server with TLS configured and enforced and make a query using a string longer than 956 characters (UTF-16 doubles the byte size + header overhead), resulting in a TDS RPC request longer than 2048 bytes over the wire. Upon receiving the message split in two TLS records, SQL Server drops the connection:

Expected behavior

SslStream.Write should send input buffers in the smallest number of output TLS records by default, resizing internal buffers accordingly, and taking into account the maximum TLS record data size (16384). This means that calling SslStream.Write with a buffer of 10000 bytes should result in a single TLS record even if internal buffer sizes were initialized to a smaller size. As long as we try sending the input buffers as complete TLS records instead of splitting them, edge cases like MS-TDS RPC requests sent to SQL Server should work.

Actual behavior

SslStream.Write with input buffers larger than 2048 bytes are automatically split and sent in multiple TLS records, breaking long SQL queries made with SqlClient on Android.

Regression?

This used to work in the old Xamarin.Android runtime prior to .NET Core (yes, the ancient one!), but since we've been waiting for .NET 6,7,8 for other SslStream-related Android issues to be fixed, we only noticed this particular regression after finally switching to .NET 8.

Known Workarounds

Increasing the internal initial buffer size from 2048 to 4096 is sufficient to work around the issue, and we have managed to inject a patched System.Net.Security.dll in our builds as a temporary solution. However, it's far from ideal and an upstream fix is needed in the .NET runtime, even if it means just increasing the initial buffer size upstream as a quick fix.

# https://github.com/dotnet/SqlClient/issues/2141
$SystemNetSecurityDllZipUrl = "https://github.com/dotnet/SqlClient/files/13468922/System.Net.Security.dll.zip"
Invoke-WebRequest -Uri $SystemNetSecurityDllZipUrl -OutFile "System.Net.Security.dll.zip"
Expand-Archive "System.Net.Security.dll.zip" -DestinationPath .
$AndroidRuntimePrefix = "C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Runtime.Mono.android"
Get-Item "${AndroidRuntimePrefix}-*\8.0.0\runtimes\android-*\lib\net8.0\System.Net.Security.dll" | % {
  $BackupFile = ($_.FullName + ".bak")
  Move-Item $_ $BackupFile -Force
  Copy-Item "System.Net.Security.dll" $_ -Force
  @($_, $BackupFile) | % { Get-FileHash $_ } | Format-List
}

Configuration

.NET 8 for Android - both x64 and ARM64. The same code using SqlClient on all other platforms (iOS, Windows, macOS, Linux) is unaffected by this issue.

Other information

A lot of additional information is present in the original SqlClient issue.

Author:	awakecoding
Assignees:	-
Labels:	`area-System.Net.Security`, `untriaged`
Milestone:	-

ghost · 2023-11-27T21:19:00Z

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

Issue Details

Description

The Android SslStream implementation using the Android SSLEngine APIs splits application data over 2048 bytes in multiple TLS records, something which normally isn't important, except for SQL Server which expects TDS RPC requests (query messages) to be sent in a single TLS record, breaking Microsoft.Data.SqlClient on Android. This issue only affects SslStream Android implementation, all other platforms resize internal buffers to send larger TLS records that can be safely fragmented at the transport level instead.

Reproduction Steps

Write a simple Android project that makes a TLS connection to a service and send a message of size larger than 2048 bytes - if it is encoded in multiple TLS records, the issue is successfully reproduced. As for reproducing the issue in a context where it matters enough to cause SQL Server to disconnect the client, a test project is available in the original SqlClient issue. From there, all you need is to connect to SQL Server with TLS configured and enforced and make a query using a string longer than 956 characters (UTF-16 doubles the byte size + header overhead), resulting in a TDS RPC request longer than 2048 bytes over the wire. Upon receiving the message split in two TLS records, SQL Server drops the connection:

Expected behavior

SslStream.Write should send input buffers in the smallest number of output TLS records by default, resizing internal buffers accordingly, and taking into account the maximum TLS record data size (16384). This means that calling SslStream.Write with a buffer of 10000 bytes should result in a single TLS record even if internal buffer sizes were initialized to a smaller size. As long as we try sending the input buffers as complete TLS records instead of splitting them, edge cases like MS-TDS RPC requests sent to SQL Server should work.

Actual behavior

SslStream.Write with input buffers larger than 2048 bytes are automatically split and sent in multiple TLS records, breaking long SQL queries made with SqlClient on Android.

Regression?

This used to work in the old Xamarin.Android runtime prior to .NET Core (yes, the ancient one!), but since we've been waiting for .NET 6,7,8 for other SslStream-related Android issues to be fixed, we only noticed this particular regression after finally switching to .NET 8.

Known Workarounds

Increasing the internal initial buffer size from 2048 to 4096 is sufficient to work around the issue, and we have managed to inject a patched System.Net.Security.dll in our builds as a temporary solution. However, it's far from ideal and an upstream fix is needed in the .NET runtime, even if it means just increasing the initial buffer size upstream as a quick fix.

# https://github.com/dotnet/SqlClient/issues/2141
$SystemNetSecurityDllZipUrl = "https://github.com/dotnet/SqlClient/files/13468922/System.Net.Security.dll.zip"
Invoke-WebRequest -Uri $SystemNetSecurityDllZipUrl -OutFile "System.Net.Security.dll.zip"
Expand-Archive "System.Net.Security.dll.zip" -DestinationPath .
$AndroidRuntimePrefix = "C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Runtime.Mono.android"
Get-Item "${AndroidRuntimePrefix}-*\8.0.0\runtimes\android-*\lib\net8.0\System.Net.Security.dll" | % {
  $BackupFile = ($_.FullName + ".bak")
  Move-Item $_ $BackupFile -Force
  Copy-Item "System.Net.Security.dll" $_ -Force
  @($_, $BackupFile) | % { Get-FileHash $_ } | Format-List
}

Configuration

.NET 8 for Android - both x64 and ARM64. The same code using SqlClient on all other platforms (iOS, Windows, macOS, Linux) is unaffected by this issue.

Other information

A lot of additional information is present in the original SqlClient issue.

Author:	awakecoding
Assignees:	-
Labels:	`area-System.Net.Security`, `os-android`, `untriaged`
Milestone:	-

vcsjones · 2023-11-27T21:23:30Z

cc @simonrozsival

awakecoding · 2023-11-27T21:57:51Z

I spent most of the day digging into the Android .NET SslStream implementation and the corresponding Android SSLEngine API implementation. The quickest fix by far is just to increase the initial buffer size, as I'm a bit lost in all the buffer memory management that's being done.

The SSLEngine docs mention the BUFFER_OVERFLOW result for the Wrap() calls, but only clarifies how it is used for the Unwrap() calls. I found it easier to understand how it truly works by looking at the Android source code implementation of Wrap, especially the part that checks for the output buffer size to return BUFFER_OVERFLOW:

int dataLength = (int) min(BufferUtils.remaining(srcs), SSL3_RT_MAX_PLAIN_LENGTH);
if (dst.remaining() < calculateOutNetBufSize(dataLength)) {
    return new SSLEngineResult(
        Status.BUFFER_OVERFLOW, getHandshakeStatusInternal(), 0, 0);
}

I adapted the internal calculateOutNetBufSize function for C:

static int32_t GetNetOutBufferSize(JNIEnv* env, int32_t pendingBytes)
{
    int32_t SSL3_RT_MAX_PACKET_SIZE = 16709; // SSLSession.getPacketBufferSize()
    int32_t MAX_ENCRYPTION_OVERHEAD_LENGTH = 15 + 48 + 1 + 16 + 1 + 2 + 2 + 1;
    int32_t MAX_ENCRYPTION_OVERHEAD_DIFF = INT32_MAX - MAX_ENCRYPTION_OVERHEAD_LENGTH;

    return min(SSL3_RT_MAX_PACKET_SIZE,
        MAX_ENCRYPTION_OVERHEAD_LENGTH + min(MAX_ENCRYPTION_OVERHEAD_DIFF, pendingBytes));
}

This would be useful to predict the packet output buffer size from the input buffer size, and re-allocate internal buffers accordingly to proactively avoid BUFFER_OVERFLOW. One thing that's a bit confusing to me is that the Android source code appears to not produce a TLS record when returning BUFFER_OVERFLOW, so I'm not entirely sure how we end up with 2048 chunks when called from the .NET runtime. Normally, it would reallocate and try again with a larger buffer size, unless I missed something. The DoWrap() code in the .NET runtime also just eats the BUFFER_OVERFLOW result to return SSLStreamStatus_OK instead. It's a bit difficult to understand how it works in the end just by looking at DoWrap.

If we go one level higher in the call stack, AndroidCryptoNative_SSLStreamWrite is also quite important for the internal buffer management:

PAL_SSLStreamStatus AndroidCryptoNative_SSLStreamWrite(SSLStream* sslStream, uint8_t* buffer, int32_t length)
{
    abort_if_invalid_pointer_argument (sslStream);

    JNIEnv* env = GetJNIEnv();
    PAL_SSLStreamStatus ret = SSLStreamStatus_Error;

    // int remaining = appOutBuffer.remaining();
    // int arraySize = length > remaining ? remaining : length;
    // byte[] data = new byte[arraySize];
    int32_t remaining = (*env)->CallIntMethod(env, sslStream->appOutBuffer, g_ByteBufferRemaining);
    int32_t arraySize = length > remaining ? remaining : length;
    jbyteArray data = make_java_byte_array(env, arraySize);

    int32_t written = 0;
    while (written < length)
    {
        int32_t toWrite = length - written > arraySize ? arraySize : length - written;
        (*env)->SetByteArrayRegion(env, data, 0, toWrite, (jbyte*)(buffer + written));

        // appOutBuffer.put(data, 0, toWrite);
        IGNORE_RETURN((*env)->CallObjectMethod(env, sslStream->appOutBuffer, g_ByteBufferPutByteArrayWithLength, data, 0, toWrite));
        ON_EXCEPTION_PRINT_AND_GOTO(cleanup);
        written += toWrite;

        int handshakeStatus;
        ret = DoWrap(env, sslStream, &handshakeStatus);
        if (ret != SSLStreamStatus_OK)
        {
            goto cleanup;
        }
        else if (IsHandshaking(handshakeStatus))
        {
            ret = SSLStreamStatus_Renegotiate;
            goto cleanup;
        }
    }

cleanup:
    (*env)->DeleteLocalRef(env, data);
    return ret;
}

This function takes the raw input buffer bytes and length, but then creates a temporary 'data' jbyteArray to transfer those input bytes into sslStream->appOutBuffer. The crucial part here is probably that we max the temporary buffer size based on the number of bytes remaining in sslStream->appOutBuffer (int32_t remaining = (*env)->CallIntMethod(env, sslStream->appOutBuffer, g_ByteBufferRemaining);). This means that if the initial buffer size is 2048, we don't grow that buffer prior to calling DoWrap.

However, sslStream->appOutBuffer is just the application data buffer, we would also need to grow sslStream->netOutBuffer accordingly, and including sufficient space for the TLS overhead, before the actual call to SSLEngineWrap made at the beginning of DoWrap:

    jobject result = (*env)->CallObjectMethod(
        env, sslStream->sslEngine, g_SSLEngineWrap, sslStream->appOutBuffer, sslStream->netOutBuffer);

And in order to predict the TLS maximum overhead bytes, we may have no choice but to use some of the same constants defined in the SSLEngine source code, as they are not properly exposed through the Android SSLEngine APIs. This could be done using the GetNetOutBufferSize adapted function defined earlier in this post.

This is my analysis so far, I couldn't find a clear way to fix it, but it should save some time to the person that knows this code a lot better than I do in the .NET runtime team.

simonrozsival · 2023-11-27T22:10:10Z

@awakecoding thanks for the thorough report! I will try looking into it tomorrow.

wfurt · 2023-11-27T23:23:47Z

we may build up on #87874. That one allows to allocate as needed - but I'm not sure if that works for Android. Increasing the InitialBufferSize is not great IMHO as it will make it more expensive for every single connection.

last-Programmer · 2023-11-28T10:02:45Z

I am using SMO and i need to Increase the internal initial buffer size from 2048 to 8192 to get it working.

simonrozsival · 2024-07-19T09:23:44Z

The fix should be soon available in .NET 9 Preview 7

dotnet-issue-labeler bot added the area-System.Net.Security label Nov 27, 2023

ghost added the untriaged New issue has not been triaged by the area owner label Nov 27, 2023

awakecoding mentioned this issue Nov 27, 2023

A transport-level error has occurred when receiving results from the server if query length exceeds 956 characters dotnet/SqlClient#2141

Closed

vcsjones added the os-android label Nov 27, 2023

wfurt removed the untriaged New issue has not been triaged by the area owner label Nov 29, 2023

wfurt added this to the 9.0.0 milestone Nov 29, 2023

steveisok assigned simonrozsival Dec 8, 2023

simonrozsival mentioned this issue Jun 3, 2024

[release/8.0-staging] Fix calculation of channel bindings hash in managed NTLM implementation #102565

Merged

simonrozsival mentioned this issue Jul 11, 2024

[Android] Improve SslStream PAL buffer resizing #104726

Merged

dotnet-policy-service bot added the in-pr There is an active PR which will close this issue when it is merged label Jul 11, 2024

simonrozsival closed this as completed in #104726 Jul 19, 2024

simonrozsival mentioned this issue Sep 11, 2024

SSLStream write delay on Android #106885

Closed

github-actions bot locked and limited conversation to collaborators Oct 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Android SslStream splits messages over 2048 bytes in multiple TLS records (breaks MS-TDS protocol) #95295

Android SslStream splits messages over 2048 bytes in multiple TLS records (breaks MS-TDS protocol) #95295

awakecoding commented Nov 27, 2023

ghost commented Nov 27, 2023

Description

Reproduction Steps

Expected behavior

Actual behavior

Regression?

Known Workarounds

Configuration

Other information

ghost commented Nov 27, 2023

Description

Reproduction Steps

Expected behavior

Actual behavior

Regression?

Known Workarounds

Configuration

Other information

vcsjones commented Nov 27, 2023

awakecoding commented Nov 27, 2023

simonrozsival commented Nov 27, 2023

wfurt commented Nov 27, 2023

last-Programmer commented Nov 28, 2023

simonrozsival commented Jul 19, 2024

Android SslStream splits messages over 2048 bytes in multiple TLS records (breaks MS-TDS protocol) #95295

Android SslStream splits messages over 2048 bytes in multiple TLS records (breaks MS-TDS protocol) #95295

Comments

awakecoding commented Nov 27, 2023

Description

Reproduction Steps

Expected behavior

Actual behavior

Regression?

Known Workarounds

Configuration

Other information

ghost commented Nov 27, 2023

Description

Reproduction Steps

Expected behavior

Actual behavior

Regression?

Known Workarounds

Configuration

Other information

ghost commented Nov 27, 2023

Description

Reproduction Steps

Expected behavior

Actual behavior

Regression?

Known Workarounds

Configuration

Other information

vcsjones commented Nov 27, 2023

awakecoding commented Nov 27, 2023

simonrozsival commented Nov 27, 2023

wfurt commented Nov 27, 2023

last-Programmer commented Nov 28, 2023

simonrozsival commented Jul 19, 2024