rust-sel4 icon indicating copy to clipboard operation
rust-sel4 copied to clipboard
verified publisher icon seL4

Unsafe Rust in dependencies

Open podhrmic opened this issue 2 years ago • 5 comments

I noticed that ./crates/sel4-platform-info/Cargo.toml depends on serde and serde_yaml which in turn depends on unsafe_libyaml which is a c2rust transpiled unsafe Rust code (which is kind of cool):

cargo tree --manifest-path ./crates/sel4-platform-info/Cargo.toml
...
├── serde v1.0.185 (*)
└── serde_yaml v0.9.25
    ├── indexmap v2.0.0
    │   ├── equivalent v1.0.1
    │   └── hashbrown v0.14.0
    ├── itoa v1.0.9
    ├── ryu v1.0.15
    ├── serde v1.0.185 (*)
    └── unsafe-libyaml v0.2.9

But the real question is - how much time/effort we should put into using tools like cargo-geiger and other maintained by the Rust secure Code WG to increase the trust in the Rust userspace code?

I don't have any great answers, just would like to raise the visibility of this issue!

podhrmic avatar Sep 28 '23 16:09 podhrmic

I went ahead and had some fun with cargo audit:

rust-sel4$ cargo audit --quiet
Crate:     libgit2-sys
Version:   0.14.1+1.5.0
Title:     git2 does not verify SSH keys by default
Date:      2023-01-20
ID:        RUSTSEC-2023-0003
URL:       https://rustsec.org/advisories/RUSTSEC-2023-0003
Solution:  Upgrade to >=0.13.5, <0.14.0 OR >=0.14.2
Dependency tree:
libgit2-sys 0.14.1+1.5.0
├── git2 0.16.0
│   ├── git2-curl 0.17.0
│   │   └── cargo 0.70.1
│   │       └── cargo-helpers 0.1.0
│   └── cargo 0.70.1
└── cargo 0.70.1

Crate:     time
Version:   0.1.45
Title:     Potential segfault in the time crate
Date:      2020-11-18
ID:        RUSTSEC-2020-0071
URL:       https://rustsec.org/advisories/RUSTSEC-2020-0071
Severity:  6.2 (medium)
Solution:  Upgrade to >=0.2.23
Dependency tree:
time 0.1.45
└── chrono 0.4.26
    └── mbedtls-platform-support 0.3.0
        ├── tests-root-task-mbedtls 0.1.0
        └── mbedtls 0.11.0
            ├── tests-root-task-mbedtls 0.1.0
            ├── sel4-async-network-mbedtls 0.1.0
            │   ├── tests-root-task-c 0.1.0
            │   └── microkit-http-server-example-server-core 0.1.0
            │       └── microkit-http-server-example-server 0.1.0
            └── microkit-http-server-example-server-core 0.1.0

Crate:     memmap
Version:   0.7.0
Warning:   unmaintained
Title:     memmap is unmaintained
Date:      2020-12-02
ID:        RUSTSEC-2020-0077
URL:       https://rustsec.org/advisories/RUSTSEC-2020-0077
Dependency tree:
memmap 0.7.0
└── sel4-backtrace-cli 0.1.0

Crate:     rusttype
Version:   0.9.3
Warning:   unmaintained
Title:     rusttype is Unmaintained
Date:      2021-04-01
ID:        RUSTSEC-2021-0140
URL:       https://rustsec.org/advisories/RUSTSEC-2021-0140
Dependency tree:
rusttype 0.9.3
└── banscii-assistant-core 0.1.0
    ├── banscii-assistant-core-test 0.1.0
    └── banscii-assistant 0.1.0

Crate:     atty
Version:   0.2.14
Warning:   unsound
Title:     Potential unaligned read
Date:      2021-07-04
ID:        RUSTSEC-2021-0145
URL:       https://rustsec.org/advisories/RUSTSEC-2021-0145
Dependency tree:
atty 0.2.14
└── clap 3.2.25
    ├── sel4-render-elf-with-data 0.1.0
    │   ├── sel4-kernel-loader-add-payload 0.1.0
    │   ├── sel4-capdl-initializer-add-spec 0.1.0
    │   └── sel4-backtrace-embedded-debug-info-cli 0.1.0
    ├── sel4-kernel-loader-add-payload 0.1.0
    ├── sel4-generate-target-specs 0.1.0
    ├── sel4-capdl-initializer-add-spec 0.1.0
    ├── sel4-bitfield-parser-test 0.1.0
    ├── sel4-backtrace-embedded-debug-info-cli 0.1.0
    ├── sel4-backtrace-cli 0.1.0
    └── cargo-helpers 0.1.0

