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

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

Closed
bustedware opened this issue May 4, 2023 · 2 comments
Labels
FrozenDueToAge Proposal Proposal-Crypto Proposal related to crypto packages or other security issues
Milestone

Comments

@bustedware
Copy link

bustedware commented May 4, 2023

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)

  • An adversary can eavesdrop the communication and intercept the CBC-encrypted ciphertext
  • An adversary has access to a padding oracle O, which means he or she can distinguish between VALID and INVALID paddings

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:

#32777
FiloSottile mentions that md5 is used in the key derivation function, and that MD5 is not broken for that purpose.

#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: #52384

@gopherbot gopherbot added this to the Proposal milestone May 4, 2023
@bustedware bustedware changed the title proposal: affected/package: crypto/x509: IsEncryptedPEMBlock, EncryptPEMBlock, and DecryptPEMBlock should not be deprecated proposal: crypto/x509: IsEncryptedPEMBlock, EncryptPEMBlock, and DecryptPEMBlock should not be deprecated May 4, 2023
@seankhliao
Copy link
Member

cc @golang/security

@seankhliao seankhliao added the Proposal-Crypto Proposal related to crypto packages or other security issues label May 4, 2023
@FiloSottile
Copy link
Contributor

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge Proposal Proposal-Crypto Proposal related to crypto packages or other security issues
Projects
None yet
Development

No branches or pull requests

4 participants