briansmith / ring

Safe, fast, small crypto using Rust
Other
3.64k stars 683 forks source link

Document how to port *ring* to a `no_std`-only platform #744

Open briansmith opened 5 years ago

briansmith commented 5 years ago

[Updated]: Currently the main issue is the need to implement fill_impl in ring::rand for each platform.

briansmith commented 5 years ago

The other, harder, issue, is how to get the test suite to run on such platforms, because the tests use libstd a lot currently. Some issues are probably, e.g. we could use ArrayVec or similar instead of std::vec::Vec in most cases, though we'd have to worry about stack overflows.

I suggest we prototype this first by solving it specifically for SGX, which seems to be a no_std platform that people are especially interested in. See https://github.com/baidu/rust-sgx-sdk#v104-release in particular; it would be nice to help Baidu avoid the need for forking ring.

elichai commented 5 years ago

@briansmith Are you working on this? I would be glad to try and do this for sgx.

jonas-schievink commented 5 years ago

This solution is unlikely to work in general for embedded platforms. AFAIK, for SGX there is only one obvious implementation of a random number generator (namely the rdrand instruction) that can be used for the entire target.

This is not the case for eg. embedded ARM targets, which don't have a single random number generator. Instead, vendors often provide a peripheral for random number generation, which could implement SecureRandom.

For those targets I suggest #[cfg]ing the SystemRandom struct away, and expecting users (or microcontroller support crates) to implement the SecureRandom trait for specific peripherals. Would that be an acceptable solution?

EDIT: Just noticed that SecureRandom is sealed, why is that? Can that be removed or would that break any assumptions?

briansmith commented 5 years ago

EDIT: Just noticed that SecureRandom is sealed, why is that? Can that be removed or would that break any assumptions?

That way we can guarantee it is actually secure by ensuring all the implementations are in ring.

For those targets I suggest #[cfg]ing the SystemRandom struct away, and expecting users (or microcontroller support crates) to implement the SecureRandom trait for specific peripherals. Would that be an acceptable solution?

In such situations the peripherals are almost definitely implementing a raw, unmixed, entropy source. We should take the output of that and postprocess it with a DRBG.

In other words, for #[cfg(target_os="none")] we can provide an alternative API for plugging in an entropy source, which feeds a DRBG. Probably we should do that for SGX too, in particular.

jonas-schievink commented 5 years ago

Thanks, that makes sense!

In such situations the peripherals are almost definitely implementing a raw, unmixed, entropy source. We should take the output of that and postprocess it with a DRBG.

