paritytech / substrate

Substrate: The platform for blockchain innovators
Apache License 2.0
8.39k stars 2.65k forks source link

[RUSTSEC-2021-0013]: Soundness issues in `raw-cpuid` #7979

Closed HCastano closed 3 years ago

HCastano commented 3 years ago

The sc-executor-wasmtime crate is using wasmtime v0.19.0. This in turn is using a vulnerable version of raw-cpuid (v7.0.3). While the wasmtime master branch has bumped raw-cpuid to the recommended v9.0.0 release, they haven't published a new version. When it's available we should update.

Here's the output from cargo-deny in case you're curious:

error[RUSTSEC-2021-0013]: Soundness issues in `raw-cpuid`
    ┌─ /Users/hcastano/Workspace/parity-tech/substrate/Cargo.lock:517:1
    │
517 │ raw-cpuid 7.0.3 registry+https://github.com/rust-lang/crates.io-index
    │ --------------------------------------------------------------------- security vulnerability detected
    │
    = ## Undefined behavior in `as_string()` methods

      `VendorInfo::as_string()`, `SoCVendorBrand::as_string()`,
      and `ExtendedFunctionInfo::processor_brand_string()` construct byte slices
      using `std::slice::from_raw_parts()`, with data coming from
      `#[repr(Rust)]` structs. This is always undefined behavior.

      See https://github.com/gz/rust-cpuid/issues/40.

      This flaw has been fixed in v9.0.0, by making the relevant structs
      `#[repr(C)]`.

      ## `native_cpuid::cpuid_count()` is unsound

      `native_cpuid::cpuid_count()` exposes the unsafe `__cpuid_count()` intrinsic
      from `core::arch::x86` or `core::arch::x86_64` as a safe function, and uses
      it internally, without checking the
      [safety requirement](https://doc.rust-lang.org/core/arch/index.html#overview):

      > The CPU the program is currently running on supports the function being
      > called.

      CPUID is available in most, but not all, x86/x86_64 environments. The crate
      compiles only on these architectures, so others are unaffected.

      This issue is mitigated by the fact that affected programs are expected
      to crash deterministically every time.

      See https://github.com/gz/rust-cpuid/issues/41.

      The flaw has been fixed in v9.0.0, by intentionally breaking compilation
      when targetting SGX or 32-bit x86 without SSE. This covers all affected CPUs.
    = URL: https://github.com/RustSec/advisory-db/pull/614
    = raw-cpuid v7.0.3
      └── cranelift-native v0.66.0
          └── wasmtime-jit v0.19.0
              └── wasmtime v0.19.0
                  └── sc-executor-wasmtime v0.8.1
                      └── sc-executor v0.8.1
disconnect3d commented 3 years ago

Hey, the currently latest release of wasmtime - 0.28.0 has this issue fixed. They actually moved out from using raw-cpuid to use a standard library macro for things they needed raw-cpuid for, in https://github.com/bytecodealliance/wasmtime/pull/2607.

Please update Substrate to the latest wasmtime if possible.

disconnect3d commented 3 years ago

It is also worth to mention that there is another vulnerability that the wasmtime 0.28.0 fixes: https://github.com/bytecodealliance/wasmtime/security/advisories/GHSA-hpqh-2wqx-7qp5 (or CVE-2021-32629). I am not sure if this bug affects Substrate as it is in a Cranelift x64 backend, but still.

HCastano commented 3 years ago

We're on wasmtime 0.27.0 which, while not the latest, does indeed have the fix for the original raw-cpuid issue.

As for your second comment, the security notice mentions cranelift-codegen versions <= 0.73.0 are affected. Looking at Substrate's lockfile we're using cranelift-codegen 0.74.0, so there's no immediate need for us to bump wasmtime.

disconnect3d commented 3 years ago

@HCastano

Looking at Substrate's lockfile we're using cranelift-codegen 0.74.0, so there's no immediate need for us to bump wasmtime.

That is correct. However, this is true for the master branch:

https://github.com/paritytech/substrate/blob/f2b3997004d10512aa7b412b50785e29987f883d/Cargo.lock#L1014-L1030

But not for the latest release (3.0.0): https://github.com/paritytech/substrate/blob/v3.0.0/Cargo.lock#L923-L941

To sum up, the issue is that there were no releases after 3.0.0 (10th February 2021) which contain those dependencies updates.

With this in mind, can this issue issue be reopened and then closed when there is a release including a fix for it, please?

HCastano commented 3 years ago

@disconnect3d I don't think there's anyone using Substrate in production that's running on the v3.0.0 release. Any major project using Substrate (e.g Polkadot) will be on master, or something close to it, in which case this isn't a concern.

It's also unclear when a new formal Substrate release will drop, so we may end up with a solved issue sitting around for ages.

disconnect3d commented 3 years ago

@disconnect3d I don't think there's anyone using Substrate in production that's running on the v3.0.0 release.

In case the 2.x version is still supported (I don't know), it also does not have this fixed.

Any major project using Substrate (e.g Polkadot) will be on master, or something close to it, in which case this isn't a concern.

Does it mean that master is a stable version that can be relied upon? Is this documented anywhere?

From my experience, many projects stay on whatever they got with substrate-node-template or any other example or tutorial they followed, don't update and have vulnerable dependency issues. Now, the node template is based on Substrate's monthly-2021-08 tag which has it already fixed, but still.

HCastano commented 3 years ago

In case the 2.x version is still supported (I don't know), it also does not have this fixed.

The 2.x version is definitely not supported anymore.

Does it mean that master is a stable version that can be relied upon? Is this documented anywhere?

I wouldn't call it stable per say, but it is reasonably reliable. Not sure if this is formally documented anywhere, but as soon as you try and integrate with other parts of the ecosystem (e.g, Polkadot, Cumulus) you'll see that you need to be running a pretty up-to-date version of Substrate.

From my experience, many projects stay on whatever they got with substrate-node-template or any other example or tutorial they followed, don't update and have vulnerable dependency issues.

If we look at production ready projects though, they're typically following more aggressive releases than the monthly tags which are available for the node template. For example, Acala and Moonbeam are using the Substrate version used by Polkadot v0.9.8.