eddelbuettel / digest

R package to create compact hash digests of R objects
https://eddelbuettel.github.io/digest
111 stars 47 forks source link

AES objects need to be re-created after encryption #188

Closed daattali closed 1 year ago

daattali commented 1 year ago

When creating an AES object, you get an object with an encrypt() and a decrypt() function. It's very intuitive to try to encrypt and decrypt using the same object, or try to encrypt multiple times using the same object. However, that's not possible unless you use mode "ECB".

Example:

> aes <- AES(as.raw(1:16), "CBC", as.raw(1:16))
> cipher <- aes$encrypt(as.raw(1:16))
> print(aes$decrypt(cipher, raw = TRUE))
 [1] db f1 84 11 2e b9 11 16 59 71 2b af cf f2 ab 24
> print(AES(as.raw(1:16), "CBC", as.raw(1:16))$decrypt(cipher, raw = TRUE))
 [1] 01 02 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f 10

The first time when you try to decrypt using the original AES object, you get the wrong thing. You must create a new object for decryption, which is not convenient.

Another weird thing happens if you try to encrypt using the same object twice, you'd expect to get the same output but you don't:

> aes <- AES(as.raw(1:16), "CBC", as.raw(1:16))
> aes$encrypt(as.raw(1:16))
 [1] db f1 84 11 2e b9 11 16 59 71 2b af cf f2 ab 24
> aes$encrypt(as.raw(1:16))
 [1] 63 23 c0 80 2f 33 29 6a 6a 3b c3 c7 de 64 1d 0a
> aes$encrypt(as.raw(1:16))
 [1] 36 5a 25 94 d4 86 af 06 9e 60 80 26 6d 69 89 72
> aes$encrypt(as.raw(1:16))
 [1] 0d cf d7 29 ad f4 0b ca 42 c4 42 7f cb b4 3b 51
daattali commented 1 year ago

I understand these functions were contributed -- do you know why this is the case? Does it need to be the case? Would it be ok if I try to "fix" this so that the object can be re-used? This will technically be a breaking change.

eddelbuettel commented 1 year ago

How about using the publically available information (ChangeLog, git commit, ...) to identify the contribution author and contacting him? That is pretty much what the 'social coding' aspect for git is for, no?

It looks like

daattali commented 1 year ago

I was wondering if as the author/maintainer you were involved in the decision at all.

@dmurdoch can you please chime in

eddelbuettel commented 1 year ago

Also see #185 as @daattali by now scattered comments over multiple issues and PRs, I will leave up to him to make his case as concisely and convincingly as he can :wink:

daattali commented 1 year ago

I apologize for the "spamming", I've opened mutliple issues as I thought that would be the cleanest path, to not aggregate issues.

185 does not need to be consulted as it was a separate issue, the description in this issue is complete on its own.

dmurdoch commented 1 year ago

The encryption modes other than ECB are designed to be stateful, so the second complaint (encrypting the same thing twice gives different results) is a feature, not a bug.

The first complaint (you can't encrypt then decrypt with the same object) could be worked around by keeping two copies of the internal state, one which is updated each time you encrypt and the other updated on each decryption, but that's not what the motivating implementation does, and I don't think most users would need that. If you do, you could create your own wrapper that holds two AES objects and directs encryption to one and decryption to the other, so strictly speaking a change to digest isn't needed.

I think it would be a serious error to make any breaking change that violates the NIST standard listed in the references. I can't think of what sort of breaking change you were thinking of that wouldn't do that.

daattali commented 1 year ago

Thanks for explaining. Closing as per your explanation

eddelbuettel commented 1 year ago

Thank you both. I actually agree with everything :clown_face: as it is a) indeed nice to have operations be 'idem-potent' and b) nice to be immediately reversible but as explained c) "cannot always happen". Strongly agree that adhering to the published test suites is key too.

There is a bit of pending work in the package (in that I found / experimented with a clever way to have crc32c both as a basic fallback and as an optional hardware-accelerated Suggests) but I can refine that later too. So I'll look into shipping what we have to CRAN "real soon".