-
Notifications
You must be signed in to change notification settings - Fork 150
stack zeroization #1215
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
Open
nstilt1
wants to merge
21
commits into
RustCrypto:master
Choose a base branch
from
nstilt1:stack_zeroization
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
stack zeroization #1215
Changes from all commits
Commits
Show all changes
21 commits
Select commit
Hold shift + click to select a range
8114b92
added 2 methods for stack zeroization for #810
nstilt1 9719738
removed code that doesn't work without segfaults
nstilt1 560ce88
move stack_sanitization.rs to its own crate
nstilt1 3e3a737
fmt
nstilt1 075d06b
moved to zeroize_stack
nstilt1 e791a4b
revised structure to allow miri to run, but panic
nstilt1 b02d547
added std feature to enforce panic/unwind safety; moved inline(never)…
nstilt1 6febd9f
fmt
nstilt1 d4447b9
minor doc revisions
nstilt1 b4ea722
minor doc revisions, fixed clippy; next steps: support async closures…
nstilt1 3b81376
added TODO.md
nstilt1 d665300
update TODO.md, next step will be allowing heap-stack reuse
nstilt1 ba7f0f4
added AlignedHeapStack to reuse stack space in sequential calls; see …
nstilt1 b820c3a
update TODO and some fmt looking changes
nstilt1 5e8f7c2
added raw code from stacker
nstilt1 a098bf8
modified stacker code
nstilt1 d389ae2
Merge branch 'master' into stack_zeroization
nstilt1 9eaf91f
fmt
nstilt1 0661279
clippy
nstilt1 1bb3800
audit fix?
nstilt1 afbbb0e
clippy again
nstilt1 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| [package] | ||
| name = "zeroize_stack" | ||
| version = "0.1.0" | ||
| description = """ | ||
| Securely zeroize the stack with a simple function built on | ||
| the Portable Stack Manipulation (psm) crate and zeroize crate. | ||
| """ | ||
| authors = ["The RustCrypto Project Developers"] | ||
| license = "Apache-2.0 OR MIT" | ||
| homepage = "https://github.com/RustCrypto/utils/tree/master/stack_sanitizer" | ||
| repository = "https://github.com/RustCrypto/utils" | ||
| readme = "README.md" | ||
| categories = ["cryptography", "memory-management", "no-std", "os"] | ||
| keywords = ["memory", "memset", "secure", "volatile", "zero", "stack"] | ||
| edition = "2024" | ||
| rust-version = "1.85" | ||
|
|
||
| [dependencies] | ||
| libc = "0.2.156" | ||
| psm = "0.1.26" | ||
| zeroize = { version = "1.0" } | ||
|
|
||
| [features] | ||
| default = ["heap", "stack", "std"] | ||
| heap = ["std"] | ||
| stack = [] | ||
| std = [] | ||
|
|
||
| [package.metadata.docs.rs] | ||
| all-features = true |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| # [RustCrypto]: zeroize_stack | ||
|
|
||
| [![Crate][crate-image]][crate-link] | ||
| [![Docs][docs-image]][docs-link] | ||
| ![Apache 2.0/MIT Licensed][license-image] | ||
| ![MSRV][rustc-image] | ||
| [![Build Status][build-image]][build-link] | ||
|
|
||
| Securely zero the stack (a.k.a. [zeroize]) while avoiding compiler optimizations. | ||
|
|
||
| This crate implements a portable approach to securely zeroing the stack using | ||
| techniques which guarantee they won't be "optimized away" by the compiler. | ||
|
|
||
| [Documentation] | ||
|
|
||
| ## About | ||
|
|
||
| [Zeroing memory securely is hard] - compilers optimize for performance, and | ||
| in doing so they love to "optimize away" unnecessary zeroing calls, as well | ||
| as make extra copies of data on the stack that cannot be easily zeroed. That's | ||
| what this crate is for. | ||
|
|
||
| This crate isn't about tricks: it uses [psm::on_stack] to run a function on | ||
| a portable stack, and then uses [zeroize] to zero that stack. `psm` implements | ||
| all of the assembly for several different architectures, and the [zeroize] | ||
| portion of the task was implemented in pure Rust. | ||
|
|
||
| - `#![no_std]` i.e. **embedded-friendly**! (`alloc` is required) | ||
| - No functionality besides securely zeroing the a function's stack usage! | ||
|
|
||
| ## License | ||
|
|
||
| Licensed under either of: | ||
|
|
||
| * [Apache License, Version 2.0](http://www.apache.org/licenses/LICENSE-2.0) | ||
| * [MIT license](http://opensource.org/licenses/MIT) | ||
|
|
||
| at your option. | ||
|
|
||
| ### Contribution | ||
|
|
||
| Unless you explicitly state otherwise, any contribution intentionally submitted | ||
| for inclusion in the work by you, as defined in the Apache-2.0 license, shall be | ||
| dual licensed as above, without any additional terms or conditions. | ||
|
|
||
| [//]: # (badges) | ||
|
|
||
| [crate-image]: https://img.shields.io/crates/v/zeroize.svg | ||
| [crate-link]: https://crates.io/crates/zeroize | ||
| [docs-image]: https://docs.rs/zeroize/badge.svg | ||
| [docs-link]: https://docs.rs/zeroize/ | ||
| [license-image]: https://img.shields.io/badge/license-Apache2.0/MIT-blue.svg | ||
| [rustc-image]: https://img.shields.io/badge/rustc-1.85+-blue.svg | ||
| [build-image]: https://github.com/RustCrypto/utils/actions/workflows/zeroize.yml/badge.svg?branch=master | ||
| [build-link]: https://github.com/RustCrypto/utils/actions/workflows/zeroize.yml?query=branch:master | ||
|
|
||
| [//]: # (general links) | ||
|
|
||
| [RustCrypto]: https://github.com/RustCrypto | ||
| [zeroize]: https://en.wikipedia.org/wiki/Zeroisation | ||
| [`Zeroize` trait]: https://docs.rs/zeroize/latest/zeroize/trait.Zeroize.html | ||
| [Documentation]: https://docs.rs/zeroize/ | ||
| [Zeroing memory securely is hard]: http://www.daemonology.net/blog/2014-09-04-how-to-zero-a-buffer.html | ||
| [psm::on_stack]: https://docs.rs/psm/latest/psm/fn.on_stack.html | ||
| [good cryptographic hygiene]: https://github.com/veorq/cryptocoding#clean-memory-of-secret-data |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| # TODO: | ||
|
|
||
| ## Likely impossible and/or unsafe | ||
|
|
||
| * Add support for async closures, possibly using a macro to define the functions if necessary. Use `futures::executor::block_on(f())` to poll the entire future completion inside the stack switched context, and avoid `.await` that yields control outside of the `on_stack()` boundary. Something like: | ||
|
|
||
| ```rust | ||
| pub unsafe fn exec_async_on_sanitized_stack<Fut, F, R>( | ||
| stack: &mut [u8], | ||
| f: F, | ||
| ) -> Result<R, Box<dyn std::any::Any + Send>> | ||
| where | ||
| F: FnOnce() -> Fut + UnwindSafe, | ||
| Fut: Future<Output = R>, | ||
| { | ||
| let mut result = None; | ||
|
|
||
| on_stack(stack, || { | ||
| result = Some(catch_unwind(AssertUnwindSafe(|| { | ||
| // Block on the future inside the heap stack | ||
| futures::executor::block_on(f()) | ||
| }))); | ||
| }); | ||
|
|
||
| result.expect("Closure did not run") | ||
| } | ||
| ``` | ||
|
|
||
| Copilot provided that code, but Gemini says that after the future is awaited, there will be no way for the program to know which stack to return to. Also, there is an open issue regarding async closures in `stacker` that has not been resolved after 7 months. https://github.com/rust-lang/stacker/issues/111 | ||
|
|
||
| ## Safe | ||
|
|
||
| * Handle unwinds better: currently we return a `Result<R, Box<dyn Any + Send>>`. The error case is a little bit tricky to handle, as dropping the error could cause a panic. The program should either panic, or return the panic payload's message. | ||
|
|
||
| * Either: | ||
| * Panic when the OS is `hermit` or it is running on `wasm32` or `wasm64`, as their stacks don't behave the same as all of the others. | ||
| * Run the closure without `psm::on_stack` and generate a compiler warning stating that the target's stack layout is not supported with basic stack switching. | ||
| * Implement different types of `AlignedHeapStack` to cover `wasm32` and `hermit` as performed in the `stacker` crate. | ||
|
|
||
| ## Would require a PR to `stacker` to zero the allocated stack on drop | ||
|
|
||
| * Use stacker crate to handle stack size management: if I read some of the `stacker` docs correctly, that crate should be able to extend the size of the stack when it is about to overflow. If that is correct, we could use their techniques to allocate a new stack and zeroize the old one whenever our allocated stack is about to overflow, eliminating the primary remaining `# Safety` comment. Note: we may not be able to zeroize the old stack immediately as the stack switching process likely attempts to return to the old stack once execution completes; we might have to wait until execution completes before zeroizing all heap-stacks. | ||
|
|
||
| ## Requires `asm!` | ||
|
|
||
| * Add an `asm!` alternative method for stack bleaching. In theory, it would be better to use `asm!` as we would not need to worry about the size of the allocated switched stack, and it would keep all of the code running on the actual stack and not the heap, possibly preserving performance. The problem with this is that using pointers from `asm!` and rust code to zero the space between the pointers results in segmentation faults on `x86_64`. | ||
| * when testing this, assert that the two pointers are not equal to each other and not null. | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| //! This file contains code derived from the Rust project, | ||
| //! originally written by Alex Crichton and licensed under | ||
| //! the Apache License, Version 2.0 or the MIT license, at | ||
| //! your option. | ||
| //! | ||
| //! Copyright (c) 2014 Alex Crichton | ||
| //! | ||
| //! Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or | ||
| //! http://www.apache.org/licenses/LICENSE-2.0> or the MIT license | ||
| //! <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your | ||
| //! option. This file may not be copied, modified, or distributed | ||
| //! except according to those terms. | ||
|
|
||
| use core::{ptr, sync::atomic}; | ||
|
|
||
| extern crate std; | ||
|
|
||
| /// A zeroizing heap-based stack. Feed one of these into the `switch_stacks` | ||
| /// function. | ||
| pub struct ZeroizingHeapStack { | ||
| new_stack: *mut u8, | ||
| stack_bytes: usize, | ||
| } | ||
|
|
||
| const ALIGNMENT: usize = 32; | ||
|
|
||
| impl ZeroizingHeapStack { | ||
| /// Initializes a new "Zeroizing Heap Stack". To be fed into the `switch_stacks` | ||
| /// function, and it can be reused, but it must not be reused while it is in use. | ||
| /// The borrow-checker should enforce this. | ||
| pub fn new(stack_kb: usize) -> ZeroizingHeapStack { | ||
| let stack_bytes = stack_kb * 1024; | ||
| assert!( | ||
| stack_bytes as isize > 0, | ||
| "stack_kb must be positive and must not overflow isize when expanded to number of bytes instead of KB" | ||
| ); | ||
| // On these platforms we do not use stack guards. this is very unfortunate, | ||
| // but there is not much we can do about it without OS support. | ||
| // We simply allocate the requested size from the global allocator with a suitable | ||
| // alignment. | ||
| let stack_bytes = stack_bytes | ||
| .checked_add(ALIGNMENT - 1) | ||
| .expect("unreasonably large stack requested") | ||
| / ALIGNMENT | ||
| * ALIGNMENT; | ||
| let layout = std::alloc::Layout::from_size_align(stack_bytes, ALIGNMENT).unwrap(); | ||
| let ptr = unsafe { std::alloc::alloc(layout) }; | ||
| assert!(!ptr.is_null(), "unable to allocate stack"); | ||
| ZeroizingHeapStack { | ||
| new_stack: ptr, | ||
| stack_bytes, | ||
| } | ||
| } | ||
| /// Returns (`start ptr of usable stack`, `size of usable stack`). | ||
| pub fn stack_area(&self) -> (*mut u8, usize) { | ||
| (self.new_stack, self.stack_bytes) | ||
| } | ||
| } | ||
|
|
||
| impl Drop for ZeroizingHeapStack { | ||
| fn drop(&mut self) { | ||
| let mut ptr = self.new_stack as *mut u128; | ||
| for _ in 0..self.stack_bytes / size_of::<u128>() { | ||
| unsafe { | ||
| ptr::write_volatile(ptr, 0); | ||
| ptr = ptr.add(1); | ||
| } | ||
| } | ||
| atomic::compiler_fence(atomic::Ordering::SeqCst); | ||
| unsafe { | ||
| std::alloc::dealloc( | ||
| self.new_stack, | ||
| std::alloc::Layout::from_size_align_unchecked(self.stack_bytes, ALIGNMENT), | ||
| ); | ||
| } | ||
| } | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we agree with Gemini that this is either impossible or unsafe to perform? I can try to implement it and see if I get any segfaults, but async code is a low priority for me since basically all crypto operations are synchronous, and it can be ran in a way such that only crypto operations are done on separate stacks