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

fix(errors): Handle randomness generation and invalid random values as errors in cryptographic code #6385

Merged
merged 7 commits into from
Apr 20, 2023

Conversation

oxarbitrage
Copy link
Contributor

Motivation

We want to avoid some panics in some cryptography code. There are some specific problems that were pointed by the audit.

This should close #6338

The code modified here is currently not used but we expect to use it in the future and we will want to avoid panics.

Solution

Most related code use a string as an error with this pattern.
However, this is not always possible, for example if you want to do that here you will get the following compiler error:

inherent associated types are unstable
see issue #8995 <https://github.com/rust-lang/rust/issues/8995> for more information

So i think that limitation might be the cause of not using try_fill_bytes() in the first place.

What is needed is our own errors for handling this and that is what this PR tries to do.

I think all the string errors should be eventually refactored to this but i didnt wanted to do that here as it will be too intrusive so i just fixed what was pointed by the audit ticket.

Review

I think anyone can review, the names chosen could be improved maybe.

Reviewer Checklist

  • Will the PR name make sense to users?
    • Does it need extra CHANGELOG info? (new features, breaking changes, large changes)
  • Are the PR labels correct?
  • Does the code do what the ticket and PR says?
    • Does it change concurrent code, unsafe code, or consensus rules?
  • How do you know it works? Does it have tests?

Follow Up Work

Add errors for all string errors used inside zebra-chain ?

@oxarbitrage oxarbitrage requested review from a team as code owners March 22, 2023 20:48
@oxarbitrage oxarbitrage requested review from teor2345 and removed request for a team March 22, 2023 20:48
@github-actions github-actions bot added C-bug Category: This is a bug C-feature Category: New features labels Mar 22, 2023
zebra-chain/src/error.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for making minimal changes!

I'd just like to check a few cryptographic details.

@teor2345 teor2345 changed the title fix(errors): Add some error handling to orchard and sapling submodules. fix(errors): Add some error handling to orchard and sapling submodules Mar 22, 2023
@teor2345 teor2345 changed the title fix(errors): Add some error handling to orchard and sapling submodules fix(errors): Handle randomness generation and invalid random values as errors in cryptographic code Mar 22, 2023
@codecov
Copy link

codecov bot commented Mar 22, 2023

Codecov Report

Merging #6385 (47a3adb) into main (f2133c9) will increase coverage by 0.04%.
The diff coverage is 0.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6385      +/-   ##
==========================================
+ Coverage   77.92%   77.97%   +0.04%     
==========================================
  Files         306      307       +1     
  Lines       40236    40249      +13     
==========================================
+ Hits        31355    31383      +28     
+ Misses       8881     8866      -15     

@mpguerra mpguerra mentioned this pull request Mar 23, 2023
36 tasks
@mpguerra
Copy link
Contributor

mpguerra commented Mar 23, 2023

Looks great, thanks for making minimal changes!

I'd just like to check a few cryptographic details.

I'm not sure we're going to be able to get a cryptography review until after RWC and/or Easter so this may have to move to Sprint 8.

EDIT: Unless @upbqdn is around today or tomorrow?

@upbqdn
Copy link
Member

upbqdn commented Mar 23, 2023

I'll review this.

@upbqdn upbqdn self-requested a review March 23, 2023 11:02
Copy link
Member

@upbqdn upbqdn left a comment

Choose a reason for hiding this comment

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

The error names make sense to me, but I'd rather let @dconnolly give it the final approval.

zebra-chain/src/error.rs Outdated Show resolved Hide resolved
@mpguerra mpguerra requested a review from dconnolly March 28, 2023 08:49
@teor2345 teor2345 added P-Medium ⚡ A-cryptography Area: Cryptography related C-audit Category: Issues arising from audit findings no-review-reminders Turn off review reminders and removed C-feature Category: New features labels Apr 2, 2023
@teor2345
Copy link
Contributor

teor2345 commented Apr 3, 2023

This is waiting for a final review from Deirdre when she gets back.

@mpguerra mpguerra removed the no-review-reminders Turn off review reminders label Apr 11, 2023
zebra-chain/src/error.rs Outdated Show resolved Hide resolved
@dconnolly
Copy link
Contributor

@oxarbitrage @upbqdn @teor2345 I made some suggestions in ed6596a, lmk what you think?

teor2345
teor2345 previously approved these changes Apr 19, 2023
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Looks good to me, just needs to resolve a merge conflict?

teor2345
teor2345 previously approved these changes Apr 19, 2023
teor2345
teor2345 previously approved these changes Apr 19, 2023
@daira
Copy link
Contributor

daira commented Apr 19, 2023

I question the idea that we want to avoid a panic when a cryptographic RNG fails. #6338 (comment)

mergify bot added a commit that referenced this pull request Apr 20, 2023
@mergify mergify bot merged commit 85534ab into main Apr 20, 2023
@mergify mergify bot deleted the issue6338 branch April 20, 2023 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cryptography Area: Cryptography related C-audit Category: Issues arising from audit findings C-bug Category: This is a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[NCC-E005955-DBV] zebra-chain: Unbounded Rejection Sampling with Possibility of Panics
6 participants