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

Implement Zeroize #553

Open
data-retriever opened this issue Dec 1, 2022 · 12 comments
Open

Implement Zeroize #553

data-retriever opened this issue Dec 1, 2022 · 12 comments

Comments

@data-retriever
Copy link

I don't know how feasible it is, but as there are a lot of secrets here, it would make sense.

@apoelstra apoelstra transferred this issue from rust-bitcoin/rust-bitcoin Dec 1, 2022
@apoelstra
Copy link
Member

Moving to rust-secp. I'm surprised that no such issue already exists.

The TL;DR is that these zeroing crates (there are a couple, IIRC the other popular one is called subtle) can't make very strong guarantees at all, because Rust's model results in a ton of memcpys of types like this. So we'd probably get little benefit from this, and the costs are

  • An extra dependency, with associated review burden and MSRV difficulty
  • Prevents impl'ing Copy on SecretKey which is a pretty big ergonomics hit

Having said this, I would like to support this somehow, even if the library doesn't have first-class support for it. I don't know either whether this is feasible.

@Kixunil
Copy link
Collaborator

Kixunil commented Dec 1, 2022

I believe this is a lost cause. The compiler may copy secret bytes to unexpected locations as a result of optimizations and there's no way to prevent that. Also the only thing this protects against is leakage due to UB. Rust has much less UB than C, so the value is questionable and the significant effort required to actually make this possible would be better spent at preventing (the remaining) UB in the first place.

@sanket1729
Copy link
Member

@apoelstra
Copy link
Member

@sanket1729 but the immediately following section says that copies and moves undermine the crate and that you need to use Pin to prevent this. And it is basically impossible to use Pin in this library because we produce objects we don't own (after creation) and can't box due to nostd.

@sanket1729
Copy link
Member

@apoelstra thanks for pointing that out. I am with @Kixunil now, this is a very hard problem to solve and our resources are better invested elsewhere.

@kayabaNerve
Copy link

subtle is about being constant time, not about zeroing memory.

zeroize is about traits to zeroize memory. Zeroize does not require removing the copy syntax which is a massive UX hit, as I've tried in the past. The nice thing though, beyond the ability to derive Zeroize for higher level objects since all members implement it, is the fact you can then use Zeroizing. Zeroizing wraps any type into a non-copy instance which will be zeroized on drop and IMO, is the preferable way to handle secret keys (so basically, you leave the choice to users).

Accordingly, I don't believe this is fruitless regarding functionality. I also don't believe implementing Zeroize would affect being no_std/Pin/whatever at all. It'd just be a function which accesses the underlying C struct and writes zeros (though, with syntax which makes it so it won't be optimized out. Ideally, you just call zeroize's <&mut [u8]>::zeroize).

Not to say that magically gives you time or makes it a priority. I just wanted to to highlight the benefits/impacts over it.

@Kixunil
Copy link
Collaborator

Kixunil commented Dec 8, 2022

@kayabaNerve thanks for explaining, that sounds at least doable (even though I still find value questionable).

@kayabaNerve
Copy link

Yeah, memory access will always be a question of "If someone already has that much access, isn't it moot?" My preference is to minimize copies, and do what I can, even if sure, the second I perform multiplication, the underlying lib probably makes a few copies (hopefully on the stack, at least). The stack ones will hopefully get cleared out soon, and I can know my long lived ones aren't just available for the next program which calls malloc.

@Kixunil
Copy link
Collaborator

Kixunil commented Dec 8, 2022

aren't just available for the next program which calls malloc

They already aren't. The kernel zeroizes allocated pages. It'd be a huge vulnerability if it didn't.

@kayabaNerve
Copy link

Some systems do not offer that behavior, per malloc's memory being undefined (justifying calloc), nor do I believe Linux is complete in that behavior. Beyond apparently being configurable when compiling the kernel, both Linux and Windows seem to only zero pages before handing off to a distinct process. While I did say, "next program" (which may still be feasible on some embedded systems), there are other concerns where a process both manages a secret key and exposes some level of memory buffer access to users (though Rust thankfully prevents most of these).

@Kixunil
Copy link
Collaborator

Kixunil commented Dec 8, 2022

justifying calloc

That's something completely different. The process gets zeroed memory from the kernel but it doesn't zero it's own which means that if you allocate, write something and then deallocate the next allocation may obtain the pointer containing what you just wrote (or may not). calloc is for zeroing those.

a process both manages a secret key and exposes some level of memory buffer access to users (though Rust thankfully prevents most of these).

Rust protecting against this is exactly my point. It's no longer that big issue (as an interesting data point, according to a recent report, Google found no vulnerabilities in their Rust code so far). If we focus our time on getting unsafe right we can get much more security than chasing secrets all over memory.

@elichai
Copy link
Member

elichai commented Dec 9, 2022

Moving to rust-secp. I'm surprised that no such issue already exists.

It just exists as PRs :) : #102, #165, #262, #311

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

No branches or pull requests

6 participants