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

crypto/x509: CreateCertificate should maybe check the signatures it generates #40458

Closed
rolandshoemaker opened this issue Jul 28, 2020 · 6 comments
Closed
Assignees
Milestone

Comments

@rolandshoemaker
Copy link
Member

@rolandshoemaker rolandshoemaker commented Jul 28, 2020

x509.CreateCertificate, and all of the other CreateXXX methods in the package, appear to accept zero length signatures returned by the signer. It'd require a rather broken signer to produce this, but it seems like something the package should be checking for rather than blindly accepting the returned signature. Example.

Off the top of my head I don't think there is anything that explicitly disallows this, but none of the supported signature algorithms should produce a zero length signature, so it should be safe to just disallow it.

@cagedmantis cagedmantis added this to the Backlog milestone Jul 29, 2020
@cagedmantis
Copy link
Contributor

@cagedmantis cagedmantis commented Jul 29, 2020

@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Jul 29, 2020

I'm not sure why special-case empty signatures. There is definitely an argument to be made about verifying the signature while generating, because Signers produce all sorts of invalid signatures (empty, PKCS #1 v1.5 when asked for PSS, wrong private key for the public key...) but I feel like we should either do a full verification or just accept that we trust the Signer's output.

@rolandshoemaker
Copy link
Member Author

@rolandshoemaker rolandshoemaker commented Jul 30, 2020

Seems reasonable, in general I'd be strongly supportive of actually fully verifying the signer output so that we're not creating malformed certificates. That said I opened this because this was surprising behavior to me, this is the base broken case for a signer that requires no actual knowledge of the signature algorithm to detect, all of the supported signature schemes should produce a non-zero signature, so preventing it would be very simple and remove an entire class of signer brokenness ¯\_(ツ)_/¯.

@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Oct 5, 2020

@rolandshoemaker I'd like to see a CL that introduces a signature verification in CreateCertificate with a benchmark. If the numbers are not too bad, we can fix the whole class and avoid a lot of pain to a lot of people (and even maybe save some RSA keys from the CRT in case of bugs/faults).

@FiloSottile FiloSottile modified the milestones: Backlog, Go1.16 Oct 5, 2020
@FiloSottile FiloSottile changed the title crypto/x509: CreateCertificate accepts zero length signatures crypto/x509: CreateCertificate should maybe check the signatures it generates Oct 5, 2020
@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Oct 5, 2020

See also #37845

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 5, 2020

Change https://golang.org/cl/259697 mentions this issue: crypto/x509: add signature verification to CreateCertificate

@gopherbot gopherbot closed this in 2ec71e5 Oct 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.