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

Add advisory for pkcs11 #1282

Merged
merged 1 commit into from
Jul 23, 2022
Merged

Add advisory for pkcs11 #1282

merged 1 commit into from
Jul 23, 2022

Conversation

ionut-arm
Copy link
Contributor

#1280

Feedback is more than welcome, I think the Impact section is a bit verbose :[

cc @5225225

@ionut-arm
Copy link
Contributor Author

Also, depending on what happens with mheese/rust-pkcs11#57 I might add the unmaintained flag.

@tarcieri
Copy link
Member

Did a quick pass. Looks good so far 👍

@5225225
Copy link
Contributor

5225225 commented Jul 21, 2022

Also, notably, the security issues in the crate will become not as significant soon since the incorrect use of transmute_copy will just panic :)

Looks good to me though (other than the date, which I noted). I checked and the guideline for an unmaintained advisory is 90 days. So while I think the crate is pretty clearly unmaintained, it might be best to just put this out. Besides, "unmaintained" is a 'weaker' advisory than "wow this will segfault if you touch it wrong".

The impact section does seem verbose... Maybe at least split it into 2 paragraphs, 1 being the background knowledge about PKCS11 and its API, the second one being "so what does this mean for me who uses this library?" That or remove the background info entirely and only leave the sentences that are directly relevant to "why should I, as a pkcs11 user, care?".

I feel everything before "The raw pointers can be easily created in Rust, [...]" can either be split out into a new paragraph or removed.

@tarcieri
Copy link
Member

tarcieri commented Jul 21, 2022

@5225225 an unmaintained crate advisory can be filed separate to this one, and marked informational = "unmaintained"

This advisory is debatably informational = "unsound", but IMO given PKCS#11's purpose is a hardware interface for public key cryptography, this falls under the category of crypto-failure and thus deserves a proper security advisory

Signed-off-by: Ionut Mihalcea <ionut.mihalcea@arm.com>
@ionut-arm
Copy link
Contributor Author

Alright, shuffled some of the text around and made it shorter, also added informational = "unsound" and crypto-failure category - let me know if it looks more accurate/better now.

@5225225
Copy link
Contributor

5225225 commented Jul 22, 2022

Looks good to me! Definitely cleaner now.

Copy link
Member

@tarcieri tarcieri left a comment

Choose a reason for hiding this comment

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

LGTM.

@ionut-arm if you want to remove "Draft" I can merge.

Also if someone's interested it would be good to remove pkcs11 from the RCIG's Awesome Rust Cryptography list since it appears it's unmaintained: https://github.com/The-DevX-Initiative/RCIG_Coordination_Repo/blob/main/Awesome_Rust_Cryptography.md

@5225225
Copy link
Contributor

5225225 commented Jul 22, 2022

Also if someone's interested it would be good to remove pkcs11 from the RCIG's Awesome Rust Cryptography list since it appears it's unmaintained

I'll make a PR to do that.

@ionut-arm
Copy link
Contributor Author

@ionut-arm if you want to remove "Draft" I can merge.

Done!

Also if someone's interested it would be good to remove pkcs11 from the RCIG's Awesome Rust Cryptography list since it appears it's unmaintained: https://github.com/The-DevX-Initiative/RCIG_Coordination_Repo/blob/main/Awesome_Rust_Cryptography.md

I'll make a PR to do that.

Thank you!

@tarcieri tarcieri merged commit 4821444 into rustsec:main Jul 23, 2022
@5225225 5225225 mentioned this pull request Jul 28, 2022
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.

3 participants