Nordic employs optional bias correction in their RNGs, so I think this step would not be needed for their devices (provided that the correction is actually turned on, of course - https://github.com/nrf-rs/nrf52-hal/issues/85).

That way we can guarantee it is actually secure by ensuring all the implementations are in ring.

That would not be the case if you'd accept external entropy sources though, right?

briansmith commented 5 years ago

Nordic employs optional bias correction in their RNGs, so I think this step would not be needed for their devices (provided that the correction is actually turned on, of course - nrf-rs/nrf52-hal#85).

Interesting. I don't know if that would be sufficient without reading more about it.

That way we can guarantee it is actually secure by ensuring all the implementations are in ring.

That would not be the case if you'd accept external entropy sources though, right?

First, let me walk back what I said a bit: In ring we don't guarantee anything. We try to make things as safe as we can given limited resources.

Your issue https://github.com/nrf-rs/nrf52-hal/issues/85 shows exactly why I've avoided providing a way to swap in an entropy source so far. Perhaps, instead of providing an extension point for entropy sources, we should provide explicit features that enable specific implementations. So, for example, we could have a nrf52-hal feature that enabled an implementation of SecureRandom that delegates its entropy collection to the nrf52-hal library. This way we can work with all the HAL providers to ensure that things like https://github.com/nrf-rs/nrf52-hal/issues/85 are properly addressed before people use them (through ring).

briansmith commented 5 years ago

From #813:

  • Lots of files refer to libc::c_int and friends, which aren't present (the libc crate is empty on this family of targets...). I believe the standard approach here is to use https://github.com/japaric/cty?

Why not just implement the size types in the libc crate for that target? That's what the libc crate is for.

Researching this, it seems the libc crate maintainers don't want to port libc to platforms that need the C types defined but which don't have a libc. See https://github.com/rust-lang/libc/pull/1092 for example.

The cty crate is basically the libc-types crate that was proposed in https://github.com/rust-lang/rfcs/pull/1783. I left a comment (https://github.com/rust-lang/rfcs/pull/1783#issuecomment-499656974) about reopening that issue. I think having a libc-types crate that is type-compatible with libc's definitions of the same-named types is the right solution. If you agree, please comment over there.

josephlr commented 5 years ago

The cty crate is basically the libc-types crate that was proposed in rust-lang/rfcs#1783. I left a comment (rust-lang/rfcs#1783 (comment)) about reopening that issue. I think having a libc-types crate that is type-compatible with libc's definitions of the same-named types is the right solution. If you agree, please comment over there.

So after my attempt in https://github.com/rust-lang/libc/pull/1244 and a later attempt in https://github.com/rust-lang/libc/pull/1285, the conclusion was to keep cty and libc separate for now (https://github.com/rust-lang/libc/pull/1285#issuecomment-467085692), but to make sure that the types between the crates will remain compatible (https://github.com/rust-lang/libc/pull/1286). So it seems like using cty (if that's all we need) is the best way forward.

In other words, for #[cfg(target_os="none")] we can provide an alternative API for plugging in an entropy source, which feeds a DRBG. Probably we should do that for SGX too, in particular.

To make things easier, we could simply have three (mutually exclusive) Cargo features that determine how SystemRandom is implemented:

josephlr commented 5 years ago

Also, @briansmith is there a reason we don't just use getrandom (the implementation crate for rand::OsRng) for SystemRandom? It seems like:

briansmith commented 5 years ago

So after my attempt in rust-lang/libc#1244 and a later attempt in rust-lang/libc#1285, the conclusion was to keep cty and libc separate for now (rust-lang/libc#1285 (comment)), but to make sure that the types between the crates will remain compatible (rust-lang/libc#1286). So it seems like using cty (if that's all we need) is the best way forward.

Generally ring doesn't depend on third-party crates. We had made an exception for spin because it was already a dependency of a rust-lang crate. As far as I understand those previous attempts to integrate cty and libc, I don't think they've failed but rather they just need more effort (political and technical) put into them. I think the correct approach is to write a new RFC, probably based on https://github.com/rust-lang/rfcs/pull/1783#issuecomment-499656974. I think there will be a lot of political support for the libc-types crate and I think it will get accepted. I think it will be a lot smoother than getting the getrandom crate accepted into libcore/libstd.

briansmith commented 5 years ago

Also, @briansmith is there a reason we don't just use getrandom

ring's doesn't use getrandom primarily because: (1) ring predates that crate by several years, (2) ring avoids depending on third-party crates to minimize risk, (3) I haven't reviewed the getrandom code, so it's not clear how correct it is. This is something that should be revisited if/when getrandom() actually does make it into libcore. I'm not holding my breath while that happens, though, since that idea has been discussed for about as long as ring has existed.

A bigger problem is that it isn't clear that ring's current API (which is similar to the getrandom crate's API) is sufficient for the lowest-level embedded use cases. When there is an external peripheral that provides a hardware RNG, what kind of API do we need that works in conjunction with the the embedded HAL to manage access to the hardware RNG? When a long-running task needs intermittent access to a CSPRNG, how can we provide that access without blocking access to other peripherals? (Somebody made a comment to this effect before, but I can't find where it was.) Note that in ring's API, every function that uses a CSPRNG takes a &SecureRandom parameter and nothing needs to hold a long-lived reference to the CSPRNG. I think this is the kind of API that is needed to handle the trickiest embedded use cases. Unfortunately, I know many ring-based libraries hold references to SystemRandom across many operations, which kind of undoes that benefit of ring's design. OTOH, they are doing that because there doesn't seem to be a clear benefit to them to doing things a different way. I hope this can be sorted out in this issue.

tarcieri commented 5 years ago

A bigger problem is that it isn't clear that ring's current API (which is similar to the getrandom crate's API) is sufficient for the lowest-level embedded use cases. In particular, when there is an external peripheral that provides a hardware RNG, what kind of API do we need that works in conjunction with the the embedded HAL to manage access to the hardware RNG?

FYI, some issues about this:

https://github.com/rust-random/getrandom/issues/4 https://github.com/rust-embedded/wg/issues/353

It's a good question and a tough one: embedded-hal generally eschews global state, attempting to make all global peripherals owned, but the getrandom API is global/ambient.

briansmith commented 5 years ago

I submitted PR #840 as a stepping stone to resolving the libc types issue in the interim until the Rust libs team can agree on a long-term solution.

Darkspirit commented 5 years ago

See also: https://github.com/rust-embedded/embedded-hal/issues/142

briansmith commented 5 years ago

One particular kind of no_std environment is a freestanding environment as defined by ISO C. PR #842 makes one step towards supporting freestanding environments by removing the dependencies on string.h. That means a user would only need to provide an assert.h replacement in a freestanding environment.

newpavlov commented 5 years ago

(1) ring predates that crate by several years, (2) ring avoids depending on third-party crates to minimize risk, (3) I haven't reviewed the getrandom code, so it's not clear how correct it is. This is something that should be revisited if/when getrandom() actually does make it into libcore.

I hope you'll revisit it even without inclusion into libcore. Otherwise it looks like a duplication of work. Your review will be certainly appreciated and to minimize risks you can pin getrandom version in your Cargo.toml and bump it only after reviewing changes (you'll have to trust crates.io here to not replace code, but you do it either way).

And it will be easier for embedded projects to simply replace a single getrandom crate with a version which supports their device.

josephlr commented 5 years ago

Otherwise it looks like a duplication of work.

@newpavlov most of my recent PRs have been to incorporate some of the good things ring has done in ring::rand into getrandom. I think the last big question concerns issues #852, #316, #326, and #558. Moving to getrandom would immediately resolve those issues. I've reviewed the code, I believe getrandom does "the right thing" to resolve these issues, but @briansmith would (at the very least) want to verify this.

There's also the separate issue that switching to getrandom would make ring start compiling on platforms it doesn't support, but I think that's a minor issue as the README seems to make it clear which platforms ring supports.

briansmith commented 5 years ago

Please take a look at PR #869, which makes libstd an optional dependency even for RSA and the test framework in favor of using alloc.

briansmith commented 5 years ago

Regarding the libc dependency, please see PR #39. Basically we just need to write a little #[cfg(...)] code to decide whether c_int and c_uint are 32 bits or 64 bits. I think the cty crate is overkill for that. (I do 100% support integrating the cty crate into libcore or some other part of the standard library. I just don't want to take the external dependency to define these two types.)

briansmith commented 4 years ago

PR #875 avoids #include <assert.h> in release builds. This means release builds do not (AFAICT) attempt to use any headers that aren't guaranteed to be present in a freestanding implementation. Further, if the C compiler is configured to avoid synthesizing calls to standard library functions (e.g. memset and memcpy), then no C runtime library should be required either.

briansmith commented 4 years ago

PR #879 will remove the libc crate dependency for most platforms.

briansmith commented 4 years ago

"In particular, our end-to-end exploit can leak the entire private key of a secure enclave running on a separate CPU core after only a single digital signature operation." - https://www.vusec.net/projects/crosstalk/

briansmith commented 4 years ago

https://software.intel.com/security-software-guidance/insights/deep-dive-special-register-buffer-data-sampling

stevenrutherford commented 3 years ago

I'm running headlong into a situation where I need no_std support for ring on x86-64. It looks like there was a flurry of effort on this last year, but progress has plateaued a bit. The most comparable thing to what I'm looking for is the work a coworker, @josephlr, proposed for UEFI (but subsequently abandoned).

It sounds to me like there are a few requirements to support no_std on a per-arch basis:

  1. Implementfill_impl for ring::rand.
  2. Avoid depending on external crates, unless it's absolutely unavoidable.
  3. Support testing, ideally in CI.
  4. Deal with feature detection (if you are in an enclave)

Does this list seem accurate?

I mention 2), since I suspect the "easy" answer to writing a fill_impl would be just depending on getrandom across the board. Once getrandom v0.2 shows up, it's actually even somewhat straightfoward to get getrandom to avoid calling cpuid (you can use features to tell getrandom to just use rdrand, and target_features to tell it to assume rdrand exists). Based on my reading of the history here, you would prefer to avoid external crates unless they are vetted (and vetting them takes non-trivial work). Once getrandom v0.2 shows up, would you be willing to depend on getrandom for x86-64 no_std targets?

W.r.t. 3), for uefi/other OS-less environments, things are a bit messy for CI. Most of the ideas I can think of depend on being able to use KVM inside CI, which might be hard if CI is a VM itself (nested virt support is rare). You could skip KVM and just use QEMU, but you'll be forced to emulate which has it's own problems (like the unaligned access emulation issues on arm), and you'll be forced to somehow test ring using a full-blown efi binary (or something similar).

