x509: disallow mismatching signature algorithm identifiers

[rolandshoemaker]

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

Fixes #699

[rolandshoemaker]

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]

Codecov Report

Attention: Patch coverage is 66.66667% with 2 lines in your changes missing coverage. Please review.

Project coverage is 74.22%. Comparing base (9d40ef1) to head (b150747).
Report is 773 commits behind head on master.

Files with missing lines	Patch %	Lines
x509/x509.go	66.66%	0 Missing and 2 partials ⚠️

Additional details and impacted files

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[rolandshoemaker]

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

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]

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

[AlCutter]

Hehe, excellent :)

[FiloSottile]

Yep, happy to look at it for Go 1.16!

…

[phbnf]

This was recently surfaced in https://groups.google.com/a/mozilla.org/g/dev-security-policy/c/p1JoBkfRhOQ/m/pF8ZdjRzCQAJ, let's merge this in.

[phbnf]

/gcbrun