Skip to content

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

Description

@bustedware

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

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions