Multiple unsoundness problems in aya-ebpf/programs

[lwz23]

hello, thank you for your contribution in this project, I am scanning the unsoundness problem in rust project.
I notice the following code:

pub struct SkBuff {
    pub skb: *mut __sk_buff,
}


impl SkBuff {
    pub fn new(skb: *mut __sk_buff) -> SkBuff {
        SkBuff { skb }
    }

    #[allow(clippy::len_without_is_empty)]
    #[inline]
    pub fn len(&self) -> u32 {
        unsafe { (*self.skb).len }
    }
............................
}

Considering that programs is a pub mod and skb is a pub field, I assume that users can directly manipulate this field, and that len is a public function. This potential situation could result in self.skb being a null pointer, and directly dereferencing it might trigger undefined behavior (UB). For safety reasons, I felt it necessary to report this issue. If you have performed checks elsewhere that ensure this is safe, please don’t take offense at my raising this issue.
Sorry, I can't provide a Poc, because trying to import SkBuff in my environment will get an error:

error[E0432]: unresolved import `aya::programs::SkBuff`
 --> src/main.rs:3:5
  |
3 | use aya::programs::SkBuff;
  |     ^^^^^^^^^^^^^^^^^^^^^ no `SkBuff` in `programs`

For more information about this error, try `rustc --explain E0432`.
error: could not compile `lwz` (bin "lwz") due to 1 previous error

If this can be solved, I am happy to provide a Poc.

[lwz23]

same problem for

aya/ebpf/aya-ebpf/src/programs/sk_msg.rs

Line 9 in 5a43bed

pub struct SkMsgContext {

[lwz23]

and maybe same problem for

pub struct ProbeContext {
    pub regs: *mut pt_regs,
}

impl ProbeContext {
    pub fn new(ctx: *mut c_void) -> ProbeContext {
        ProbeContext {
            regs: ctx as *mut pt_regs,
        }
    }
    pub fn arg<T: FromPtRegs>(&self, n: usize) -> Option<T> {
        T::from_argument(unsafe { &*self.regs }, n)
    }
}

[lwz23]

If we need fix it maybe we should add check for null pointer in new method too :)

[vadorovsky]

I think the proper fix would be to use NonNull type for these fields in context types and making sure it's always private. We could provide as_ptr() -> *mut pt_regs (or whatever pointer type is wrapped by the given context) method for callers who want to do unsafe stuff with the context, or call some BPF helper directly.

[vadorovsky]

A pull request is very welcome! 🙂 Your build error looks weird, but hard to debug for me, since it's your own project where you're using Aya as an external crate. Would you mind sharing the code, or at least Cargo.toml?

That said, I think our integration tests should be good enough for you to make and test the change just using the main Aya repo.

[lwz23]

ofcourse, here is my main.rs:

extern crate aya;

use aya::programs::SkBuff;
fn main() {
    
}

and Cargo.toml:

[package]
name = "lwz"
version = "0.1.0"
authors = ["lwz"]
edition = "2018"

[dependencies]
aya = "0.13.1"

result:

error[E0432]: unresolved import `aya::programs::SkBuff`
 --> src/main.rs:3:5
  |
3 | use aya::programs::SkBuff;
  |     ^^^^^^^^^^^^^^^^^^^^^ no `SkBuff` in `programs`

For more information about this error, try `rustc --explain E0432`.
error: could not compile `lwz` (bin "lwz") due to 1 previous error
lwz@LAPTOP-F68E79VH:/mnt/e/Github/lwz$

[vadorovsky]

You're using a wrong crate. SkBuff is a part of aya-ebpf, not aya. It's used in eBPF programs, not in the regular user-space code.

I would recommend to read through our docs (https://aya-rs.dev/) and make it to the Classifiers section, where the SkBuff is being used as a part of TcContext.

[lwz23]

Ok, I will take a look at that. And another case

aya/ebpf/aya-ebpf/src/programs/retprobe.rs

Line 21 in 1dfcfbc

impl RetProbeContext {

[lwz23]

and

aya/ebpf/aya-ebpf/src/programs/sk_buff.rs

Line 263 in 1dfcfbc

pub struct SkBuffContext {

I'm not sure if I've found all of these bugs, but it seems to be a common problem in this library. Maybe my program is underreporting. But that's probably all the cases I could find.

[vadorovsky]

Yeah, if you find any other structs wrapping a raw pointer, it totally makes sense to ensure it's NonNull and private.

[lwz23]

Considering this is a unsound problem and this crate is published on crates.io, I suggest maybe we should report it to RustSec?

[ajwerner]

Considering this is a unsound problem and this crate is published on crates.io, I suggest maybe we should report it to RustSec?

I doubt it. This crate (aya-ebpf) can only be compiled with nightly compiler targeting bpfeb-unknown-none and bpfel-unknown-none, which are Tier 3 platforms.