golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
123.87k stars 17.65k forks source link

proposal: crypto/x509: IsEncryptedPEMBlock, EncryptPEMBlock, and DecryptPEMBlock should not be deprecated #59961

Closed bustedware closed 1 year ago

bustedware commented 1 year ago

This discussion is an attempt at scoping the problem of oracle attacks and to prevent IsEncryptedPEMBlock, EncryptPEMBlock, and DecryptPEMBlock from becoming deprecated in future releases of golang. I will address the deprecation message and attempt to resolve any concerns with keeping it in future releases

The message included alongside DecryptPEMBlock deprecation indicates the routine is insecure since it does not authenticate the ciphertext and is vulnerable to padding oracle attacks.

// Deprecated: Legacy PEM encryption as specified in RFC 1423 is insecure by
// design. Since it does not authenticate the ciphertext, it is vulnerable to
// padding oracle attacks that can let an attacker recover the plaintext.

Oracle attacks rely on 2 assumptions (page 9)

The attack relies on leaking the ciphertext. The routine DecryptPEMBlock should not be deprecated because it still adds value. Without encrypted PEM blocks we would be left with plaintext - which is what the deprecation message mentions an attacker would be able to recover. Defense in depth, also known as layered defense, is a security principle where single points of complete compromise are eliminated or mitigated by the incorporation of a series or multiple layers of security safeguards and risk-mitigation countermeasures (reference). Encrypting PEM blocks, although sometimes thwarted by a clever attacker, adds to our layers of defense.

  1. Included in this PR is the removal of the "Invalid Padding" error message Removing the oracle (in this case the "Invalid Padding" error message) will strengthen the protection among the golang community who leverage this routine. This helps mitigate an attacker from determining VALID from INVALID padding for an applications usage of this routine. An attacker may still choose to use another oracle but will still need to have access to or depend on leaked ciphertext. This effectively address the second point "it is vulnerable to padding oracle attacks" from the deprecation message.

References for oracle attacks: "The error messages are the crucial basis for the attack" (page 6) "Removing the oracle would also prevent the attack" (under "Defenses" second paragraph) "The oracle could be something as simple as returning a value that says "Invalid padding" or something more complicated like taking a measurably different time to process a valid block as opposed to an invalid block." (under "Introduction" fourth paragraph)

  1. To address the first point in the deprecation message "Since it does not authenticate the ciphertext" PEM blocks are not supplemented with any metadata to perform said authentication. Consider that authentication of the ciphertext relies on a signature which can only be produced by a signer. For example, TLS relies on both the client and server to agree upon a common CA for cipher authentication. A clever golang developer can still authenticate the ciphertext outside the scope of DecryptPEMBlock routine and may choose to not DecryptPEMBlock against a ciphertext which is not accompanied with a verifiable signature. Golang developers should be empowered with the choice of whether the integrity of the ciphertext is important to their application and whether or not they should invoke the DecryptPEMBlock routine. This should also address the concerns with EncryptPEMBlock deprecation message

Here are the related issues which deprecated them:

https://github.com/golang/go/issues/32777 FiloSottile mentions that md5 is used in the key derivation function, and that MD5 is not broken for that purpose.

https://github.com/golang/go/issues/41949 FiloSottile mentions in one comment "That encryption format is legacy and broken by design, so we should deprecate it, not mix it with newer formats that encourage its use" but does not provide any context about why its broken by design. I can only assume those concerns are reflected in the deprecation message which I will address

I created a PR for this already here: https://github.com/golang/go/pull/52384

seankhliao commented 1 year ago

cc @golang/security

FiloSottile commented 1 year ago

Padding oracle attacks are still possible through timing side channels even if the error message is not different.

You seem to be confusing authentication in the asymmetric sense (what signatures are for) and in the symmetric sense (what MACs and AEADs are for). That's understandable because the term is overloaded, but what PEM encryption misses is the latter, meaning an attacker can change the plaintext by modifying the ciphertext, which can lead to a host of security issues, even ignoring padding oracles.

"It's better than nothing" is not a good reason to retain a broken cryptographic API as users might rely on it not being broken, or like in this issue they might underestimate the scope and consequence of the brokenness.

None of this is an ongoing debate. PEM encryption is staying deprecated. There are many secure alternative ways to encrypt something before encoding it as PEM, such as PKCS#8 for keys, or age for files.