Make decrypting with the wrong key easier to detect

[jennybc]

key1 <- openssl::rand_bytes(32)
key2 <- openssl::rand_bytes(32)
enc <- httr2::secret_encrypt("this is a secret", key1)
httr2::secret_decrypt(enc, key1)
#> [1] "this is a secret"
httr2::secret_decrypt(enc, key2)
#> Error in rawToChar(openssl::aes_ctr_decrypt(value, key, iv = iv)): embedded nul in string: '\xeci\b\xfcK\xdc\0\xb6\xa1&jp\xd2\xe0\xec@'

pth <- withr::local_tempfile()
httr2::secret_write_rds(head(iris), pth, key1)
httr2::secret_read_rds(pth, key1)
#>   Sepal.Length Sepal.Width Petal.Length Petal.Width Species
#> 1          5.1         3.5          1.4         0.2  setosa
#> 2          4.9         3.0          1.4         0.2  setosa
#> 3          4.7         3.2          1.3         0.2  setosa
#> 4          4.6         3.1          1.5         0.2  setosa
#> 5          5.0         3.6          1.4         0.2  setosa
#> 6          5.4         3.9          1.7         0.4  setosa
httr2::secret_read_rds(pth, key2)
#> Error in memDecompress(x_cmp, "bzip2"): internal error -5 in memDecompress(type = "bzip2")

If you use secret_decrypt() with the wrong key, you either get gibberish back or, sometimes, you get the error seen above about an embedded nul. Either way, it's not a great basis for a user figuring out what they did wrong.

I suspect the use of secret_read_rds() with the wrong key will (almost?) always error. But it's a cryptic error from memDecompress() and doesn't suggest what the user has done wrong.

It would be interesting to think about building some feature into the encrypted forms so that we are better able to detect the "wrong key" problem and throw a more informative error.

[hadley]

I think the simplest way to do this is to also store a hash of the encrypted value in secret_encrypt_raw(); that way we we decrypt we can check whether or not the hashes match. Unfortunately this means that we'll need to introduce some sort of versioning to the file format, producing a vector like c(version, iv, value, hash).

But it's not clear how to make this backwards compatible since the first bytes are currently produced by rand_bytes(16). Maybe we can assume that the first bytes after that are never 00?

[hadley]

Possibly should be informed by https://hackers.town/@zwol/114155595855705796

[mayeulk]

Hi. Thanks for this discussion. Indeed, I got confused elsewhere by the "embedded nul in string" error.

To reproduce it more easily: for (i in 1: 50) {key2 <- openssl::rand_bytes(32); print(httr2::secret_decrypt(enc, key2))}

Possibly, something similar to the following will at least avoid confusion about the "nul in string" ?

secret_decrypt <- 
 function (encrypted, key) {
  check_string(encrypted)
  enc <- base64_url_decode(encrypted)
  dec <- secret_decrypt_raw(enc, key)
  tryCatch(rawToChar(dec),
           error=function(e) {
             message('An error occurred, the key is most likely incorrect.')
             print(e)
           }           
           )
 }