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: decryption of PEM file failure not being caught #43504

Closed
pschou opened this issue Jan 5, 2021 · 6 comments
Closed

proposal: crypto/x509: decryption of PEM file failure not being caught #43504

pschou opened this issue Jan 5, 2021 · 6 comments
Labels
Proposal Proposal-Crypto Proposal related to crypto packages or other security issues
Milestone

Comments

@pschou
Copy link

pschou commented Jan 5, 2021

I'd like to propose a way of early and fast detection of PEM decryption errors.

How this check works: it uses the ASN.1 basic encoding rules (BER) to parse the first length field. This length field will contain a number which, when [properly decoded and] parsed, contains the length of the PEM encoded message blob (plus the 2-4 bytes declaring this length). The power of doing this size comparison check via length field is that the decoding routine will then have a verification, with high certainty, that the blob was decoded properly -- and since this does not depend on knowing which kind of crypto pkcs PEM file is being decoded, it is not tied to the knowing or testing any of the pkcs formats; thus it is forward compatible.

Please see the proposal here
https://go-review.googlesource.com/c/proposal/+/281454

and pull request here
#43463

View pull request discussion here
https://go-review.googlesource.com/c/go/+/281112

resolves issue:
#10171

@ianlancetaylor ianlancetaylor changed the title crypto/x509: decryption of PEM file failure not being caught proposal: crypto/x509: decryption of PEM file failure not being caught Jan 5, 2021
@gopherbot gopherbot added this to the Proposal milestone Jan 5, 2021
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Jan 5, 2021
@ianlancetaylor ianlancetaylor added the Proposal-Crypto Proposal related to crypto packages or other security issues label Jan 5, 2021
@rsc
Copy link
Contributor

rsc commented Apr 5, 2023

/cc @rolandshoemaker

@rsc
Copy link
Contributor

rsc commented Apr 6, 2023

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Apr 19, 2023

Looking at the code in the linked CL/PR, I see the following:

  1. It introduces new API in the form of IncorrectDERError. I don't see any reason to introduce a new error: the existing IncorrectPasswordError seems like it describes the failure in this new check accurately.

  2. The existing code already checks the padding, which will already catch the vast majority of incorrect decryptions.

  3. The function being edited is marked Deprecated:

    // 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.
    

Given all this, it seems like we should leave this function alone and decline both the API change and the CL.

@rolandshoemaker
Copy link
Member

Agreed. I don't see a pressing need for this, and there are better avenues to pursue if we want to provide better PKCS encryption options.

@rsc
Copy link
Contributor

rsc commented May 3, 2023

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented May 10, 2023

No change in consensus, so declined.
— rsc for the proposal review group

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Proposal Proposal-Crypto Proposal related to crypto packages or other security issues
Projects
Status: Declined
Development

No branches or pull requests

5 participants