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: Certificate.isValid should reject certificates with UnknownPublicKeyAlgorithm #66167

Open
rolandshoemaker opened this issue Mar 7, 2024 · 2 comments
Assignees
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@rolandshoemaker
Copy link
Member

rolandshoemaker commented Mar 7, 2024

A certificate with UnknownPublicKeyAlgorithm is not reasonably useful, since it cannot be used to verify any signatures, isValid should therefore reject it.

The one argument against this is for certificates which use key types which we don't support in the standard library, but for which third-party support does exist. In this case they could parse the key from the RawSubjectPublicKeyInfo, and use that to verify signatures, but that seems likely to be an incredibly uncommon use case (which I am not particularly sure actually currently exists).

Related to #66166, this would prevent putting certificates in the pool which can only reasonably be used by themselves.

@rolandshoemaker rolandshoemaker self-assigned this Mar 7, 2024
@AGWA
Copy link

AGWA commented Mar 8, 2024

I assume you mean UnknownPublicKeyAlgorithm rather than UnknownSignatureAlgorithm?

Verifying a leaf certificate with an unknown public key algorithm doesn't seem that bizarre to me (though it's certainly not useful in the WebPKI), and removing support would probably break someone. Are there any justifications for this besides #66166? CertPool.Add could just check for UnknownPublicKeyAlgorithm itself, right?

@rolandshoemaker rolandshoemaker changed the title crypto/x509: Certificate.isValid should reject certificates with UnknownSignatureAlgorithm crypto/x509: Certificate.isValid should reject certificates with UnknownPublicKeyAlgorithm Mar 8, 2024
@rolandshoemaker
Copy link
Member Author

I assume you mean UnknownPublicKeyAlgorithm rather than UnknownSignatureAlgorithm?

🤦, yes.

Verifying a leaf certificate with an unknown public key algorithm doesn't seem that bizarre to me (though it's certainly not useful in the WebPKI), and removing support would probably break someone. Are there any justifications for this besides #66166? CertPool.Add could just check for UnknownPublicKeyAlgorithm itself, right?

For roots and intermediates I think the argument is clearer, since we cannot reasonably build any chains with them, but I think you can make an equally compelling argument (although perhaps slightly less strong) argument for leaves, in that the certificate itself may be verified, but what is the certificate then to be used for? It cannot be used for TLS, since you need to verify the signature during the handshake, you can't use it for code signing, or S/MIME, because you would need to verify the signature, etc etc.

While I don't think this is extremely high priority, I think since we do explicitly target web PKI, and since isValid is only used during chain building, doing this lets us remove a little complexity that needs to be considered currently (#65390 is an example of how this has bitten us recently).

@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

3 participants