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

Address misc security issues #47

Merged
merged 4 commits into from
Jan 9, 2024
Merged

Address misc security issues #47

merged 4 commits into from
Jan 9, 2024

Conversation

timweri
Copy link
Collaborator

@timweri timweri commented Dec 24, 2023 •
edited
Loading

  • Cap the size of attestation certs for PIV and U2F key registration to mitigate DoS risks.
  • x509 authorization in external mode uses returned authority.
  • x509 authorization in local-db mode checks authority.

Testing done:

  • PIV and U2F key registration still works.
  • x509 authorization in local-db mode throws error when authority is invalid.
  • x509 authorization in local-db mode works in happy path.

@timweri timweri changed the title Limit the size of attestation certs Address misc security issues Dec 26, 2023
@timweri timweri force-pushed the req-limit branch 2 times, most recently from 07fec5e to b70e8bf Compare December 26, 2023 02:28
obelisk
obelisk reviewed Jan 4, 2024
View reviewed changes
rustica/src/verification.rs Outdated
// Restrict the max size of certificates
// For Yubikey 5 Nano, actual intermediate cert size is approx 800 bytes
// and actual client cert size is approx 700 bytes
const CERT_MAX_SIZE: usize = 1024 * 2; // 2 KiB
Copy link
Owner

Choose a reason for hiding this comment

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

These should go at the top of the file or in the lib.rs

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@obelisk Do you think the size check itself should be done in sshcerts too? That'd be cleaner but it might break some edge cases where people have unusually large attestation data.

obelisk
obelisk requested changes Jan 4, 2024
View reviewed changes
Copy link
Owner

@obelisk obelisk left a comment

Choose a reason for hiding this comment

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

Just address the constant

@timweri timweri requested a review from obelisk January 4, 2024 18:28
@obelisk obelisk merged commit fd388b9 into hacking_on_mozilla Jan 9, 2024
@timweri timweri deleted the req-limit branch March 2, 2024 01:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@obelisk obelisk obelisk approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@timweri @obelisk