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.

Sign up for GitHub

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

Jump to bottom

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

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

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

rolandshoemaker opened this issue Jul 28, 2020 · 6 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Go1.16

Comments

@rolandshoemaker
Copy link
Member

rolandshoemaker commented Jul 28, 2020 •
edited

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 cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 29, 2020
@cagedmantis
Copy link
Contributor

/cc @FiloSottile

@FiloSottile
Copy link
Contributor

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

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
Contributor

@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
Contributor

See also #37845

@gopherbot
Copy link

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

@golang golang locked and limited conversation to collaborators Oct 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Go1.16
Development

No branches or pull requests
4 participants
@cagedmantis @FiloSottile @rolandshoemaker @gopherbot