Investigate AES-GCM divergence, tag about steel_crypt HOT 17 OPEN

@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.

from steel_crypt.

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.

from steel_crypt.

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.

from steel_crypt.

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.

from steel_crypt.

Okay, I mostly figured this out:

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

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

And then, the actual issue resides here:

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.

from steel_crypt.

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

Will this result in a 2.0.4 (beta) release?

from steel_crypt.

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

from steel_crypt.

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

from steel_crypt.

Just me nagging again. :)

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

from steel_crypt.

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.

from steel_crypt.

@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.)

from steel_crypt.

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?

from steel_crypt.

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.

from steel_crypt.

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.

from steel_crypt.

@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.

from steel_crypt.

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).

from steel_crypt.

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!

from steel_crypt.