search

tink-crypto / tink-go

Go implementation of Tink
https://developers.google.com/tink
Apache License 2.0
91 stars 4 forks source link

Corrupted AEAD envelope in case remote KMS returns an empty/nil encrypted DEK #7

Closed beatrausch closed 7 months ago

beatrausch commented 9 months ago

Hi,

we run into the issue with an AEAD envelope encryption that the envelope was corrupted, in case the remote KMS returns an empty/nil encrypted DEK.

Describe the issue: In case the remote KMS returns a nil or empty encrypted DEK, the encrypt method returns an envelope that contains an encrypted DEK size of zero, an empty encrypted DEK as well as the encrypted plaintext without any error.

https://github.com/tink-crypto/tink-go/blob/440851d7cc21ac8b4e73b0a178f636a1e5c37b0e/aead/kms_envelope_aead.go#L90

The ciphertext cannot be decrypted, due to the fact, that the encrypted DEK is missing.

What was the expected behavior? In case the remote KMS returns a nil or empty encrypted DEK, encrypt shall return an error like "kms_envelope_aead: encrypted dek is invalid (zero/nil)"

What version of Tink are you using? v1.7.0 and main

Can you tell us more about your development environment? Golang 1.21

Regards,

juergw commented 8 months ago

Normally, the err should be non-nil when the remote AEAD can't encrypt the DEK. Can you give us more details how you run into the case that the encrypted dek is nil or empty? where and under what conditions does this happen?

beatrausch commented 8 months ago

Hi @juergw

I fully agree agree, normally it should not happen. We are still analyzing the issue, it seems that the hashi-vault returned this invalid response during a cluster update, but still unclear why.

However, I guess a simple len(encryptedDEK) check would increase the robustness against errors of external systems. What do you think?

juergw commented 8 months ago

Yes, I agree, I have already added that: https://github.com/tink-crypto/tink-go/commit/a9c8be3ffa8353ab10f30f9beed1b85cb2c0670c.

juergw commented 8 months ago

Are you using Tink's valut integration? Tink's valut integration uses the "Write" function on the "Logical" interface. I have now looked at the implementation of that, and the error handling for 404 looks weird: https://github.com/hashicorp/vault/blob/api/v1.10.0/api/logical.go#L181

There are several cases where err is nil, and secret is either nil or a secret with warnings, but no data. I guess I also have to add some additional checks to that code there.

juergw commented 8 months ago

Sorry, I linked the wrong line. The correct line is here: https://github.com/hashicorp/vault/blob/api/v1.10.0/api/logical.go#L244

juergw commented 7 months ago

I improved the HC vault integration a bit to better handle these cases, and to reject empty ciphertexts.

juergw commented 7 months ago

I think this is fixed now.