Crate:     zerocopy
Version:   0.6.3
Warning:   yanked
Dependency tree:
zerocopy 0.6.3
├── virtio-drivers 0.5.0
│   ├── microkit-http-server-example-virtio-net-driver 0.1.0
│   ├── microkit-http-server-example-virtio-hal-impl 0.1.0
│   │   ├── microkit-http-server-example-virtio-net-driver 0.1.0
│   │   └── microkit-http-server-example-virtio-blk-driver 0.1.0
│   └── microkit-http-server-example-virtio-blk-driver 0.1.0
├── sel4-simple-task-runtime-config-types 0.1.0
│   ├── sel4-simple-task-runtime-config-cli 0.1.0
│   └── sel4-simple-task-runtime 0.1.0
│       ├── tests-capdl-utcover-components-test 0.1.0
│       └── tests-capdl-threads-components-test 0.1.0
├── sel4-shared-ring-buffer-block-io-types 0.1.0
│   ├── sel4-shared-ring-buffer-block-io 0.1.0
│   │   ├── microkit-http-server-example-server 0.1.0
│   │   └── meta 0.1.0
│   ├── microkit-http-server-example-virtio-blk-driver 0.1.0
│   ├── microkit-http-server-example-server 0.1.0
│   └── meta 0.1.0
├── sel4-shared-ring-buffer 0.1.0
│   ├── sel4-shared-ring-buffer-smoltcp 0.1.0
│   │   ├── microkit-http-server-example-server 0.1.0
│   │   └── meta 0.1.0
│   ├── sel4-shared-ring-buffer-block-io-types 0.1.0
│   ├── sel4-shared-ring-buffer-block-io 0.1.0
│   ├── microkit-http-server-example-virtio-net-driver 0.1.0
│   ├── microkit-http-server-example-virtio-blk-driver 0.1.0
│   ├── microkit-http-server-example-server 0.1.0
│   └── meta 0.1.0
├── sel4-microkit-message-types 0.1.0
│   ├── sel4-microkit-message 0.1.0
│   │   ├── microkit-http-server-example-virtio-net-driver 0.1.0
│   │   ├── microkit-http-server-example-sp804-driver 0.1.0
│   │   ├── microkit-http-server-example-server 0.1.0
│   │   ├── meta 0.1.0
│   │   ├── banscii-pl011-driver 0.1.0
│   │   ├── banscii-assistant 0.1.0
│   │   └── banscii-artist 0.1.0
│   └── meta 0.1.0
└── sel4-async-block-io-cpiofs 0.1.0
    ├── microkit-http-server-example-server-core 0.1.0
    │   └── microkit-http-server-example-server 0.1.0
    └── meta 0.1.0

error: 2 vulnerabilities found!
warning: 4 allowed warnings found

podhrmic avatar Sep 28 '23 18:09 podhrmic

Something that may be worth considering is supporting configurations that use pre-built code-gen outputs that can be checked into a VCS system and manually inspected.

kent-mcleod avatar Sep 28 '23 23:09 kent-mcleod

I went ahead and had some fun with cargo audit:

cargo-audit should definitely be part of the project's workflow! I've opened https://github.com/seL4/rust-sel4/pull/22, which adds cargo-audit to CI and addresses these vulnerabilities. I've also opened https://github.com/seL4/rust-sel4/issues/23.

nspin avatar Oct 03 '23 01:10 nspin

I noticed that ./crates/sel4-platform-info/Cargo.toml depends on serde and serde_yaml which in turn depends on unsafe_libyaml which is a c2rust transpiled unsafe Rust code (which is kind of cool)

Interesting. From a risk-management perspective, I feel like this crate's approach isn't fundamentally different from that of a crate which wraps and links against a C library. That said, I would expect the unsafe_libyaml crate to include a clear, ideally executable and reproducible, description of where the C code comes from and exactly how it is transpiled into Rust. I'm surprised not to see even a mention of the libyaml version used nor the version of c2rust used.

Given that serde_yaml is so widely used in the Rust ecosystem, is the only widely used YAML crate for serde, and is a build-time dependency that is only fed trusted input (just platform_gen.yaml), I'm not immediately concerned about its presence in our dependency tree, but we I agree that we should be keeping an eye on this sort of thing.

But the real question is - how much time/effort we should put into using tools like cargo-geiger and other maintained by the Rust secure Code WG to increase the trust in the Rust userspace code?

