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

x509: disallow mismatching signature algorithm identifiers #702

Closed
wants to merge 2 commits into from
Closed

x509: disallow mismatching signature algorithm identifiers #702

wants to merge 2 commits into from

Conversation

rolandshoemaker
Copy link
Contributor

This prevents an easy method for spamming a log by modifying one of the
malleable fields in the certificate structure.

Fixes #699

@rolandshoemaker
x509: disallow mismatching signature algorithm identifiers
7e0b212 
This prevents an easy method for spamming a log by modifying one of the
malleable fields in the certificate structure.

Fixes google#699
@rolandshoemaker
Copy link
Contributor Author

Oh bleh, I forgot x509.ParseCertificate is also used for parsing precert tbsCertificates... I think this can probably be made to work together with that, but it's going to be a bit more complicated.

@codecov
Copy link

codecov bot commented Jul 13, 2020 •
edited

Codecov Report

Merging #702 into master will increase coverage by 2.46%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #702      +/-   ##
==========================================
+ Coverage   71.75%   74.22%   +2.46%     
==========================================
  Files          90       81       -9     
  Lines        9879     8946     -933     
==========================================
- Hits         7089     6640     -449     
+ Misses       2295     1870     -425     
+ Partials      495      436      -59
Impacted Files Coverage Δ
x509/x509.go 71.35% <66.66%> (+0.50%) ⬆️
submission/races.go 87.78% <0.00%> (-3.06%) ⬇️
asn1/asn1.go 79.46% <0.00%> (-0.54%) ⬇️
asn1/common.go 96.05% <0.00%> (ø)
client/multilog.go 65.09% <0.00%> (ø)
trillian/ctfe/sth.go 100.00% <0.00%> (ø)
trillian/ctfe/handlers.go 81.11% <0.00%> (ø)
trillian/util/log_leaf.go
trillian/util/resolver.go
trillian/util/timesource.go
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9d40ef1...b150747. Read the comment docs.

@rolandshoemaker
Copy link
Contributor Author

@Martin2112 @AlCutter do you folks have a strong opinion on this? It's a bit of a spam opportunity hole I'd like to close, but I don't think it's super urgent.

@AlCutter
Copy link
Member

Hey @rolandshoemaker thanks for sending - looks ok to me in principle.
RFC5280 says This field [TBSCertificate.signature] MUST contain the same algorithm identifier as the signatureAlgorithm field in the sequence Certificate (Section 4.1.1.2).

@FiloSottile wdyt? Is this something that could/should be fixed upstream? I know you folks have historically been very keen to make the golang x509 library err on the side of correctness.

@rolandshoemaker
Copy link
Contributor Author

Heh, I actually have a similar upstream change https://go-review.googlesource.com/c/go/+/235118.

@AlCutter
Copy link
Member

Hehe, excellent :)

@FiloSottile
Copy link
Contributor

FiloSottile commented Jul 22, 2020 via email

@AlCutter AlCutter closed this Oct 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Certificate verifier doesn't reject mismatched signature algorithm
4 participants
@rolandshoemaker @AlCutter @FiloSottile @googlebot