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

Add defensive programming in FromBytes::new_box_slice_zeroed #64

Closed
joshlf opened this issue Oct 15, 2022 · 2 comments
Closed

Add defensive programming in FromBytes::new_box_slice_zeroed #64

joshlf opened this issue Oct 15, 2022 · 2 comments
Labels
blocking-next-release This issue should be resolved before we release on crates.io experience-hard This issue is hard, and requires a lot of experience

Comments

@joshlf
Copy link
Member

joshlf commented Oct 15, 2022 •
edited
Loading

In #63, we switched from manual bounds checking plus Layout::from_size_align_unchecked to calling Layout::from_size_align and relying on its bounds checking. There seemed to be a bug in that bounds checking prior to 1.65.0, and so we wrote a test which we disabled on Rust versions prior to 1.65.0. The reasoning was that the worst that could happen was a failed allocation, so it wasn't actually dangerous to expose this bug in the API.

dtolnay/semver#294 deals with this as well, and takes a more defensive stance. It observes that allocation with an invalid Layout is actually UB, and if the API is somehow reachable via attacker-controlled input, it results in an easy-to-exploit path to attacker-controlled UB. I think we may want to add defenses along the same lines.

The specific task is to:

  • Figure out what UB is possible when combining the code as currently written with the version of Layout::from_size_align on 1.64.0
  • Modify FromBytes::new_box_slice_zeroed to ensure that that UB cannot be triggered
  • Leave a // TODO(#67): ... comment to remove the workaround once our MSRV is at least 1.65.0
  • Add that TODO to the list for 1.65.0 kept in List of TODOs blocked on MSRV #67
  • Remove the conditional compilation on test_new_box_slice_zeroed_panics_isize_overflow and update the comment there
@joshlf joshlf added Hacktoberfest experience-hard This issue is hard, and requires a lot of experience blocking-next-release This issue should be resolved before we release on crates.io labels Oct 15, 2022
@joshlf
Copy link
Member Author

joshlf commented Feb 23, 2023

Updated MSRV to 1.65.0 in #156, so this isn't relevant anymore.

@joshlf joshlf closed this as completed Feb 23, 2023
joshlf added a commit that referenced this issue Aug 8, 2023
@joshlf
Reduce MSRV to 1.61.0
475472f 
It turned out that there were only a few lines of code that required
our MSRV to be as high as it was before this commit (1.65.0), and those
lines were easy to modify to be compatible with this new MSRV.

Note that this requires re-introducing a bug that was removed in #156
because the bug is based on an underlying bug in the standard library.
That bug was considered minor at the time it was discovered, and I still
consider it minor now. See #64 for more details.
@joshlf
Copy link
Member Author

joshlf commented Aug 8, 2023

This became relevant again in #234, but that PR also introduced defensive programming to mitigate the issue.

@joshlf joshlf mentioned this issue Aug 20, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
blocking-next-release This issue should be resolved before we release on crates.io experience-hard This issue is hard, and requires a lot of experience
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@joshlf