Great question. I think that applying these tools to the exports of this projects is a especially important. By exports, I mean crates which are visible to users of this project, rather than those which are just used internally to support testing and development. Furthermore, I think that, for certain types of signals, we will get more bang for our buck by being more sensitive to dependencies of crates that run at run-time than dependencies that just run at build-time. For example, I think that stats about unsafe are more meaningful when they are about code that runs in seL4 userspace, as opposed to those about code which just runs at build-time.

In general, I think one key to managing the risk that these tools help us assess is being judicious with permitting run-time dependencies of the most import crates. For example, here is the dependency tree of sel4-microkit:

$ cargo tree -e normal -e no-proc-macro -p sel4-microkit
sel4-microkit v0.1.0 (/home/x/i/rust-sel4/crates/sel4-microkit)
├── cfg-if v1.0.0
├── sel4 v0.1.0 (/home/x/i/rust-sel4/crates/sel4)
│   ├── cfg-if v1.0.0
│   ├── sel4-config v0.1.0 (/home/x/i/rust-sel4/crates/sel4/config)
│   └── sel4-sys v0.1.0 (/home/x/i/rust-sel4/crates/sel4/sys)
│       ├── log v0.4.20
│       ├── sel4-bitfield-types v0.1.0 (/home/x/i/rust-sel4/crates/sel4/bitfield-types)
│       └── sel4-config v0.1.0 (/home/x/i/rust-sel4/crates/sel4/config)
├── sel4-externally-shared v0.1.0 (/home/x/i/rust-sel4/crates/sel4-externally-shared)
├── sel4-immediate-sync-once-cell v0.1.0 (/home/x/i/rust-sel4/crates/sel4-immediate-sync-once-cell)
├── sel4-immutable-cell v0.1.0 (/home/x/i/rust-sel4/crates/sel4-immutable-cell)
├── sel4-panicking v0.1.0 (/home/x/i/rust-sel4/crates/sel4-panicking)
│   ├── cfg-if v1.0.0
│   ├── sel4-immediate-sync-once-cell v0.1.0 (/home/x/i/rust-sel4/crates/sel4-immediate-sync-once-cell)
│   ├── sel4-panicking-env v0.1.0 (/home/x/i/rust-sel4/crates/sel4-panicking/env)
│   └── unwinding v0.1.7
│       └── gimli v0.26.2
├── sel4-panicking-env v0.1.0 (/home/x/i/rust-sel4/crates/sel4-panicking/env)
└── sel4-runtime-common v0.1.0 (/home/x/i/rust-sel4/crates/sel4-runtime-common)
    ├── cfg-if v1.0.0
    ├── sel4-dlmalloc v0.1.0 (/home/x/i/rust-sel4/crates/sel4-dlmalloc)
    │   ├── dlmalloc v0.2.4
    │   │   └── libc v0.2.148
    │   └── sel4-sync v0.1.0 (/home/x/i/rust-sel4/crates/sel4-sync)
    │       ├── sel4 v0.1.0 (/home/x/i/rust-sel4/crates/sel4) (*)
    │       └── sel4-immediate-sync-once-cell v0.1.0 (/home/x/i/rust-sel4/crates/sel4-immediate-sync-once-cell)
    ├── sel4-initialize-tls-on-stack v0.1.0 (/home/x/i/rust-sel4/crates/sel4-initialize-tls-on-stack)
    │   ├── cfg-if v1.0.0
    │   └── sel4 v0.1.0 (/home/x/i/rust-sel4/crates/sel4) (*)
    ├── sel4-sync v0.1.0 (/home/x/i/rust-sel4/crates/sel4-sync) (*)
    └── unwinding v0.1.7 (*)

I think this looks pretty good. These are the only external runtime dependencies:

  • cfg-if
  • log
  • unwinding (Only when unwinding feature is enabled. Full of unsafe code, of course, but provides crucial functionality that we would implement in the very same way.)
    • gimli
  • dlmalloc (Only when alloc feature is enabled. Notably also a dependency of libstd, used on Wasm targets.)
    • libc (Does not actually introduce or link against any C code. For our target platform, it is just a few FFI type declarations.)

nspin avatar Oct 03 '23 02:10 nspin

Something that may be worth considering is supporting configurations that use pre-built code-gen outputs that can be checked into a VCS system and manually inspected.

@kent-mcleod can you elaborate on the sort of build flow you are envisioning? The generated Rust code in sel4-sys is quite configuration-dependent. Do you imagine downstream developers checking generated Rust code in for each of the configurations they are developing with?

Also, do you think that a feature like this would also be worthwhile in the C case, or is there something about Rust case in particular that necessitates this? At the Summit, you mentioned the dependency on bindgen.

nspin avatar Oct 03 '23 11:10 nspin