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

[C#] AES CMAC implementation crashes with certain plain text len #187

Closed
tanascius opened this Issue Aug 1, 2018 · 6 comments

Comments

Projects
None yet
4 participants
@tanascius

tanascius commented Aug 1, 2018

Hello,
I think I found a bug. Please have a look at the following C# code:

var aead = Aead.CreateAesCmacSiv( masterKey );
aead.Seal( Encoding.UTF8.GetBytes( "123456789012345" ) ); // Works
aead.Seal( Encoding.UTF8.GetBytes( "1234567890123456" ) ); // Crashes
aead.Seal( Encoding.UTF8.GetBytes( "12345678901234567" ) ); // Works

Message: "Index out of range"
StackTrace: "Miscreant.Utils.Pad(Byte[] buffer, Int32 position)\r\n bei Miscreant.AesSiv.S2V(Byte[][] headers, Byte[] message)\r\n bei Miscreant.AesSiv.Seal(Byte[] plaintext, Byte[][] data)"

An aead.Open() will crash, too (e.g. when the seal with 16 chars is done with another implementation).
NuGet shows me a Miscreant version 0.3.1.

@tanascius tanascius changed the title from [C#] AES CMAC implementation crashes with non-ASCII characters to [C#] AES CMAC implementation crashes with certain plain text len Aug 1, 2018

@tarcieri

This comment has been minimized.

Show comment
Hide comment
@tarcieri

tarcieri Aug 1, 2018

Contributor

Any ideas here @Metalnem?

Contributor

tarcieri commented Aug 1, 2018

Any ideas here @Metalnem?

@bojanrajkovic

This comment has been minimized.

Show comment
Hide comment
@bojanrajkovic

bojanrajkovic Aug 1, 2018

The following test fails to pass with master:

[Fact]
public void CanSealWhenExactlyBlockSized()
{
	var rng = RandomNumberGenerator.Create();
	var key = new byte[32];
	rng.GetBytes(key);

	var cipher = Aead.CreateAesCmacSiv(key);
	var plainText = Encoding.UTF8.GetBytes("1234567890123456");
	var @sealed = cipher.Seal(plainText);

	Assert.NotNull(@sealed);

	var unsealed = cipher.Open(@sealed);
	Assert.Equal(plainText, unsealed);
}

The crash happens because Utils.Pad tries to insert an 0x80 at the 17th byte of a 16 byte array. The obvious fix is to not call Utils.Pad if message.Length is an even multiple of the block size, but that breaks other tests, and I don't understand the code well enough yet to ascertain the correct fix.

bojanrajkovic commented Aug 1, 2018

The following test fails to pass with master:

[Fact]
public void CanSealWhenExactlyBlockSized()
{
	var rng = RandomNumberGenerator.Create();
	var key = new byte[32];
	rng.GetBytes(key);

	var cipher = Aead.CreateAesCmacSiv(key);
	var plainText = Encoding.UTF8.GetBytes("1234567890123456");
	var @sealed = cipher.Seal(plainText);

	Assert.NotNull(@sealed);

	var unsealed = cipher.Open(@sealed);
	Assert.Equal(plainText, unsealed);
}

The crash happens because Utils.Pad tries to insert an 0x80 at the 17th byte of a 16 byte array. The obvious fix is to not call Utils.Pad if message.Length is an even multiple of the block size, but that breaks other tests, and I don't understand the code well enough yet to ascertain the correct fix.

@bojanrajkovic

This comment has been minimized.

Show comment
Hide comment
@bojanrajkovic

bojanrajkovic Aug 1, 2018

@tanascius It would probably help if you provide some example data of 16 bytes sealed with another implementation and the key used to seal it so a similar test could be written for opening data from another implementation.

bojanrajkovic commented Aug 1, 2018

@tanascius It would probably help if you provide some example data of 16 bytes sealed with another implementation and the key used to seal it so a similar test could be written for opening data from another implementation.

@bojanrajkovic

This comment has been minimized.

Show comment
Hide comment
@bojanrajkovic

bojanrajkovic Aug 1, 2018

Hmm, I think I have what appears to be, but is probably not, a correct fix: resize the array so that the 0x80 can be appended. This makes all the tests pass, including my new test.

Patch is at https://gist.github.com/bojanrajkovic/a474d6fd75620cc1a34ebd25373658f1, I can PR it if people who actually understand the code think it's correct. 😆

bojanrajkovic commented Aug 1, 2018

Hmm, I think I have what appears to be, but is probably not, a correct fix: resize the array so that the 0x80 can be appended. This makes all the tests pass, including my new test.

Patch is at https://gist.github.com/bojanrajkovic/a474d6fd75620cc1a34ebd25373658f1, I can PR it if people who actually understand the code think it's correct. 😆

@tanascius

This comment has been minimized.

Show comment
Hide comment
@tanascius

tanascius Aug 1, 2018

Thanks for caring about this issue.
Actually I did some tests, too. But like @bojanrajkovic I am not sure about my fix and whether it is correct. All tests and the reported issue are working with this fix:
https://gist.github.com/tanascius/4ada0012a42478bc39dc3c8ba3a38cd0

I have only Visual Studio 2015, so the project is not running without modifications when I check out. I am probably not really able to provide a useful PR :(

@bojanrajkovic I can't provide example data of 16 bytes sealed. The only data I have was created with a real cryptomator installation and I can't provide the masterkey and hmac for that volume here :-(

tanascius commented Aug 1, 2018

Thanks for caring about this issue.
Actually I did some tests, too. But like @bojanrajkovic I am not sure about my fix and whether it is correct. All tests and the reported issue are working with this fix:
https://gist.github.com/tanascius/4ada0012a42478bc39dc3c8ba3a38cd0

I have only Visual Studio 2015, so the project is not running without modifications when I check out. I am probably not really able to provide a useful PR :(

@bojanrajkovic I can't provide example data of 16 bytes sealed. The only data I have was created with a real cryptomator installation and I can't provide the masterkey and hmac for that volume here :-(

@Metalnem

This comment has been minimized.

Show comment
Hide comment
@Metalnem

Metalnem Aug 2, 2018

Contributor

Thanks all! The problem is now fixed (the fix @tanascius suggested was correct), and additional test was also added. New build (version 0.3.2) is published to NuGet, and should become visible probably in the next hour.

Contributor

Metalnem commented Aug 2, 2018

Thanks all! The problem is now fixed (the fix @tanascius suggested was correct), and additional test was also added. New build (version 0.3.2) is published to NuGet, and should become visible probably in the next hour.

@tarcieri tarcieri closed this in c61e70e Aug 3, 2018

tarcieri added a commit that referenced this issue Aug 3, 2018

Merge pull request #188 from Metalnem/master
Fix #187 and add test for encrypting block-sized data
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment