Investigate AES-GCM divergence, tag

[Archprogrammer]

Following up on issue #11

The following code:

    List<int> key128bit=[
      122,140,235,87,
      10,140,37,178,
      172,182,154,240,
      211,126,138,11
    ];
    List<int> encodeIV=[
      0,0,0,0,
      1,1,1,1,
      2,2,2,2
    ];
    String plaintext='a sample plaintext';   // utf-8 encoded in AesCrypt.encrypt()

    AesCrypt encrypter=AesCrypt(
      mode: ModeAES.gcm,
      padding: PaddingAES.pkcs7,   // Setting this to "none" results in an exception
      key: base64.encode(key128bit)
    );

    String result=encrypter.encrypt(plaintext, iv: base64.encode(encodeIV));

    dev.log("plain.length=${plaintext.length}");
    dev.log("result.length=${base64.decode(result).length}");
    dev.log(base64.decode(result).toString());

Results in the output :

[log] plain.length=18
[log] result.length=32
[log] [57, 126, 210, 236, 146, 205, 233, 139, 16, 45, 193, 146, 164, 250, 98, 165, 147, 120, 153, 51, 116, 146, 11, 111, 218, 175, 8, 183, 211, 164, 245, 44]

Encrypting the same string (in utf-8) with the same key and the same IV in Firefox using subtle.crypto (but setting tag-length to 96 bits) results in the following byte sequence:

[ 57 126 210 236 146 205 233 139 16 45 193 146 164 250 98 165 147 120  73 255 75 137 95 252 156 90 215 157 98 122 ]

The sequences match for the first 18 bytes after which they diverge - if I understand it correctly, they should be identical (to the shortest length).

Furthermore, as referenced in #11 , an input string of 88 bytes resulted in a 64-bit tag (8 bytes) but this sample code results in a 112-bit tag (14 bytes).

To sum it up - there appears to be two major and one minor problem:

1. The AES-GCM tag appears garbled.
2. There is no way (currently) to set the tag length.
3. (minor) PaddingAES.none results in an exception.

If I can provide any more information or help in any other way, just ask.

[AKushWarrior]

I'll investigate this.

I think that using PaddedBlockCipher for AES-GCM is messing up the encryption. I'm not as sure about the tag length, but my guess is that that can be fixed as well.

PaddingAES.none is something I wanted to avoid (runtime-errors are more frequent; lib harder to use) but, as you said, not having it appears to be causing more trouble.

[AKushWarrior]

Okay, I mostly figured this out:

Internally, in our fork of PointyCastle, tagSize is represented as macSize:

steel_crypt/lib/PointyCastleN/src/api/aead_parameters.dart

Line 13 in fdf3b2c

final int macSize;

This code is fine.

To conform with the unified BlockCipher interface, we do some things internally:

steel_crypt/lib/PointyCastleN/src/impl/base_aead_block_cipher.dart

Lines 131 to 181 in fdf3b2c

	@override
	Uint8List process(Uint8List data) {
	var out = Uint8List(_getOutputSize(data.length));
	var len = processBytes(data, 0, data.length, out, 0);
	len += doFinal(out, len);
	return Uint8List.view(out.buffer, 0, len);
	}
	@override
	int processBytes(
	Uint8List inp, int inpOff, int len, Uint8List out, int outOff) {
	if (len == 0) return 0;
	if (forEncryption) {
	// all bytes are plain text bytes
	return _processCipherBytes(inp, inpOff, len, out, outOff);
	}
	// last macSize bytes are possibly mac bytes and not cipher text bytes
	// -> keep them in buffer
	var cipherLen = _lastMacSizeBytesOff + len - macSize;
	var resultLen = 0;
	if (cipherLen > 0 && _lastMacSizeBytesOff > 0) {
	// at least part of the buffer are actually cipher text bytes
	// process them and update the buffer
	var l = min(_lastMacSizeBytesOff, cipherLen);
	resultLen += _processCipherBytes(_lastMacSizeBytes, 0,
	min(_lastMacSizeBytesOff, cipherLen), out, outOff);
	outOff += resultLen;
	cipherLen -= l;
	_lastMacSizeBytes.setRange(0, macSize - l, _lastMacSizeBytes.skip(l));
	_lastMacSizeBytesOff -= l;
	}
	if (cipherLen > 0) {
	// part of the input are cipher text bytes
	resultLen += _processCipherBytes(inp, inpOff, cipherLen, out, outOff);
	}
	_lastMacSizeBytes.setRange(_lastMacSizeBytesOff,
	_lastMacSizeBytesOff + len - cipherLen, inp.skip(inpOff + cipherLen));
	_lastMacSizeBytesOff += len - cipherLen;
	return resultLen;
	}

This code is also fine.

And then, the actual issue resides here:

steel_crypt/lib/src/encoded/aes.dart

Line 92 in fdf3b2c

AEADParameters(KeyParameter(key), 128, ivLocal, aadLocal);

While this is mainly to ensure ease of use (auto-setting the tag to 128 bits, the most secure length), it also screws up anyone who needs the flexibility.

I'm thinking that some serious rearchitecture of AesCrypt is necessary. It just doesn't make sense to abstract GCM and CBC and CTR and ECB in the same method. It creates classes of bugs like this one (which are mostly avoidable).

A better way to do this would be:

var aes = AesCrypt(key); //Usable for literally anything at this point.
print(aes.gcm.encrypt(inp: 'words', iv: iv, tagLength: tl, aad: aad)); //GCM encrypt.
print(aes.ecb.encrypt(inp: 'words')); //ECB encrypt

This bug will be fixed and the new architecture will be implemented in the next commit.

[Archprogrammer]

This sounds excellent - thank you for the quick reply as well!

Will this result in a 2.0.4 (beta) release?

[AKushWarrior]

It will, yeah. I can probably have it done by mid-week.

[Archprogrammer]

Sorry to bug you, but any update on this or a timeframe for an initial attempt?

[AKushWarrior]

@Archprogrammer Woah sorry it's been a while. I was wrapped up in this test suite I've been creating.

I'll try to have it out by the end of this week.

[Archprogrammer]

Just me nagging again. :)

Any progress on this issue? Is there anything I can do to help out?

[AKushWarrior]

Ugh. I'm really sorry about this. I've been wrapped up in some other things professionally which are kind of a time sink.

Regardless, I'll try to get this update out today.

[AKushWarrior]

Alright, this feature is actually complete and ready to publish. I'm working on a CMac bug right now; once that's done I'll publish this.

[AKushWarrior]

@Archprogrammer done with the latest commit and published to pub. See example/example.dart to see the new syntax. You can put in tagLength and aad as named parameters (e.g. the syntax shown above.)

[Archprogrammer]

I'm afraid this doesn't work quite as intended. Testing the new version runs into this problem:

https://github.com/bcgit/pc-dart/blob/52ef0aea5303466c015463a61d8b193994490d78/lib/block/modes/gcm.dart#L37

macSize is required to be 16 in pointycastle.

However - the resulting output looks correct, so it's possible to set the tagSize to 128 bits and just chop off the last 4 bytes to get a 96-bit tag even if this is a bit cumbersome.

Without examining the code further, I'd hazard a guess that the easiest way to support a tagLength argument would be to implement it in steel_crypt as a post-encryption step?

[AKushWarrior]

macSize is required to be 16 in pointycastle.

However - the resulting output looks correct, so it's possible to set the tagSize to 128 bits and just chop off the last 4 bytes to get a 96-bit tag even if this is a bit cumbersome.

Does the output come out to the correct thing if you truncate the last 32 bits? This is pretty minimal overhead for me to implement, so I think I could probably do this.

[Archprogrammer]

Yes, that is a property of the tag - a shorter tag is a prefix of a longer tag. You can always truncate the tag to the wanted length if you have a longer one and the resulting, shorter tag, will be correct. At least that is how I understand it.

[AKushWarrior]

Yes, that is a property of the tag - a shorter tag is a prefix of a longer tag. You can always truncate the tag to the wanted length if you have a longer one and the resulting, shorter tag, will be correct. At least that is how I understand it.

Cool. I'll publish a hotfix for this in a few hours.

[AKushWarrior]

@Archprogrammer I tried to implement this, but decryption doesn't work:

Uint8List encrypt({@required Uint8List inp, @required Uint8List iv, Uint8List aad, int tagLength = 128}) {
    dynamic params = (padding == PaddingAES.none)
        ? AEADParameters(KeyParameter(key), 128, iv, aad)
        : PaddedBlockCipherParameters(
            AEADParameters(KeyParameter(key), 128, iv, aad), null);
    var cipher = (padding == PaddingAES.none)
        ? GCMBlockCipher(AESFastEngine())
        : PaddedBlockCipher('AES/GCM/' + parsePadding(padding));
    cipher.init(true, params);
    return cipher.process(inp).sublist(0, tagLength ~/ 8);
  }

  Uint8List decrypt({@required Uint8List enc, @required Uint8List iv, Uint8List aad, int tagLength = 128}) {
    dynamic params = (padding == PaddingAES.none)
        ? AEADParameters(KeyParameter(key), 128, iv, aad)
        : PaddedBlockCipherParameters(
            AEADParameters(KeyParameter(key), 128, iv, aad), null);
    var cipher = (padding == PaddingAES.none)
        ? GCMBlockCipher(AESFastEngine())
        : BlockCipher('AES/GCM/' + parsePadding(padding));
    cipher.init(false, params);

    return cipher.process(enc);
  }

This is some code that I'm testing (from GcmSatelliteRaw). It fails during decryption because PointyCastle expects an input length that is a multiple of the block size; I may have to "re-pad" the input, but I'm not sure how to go about that.

[AKushWarrior]

I think it's easier to file this as an issue upstream than to try and deal with it here. It's clear that this is a pointycastle issue, not really one with this library. I'll leave this open as a marker for the upstream issue (which I will file when I get around to it).

[Archprogrammer]

Sounds like a good plan - I can adjust the tag outside of steel_crypt for now since I only need to encrypt at the moment, so this works for me right now. Hopefully this can be solved in PointyCastle soon.

Thanks for the help!