The other option for me is to fork ring, but I'm interested in avoiding the proliferation of ring forks, particularly when the changes I'm interested in aren't particularly invasive.

devsnek commented 10 months ago

@briansmith since this issue was opened I notice that this comment popped up:

// Use the `getrandom` crate whenever it is using the environment's (operating
// system's) CSPRNG. Avoid using it on targets where it uses the `rdrand`
// implementation.

I'm trying to use rustls in an operating system i'm writing, and the missing link seems to be a random source for ring. From this issue, I would expect rdrand to be a reasonable solution here, but this comment seems to imply there might be an issue with it?

I'm also curious about whether supporting the custom feature for getrandom might be reasonable as a way to make bootstrapping platforms a lot simpler.

briansmith commented 10 months ago

@devsnek I mostly agree with the Linux kernel developers on this issue. See the old discussions about Linux. Basically you should have a software CSPRNG in your operating system that is seeded by rdrand and maybe other things. I am hoping that ring will eventually provide this, but right now it doesn't. In the meantime, please provide a syscall like getrandom if you an.

briansmith commented 10 months ago

@stevenrutherford The last time I looked at getrandom's rdrand-based implementation, it was broken; see https://github.com/rust-random/getrandom/issues/228. It looks like the issue was fixed but I haven't evaluated the fix yet. Regardless, like I suggested to @devsnek, a pure rdrand-based implementation of getrandom should be avoided unless there is no other alternative.

We should really "just" implement a CSPRNG that is seeded by rdrand for targets that don't have their own CSPRNG. I would be happy to work with somebody to make that happen.

briansmith commented 9 months ago

It seems getrandom's RDRAND implementation was fixed.

For the time being, we can add a less-safe-getrandom-rdrand feature that lets us use getrandom as it is for UEFI, as I suggest in my review of PR #1406. And the same for other targets where getrandom uses rdrand.

Going forward, I expect ring will provide a "userspace" CSPRNG for use within ring, including the public ring::rand API, specifically to support these targets so we don't have to read RDRAND directly. This is TBD. But the short-term solution outlined above is OK for 0.17.