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

Adding security-warnings to #[must_use] #90

Closed
brycx opened this issue Sep 3, 2019 · 3 comments
Closed

Adding security-warnings to #[must_use] #90

brycx opened this issue Sep 3, 2019 · 3 comments
Labels
improvement General improvements to code

Comments

@brycx
Copy link
Member

brycx commented Sep 3, 2019

Functions that return a Result generate a warning when these are not used:

warning: unused `std::result::Result` that must be used
   --> src/high_level_api.rs:111:5
    |
111 |     orion::aead::seal(&aead_key, "Test".as_bytes());
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: #[warn(unused_must_use)] on by default
    = note: this `Result` may be an `Err` variant, which should be handled

This is the standard warning for a Result, but adding an explicit security warning to these functions may help users understand the importance of not ignoring a Result. Adding this would then produce two separate warnings by the compiler:

#[must_use = "SECURITY WARNING: Ignoring a Result may have security-implications."]

warning: unused `std::result::Result` that must be used
   --> src/high_level_api.rs:111:5
    |
111 |     orion::aead::seal(&aead_key, "Test".as_bytes());
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: #[warn(unused_must_use)] on by default
    = note: this `Result` may be an `Err` variant, which should be handled

warning: unused return value of `orion::aead::seal` that must be used
   --> src/high_level_api.rs:111:5
    |
111 |     orion::aead::seal(&aead_key, "Test".as_bytes());
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: SECURITY WARNING: Ignoring a Result may have security-implications.

The question is whether this approach is worthwhile.

Edit: If this is decided against, then we should remove the useless #[must_use] on Result as mentioned in #89.

@brycx brycx added question Further information is requested improvement General improvements to code labels Sep 3, 2019
@brycx brycx removed the question Further information is requested label Sep 3, 2019
@brycx
Copy link
Member Author

brycx commented Sep 3, 2019

I have gotten positive feedback on this and it will therefor be implemented. This is based on the same assumption as mentioned above: that users hopefully will be more cautious about ignoring such Results which can have a security-impact. The error message that seemed most clear and concise was:

"SECURITY WARNING: Ignoring a Result can have real security implications."

@brycx
Copy link
Member Author

brycx commented Sep 4, 2019

@colelawrence expressed interest and is taking this on.

colelawrence added a commit to colelawrence/orion that referenced this issue Sep 6, 2019
Remove redundant must_use from internal fns
brycx pushed a commit that referenced this issue Sep 6, 2019
* Address #90: Add must_use message to pub Results

Remove redundant must_use from internal fns

* Fix formatting for build step

* Remove must_use for Sha512::init

* Remove must_use for struct Hmac

* Remove must_use for Poly1305::init

* Update must_use messages in chacha20.rs

* run cargo fmt
@brycx
Copy link
Member Author

brycx commented Sep 6, 2019

Fixed in #95.

@brycx brycx closed this as completed Sep 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement General improvements to code
Projects
None yet
Development

No branches or pull requests

1 participant