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

Add Mac: CMAC ("AES/CMAC") #174

Merged
merged 17 commits into from Jul 19, 2019

Conversation

Projects
None yet
2 participants
@paulreimer
Copy link
Contributor

commented Mar 1, 2019

Based on BouncyCastle sources, using the test vectors from the AES-CMAC RFC:
https://tools.ietf.org/html/rfc4493

Those test vectors are binary plaintext, so I refactored _runMacTest to use accept UInt8List.

I'm not strong in Java, Dart, or crypto, please review accordingly.

paulreimer added some commits Mar 1, 2019

_cipher.reset();

// Must be done after reset
_cipher.init(true, _params);

This comment has been minimized.

Copy link
@paulreimer

paulreimer Mar 1, 2019

Author Contributor

This seems weird to me, but it made the test vectors pass.

paulreimer added some commits Mar 1, 2019

*
* @param cipher the cipher to be used as the basis of the MAC generation.
*/
// CMac(BlockCipher cipher) : CMac(this._cipher, _cipher.blockSize * 8);

This comment has been minimized.

Copy link
@paulreimer

paulreimer Mar 1, 2019

Author Contributor

Not sure how to do multiple constructors in Dart...

paulreimer added some commits Mar 1, 2019

@paulreimer

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2019

This PR requires #175 (ISO7816-4 padding) to be merged first.

@paulreimer

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2019

I don't know why there is a separate macs/CBCBlockCipherMac and macs/CMac class in BouncyCastle, with very similar (but slightly different) implementations. I'm porting the other one as well, and I will add it to this PR in the same manner that BouncyCastle has it (that is, two nearly identical algorithms in the same macs/ folder).

I'll note that that my goal is to get AES-CCM working (by porting BouncyCastle's CCMBlockCipher.java), and that one uses the CBCBlockCipherMac. Other than that, I'm not sure which one is best, or just to ship them both?

paulreimer added some commits Mar 2, 2019

// reset the underlying cipher.
_cipher.reset();

_cipher.init(true, _params);

This comment has been minimized.

Copy link
@paulreimer

paulreimer Mar 2, 2019

Author Contributor

Again, this seems weird to me, but storing _params and re-initializing the key seems to be necessary. Maybe there's a better place to put it (e.g. just after every reset call site?)

PlainTextDigestPair(
Uint8List.fromList([]), "bb1d6929e95937287fa37d129b756746"),
PlainTextDigestPair(
Uint8List.fromList([

This comment has been minimized.

Copy link
@paulreimer

paulreimer Mar 2, 2019

Author Contributor

I could change this to createUint8ListFromString?

This comment has been minimized.

Copy link
@stevenroose

stevenroose Jul 10, 2019

Member

Whatever makes it easier to compare with the upstream vectors.

paulreimer added some commits Mar 2, 2019

@override
String get algorithmName {
String paddingName = _padding != null ? "/${_padding.algorithmName}" : "";
return "${_cipher.algorithmName}_CMAC${paddingName}";

This comment has been minimized.

Copy link
@stevenroose

stevenroose Jul 10, 2019

Member

With an underscore?

This comment has been minimized.

Copy link
@paulreimer

paulreimer Jul 10, 2019

Author Contributor

I think this is the nit, and you’d expect a slash? I think there would be a naming conflict with the other CMAC algorithmName. I’m not sure why there are two slightly different CMAC implementations in BouncyCastle, or why they don’t pass the same test vectors (with default parameters, at least)

If you think it is appropriate to include both, how would you like to see the naming?

This comment has been minimized.

Copy link
@stevenroose

stevenroose Jul 19, 2019

Member

Meh, I'm not sure here anyway. I'll just take your word for it :)

@stevenroose

This comment has been minimized.

Copy link
Member

commented Jul 10, 2019

Sorry for the late review. I was hoping someone who actually uses this library could perhaps review this..

It looks quite good, even though I didn't verify the validity of the test vectors.

I found one nit, once addressed, I can merge this.

@stevenroose stevenroose merged commit d2f2a8b into PointyCastle:master Jul 19, 2019

@stevenroose

This comment has been minimized.

Copy link
Member

commented Jul 19, 2019

Thanks for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.