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

Append leftovers of FromBase64Transform's input buffer to the transform buffer #82148

Merged
merged 4 commits into from
Mar 23, 2023

Conversation

jozkee
Copy link
Member

@jozkee jozkee commented Feb 15, 2023

Fixes #76918,

As @gfoidl pointed out on #76918 (comment); we need to append _inputBuffer followed by the CryptoStream's inputBuffer to the transform buffer in order to properly decode. Currently we were only appending the latter and on cases where there was only 1 byte we were causing a IndexOutOfRangeException on GetOutputSize.

Could there be more severe bugs related to this issue that require additional test cases?

@ghost
Copy link

ghost commented Feb 15, 2023

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

Issue Details

Fixes #76918,

As @gfoidl pointed out on #76918 (comment); we need to append _inputBuffer followed by the CryptoStream's inputBuffer to the transformedBuffer in order to properly decode. Currently we were only appending the latter and on cases where there was only 1 byte we were causing a IndexOutOfRangeException on GetOutputSize.

Could there be more severe bugs related to this issue that require additional test cases?

Author: Jozkee
Assignees: Jozkee
Labels:

area-System.Security

Milestone: -

@vcsjones
Copy link
Member

vcsjones commented Feb 15, 2023

@jozkee these changes appear to have introduced a regression. I did some light fuzzing of these changes and distilled a repro down to:

[Fact]
public void vcsjones_repro()
{
    ICryptoTransform transform = new FromBase64Transform();
    string base64 = "s3 DhwGM/aBkdcMBqsXMLINDLvPH9htGe01DfPYYHB4P4mSdH42fDSo9kX5Z89hmBT8IlrDjvAkHcYMxTfwW3l8XUmKVossWvznTtByq5bIQwkc9WJu9om49Irk7PvATqbiHmcIH+KpPLWL1UBtXG3JKfWydpVwjGN/a2F+zUEYM=";
    byte[] input = System.Text.Encoding.ASCII.GetBytes(base64);
    byte[] destination = new byte[128];
    byte[] oneshot = Convert.FromBase64String(base64);
    int written = 0;
    int remaining = base64.Length;

    while (remaining > 0)
    {
        written += transform.TransformBlock(input, base64.Length - remaining, 1, destination, written);
        remaining--;
    }

    Assert.Equal(oneshot, destination);
}

In main, this does not throw an exception. In the PR, this will fall with:

Error Message:
   System.ArgumentException : Destination is too short. (Parameter 'destination')
  Stack Trace:
     at System.Security.Cryptography.FromBase64Transform.TransformBlock(Byte[] inputBuffer, Int32 inputOffset, Int32 inputCount, Byte[] outputBuffer, Int32 outputOffset) in /Users/vcsjones/Projects/runtime/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/Base64Transforms.cs:line 156
   at System.Security.Cryptography.Tests.Base64TransformsTests.fuzzcase() in /Users/vcsjones/Projects/runtime/src/libraries/System.Security.Cryptography/tests/Base64TransformsTests.cs:line 128
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
   at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr) in /Users/vcsjones/Projects/runtime/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs:line 59

We should fix and add some additional test cases since an existing test did not catch this.

EDIT: I cleaned up the test case to look a little more like an actual test someone would write and add an assertion.

@jozkee jozkee requested a review from vcsjones February 16, 2023 16:33
@jozkee
Copy link
Member Author

jozkee commented Feb 16, 2023

@vcsjones the problem was when inputBuffer was small enough to save it in the ICryptoTransform buffer in more than one call. Since now we append both buffers before "checking if bytesToTransform < InputBlockSize else save them", I just removed the AsSpan that was acting as a Slice that was causing the problem.

I also added a test for ReadOneByteAtTheTime with multiple sizes of inputBuffer.

@vcsjones
Copy link
Member

I haven't been able to break this with the latest changes, so, LGTM.

@@ -361,5 +367,27 @@ private static int ReadAll(Stream stream, Span<byte> buffer)

return totalRead;
}

[Theory, MemberData(nameof(TestData_All_Ascii))]
public void ReadOneByteAtTheTime(string expected, string data)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if we should also feed it byte by byte to be sure the incomplete input is handled properly.

Copy link
Member Author

@jozkee jozkee Mar 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is also feeding one byte at a time. On Line 384 inputCount argument is 1:

written += transform.TransformBlock(inputBytes, inputBytes.Length - remaining, 1, outputBytes, written);

Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
We should probably rebase it or sync up as it seems pretty old and there some some strange infrastructure failures.

@jozkee
Copy link
Member Author

jozkee commented Mar 23, 2023

CI issues are WASM specific. Thanks for the review @wfurt.

@jozkee jozkee merged commit 3e1abf4 into dotnet:main Mar 23, 2023
@jozkee jozkee deleted the security_76918 branch March 23, 2023 20:32
@ghost ghost locked as resolved and limited conversation to collaborators Apr 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IndexOutOfRangeException on decoding base64 bytes with LF characters included (>= .NET 5 only)
3 participants