Skip to content

std.crypto: enhance Certificate security #19759

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

Closed
wants to merge 1 commit into from

Conversation

clickingbuttons
Copy link
Contributor

@clickingbuttons clickingbuttons commented Apr 24, 2024

Enhanced security:

  • Add and use more secure DER parser which prevents previously possible buffer overflows and OOB reads.
  • Fail on unknown critical extensions.
  • Verify key usage and extended key usage (closes std.crypto.Certificate.verify: additionally verify "key usage" #14175).
  • Verify policy (needs future work + validation).
  • Verify basic constraints.
  • Verify that Certificates loaded into bundles are indeed CAs.
  • Correctly handle certificate dates before 1970.

Enhanced compatibility:

  • Allow any SHA2 hash function with RSA and ECDSA public keys.

Added features:

Enhanced security:
	- Add and use more secure DER parser which prevents
		previously possible buffer overflows and OOB reads.
	- Fail on unknown critical extensions.
	- Verify key usage and extended key usage (closes ziglang#14175).
	- Verify policy (needs future work + validation).
	- Verify basic constraints.
	- Verify that Certificates loaded into bundles are indeed CAs.
	- Correctly handle certificate dates before 1970.

Enhanced compatibility:
	- Allow any SHA2 hash function with RSA and ECDSA public keys.
@clickingbuttons
Copy link
Contributor Author

clickingbuttons commented Apr 24, 2024

Putting this up for early feedback from @jedisct1 regarding crypto API changes. I feel rsa.zig is good enough to make public (still need keypair generation, happy to delay making public until that's been added) and I've made ecdsa.zig match.

Still need to:

  • Remove defer parser.seek(seq.slice.end) statements
  • Allow for bundles to contain certificates from same issuer but with different time validities.
  • Add a certificate verification test suite. Will require finding an existing one OR writing a DER formatter and creating one ourselves (I much prefer the former)
  • Add error sets

Comment on lines +156 to +157
// // check that d * e is one mod p-1 and mod q-1. Note d and e were bound
// const de = secret.d.mul(public.e); // can't mul these :(
Copy link
Contributor

@jedisct1 jedisct1 Apr 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Divide de by (p-1) (and (q-1)) and check that the remainder is 1.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how to compute de when there's no FeUnit multiplication except under the modulus.

Also not sure how to divide since there's no div function in ff.zig.

Montgomery would be ashamed of me...

@jedisct1
Copy link
Contributor

This is great work, but it would be really better if you could split these changes into smaller, more scoped PRs, that would be way easier to review and discuss.

@nektro
Copy link
Contributor

nektro commented Apr 24, 2024

this feels like it should be split up into multiple independently reviewed PRs. being a 10k+ line diff in a single commit makes the effort to merge this exponentially higher than otherwise.

@clickingbuttons
Copy link
Contributor Author

I spent 2 days thinking of how to split this up. I could only think of a dirty way, but I'll pursue that given the feedback.

being a 10k+ line diff in a single commit makes the effort to merge this exponentially higher than otherwise.

6K of it is test files, but 4k is indeed still large.

@clickingbuttons
Copy link
Contributor Author

Closing in favor of #19771 and another Certificate PR after that. All RSA feedback has been addressed there except for how to check d * e is one mod p-1 and mod q-1, which is luckily not essential.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

std.crypto.Certificate.verify: additionally verify "key usage"
3 participants