BorshDeserialize can cause UB by copying zero sized objects with no safe Copy impl

[dtolnay]

Repro:

mod singleton {
    use std::sync::atomic::{AtomicBool, Ordering};

    // Important: no Copy or Clone impl
    pub struct Handle(());

    impl Handle {
        pub fn get() -> Self {
            static EXISTS: AtomicBool = AtomicBool::new(false);
            if EXISTS.swap(true, Ordering::Relaxed) {
                panic!("one singleton handle has already been created");
            } else {
                Handle(())
            }
        }

        // Proof of exclusive access to a handle grants exclusive access to some
        // corresponding underlying state. It's guaranteed that at most one
        // handle exists in the entire program so this is sound.
        pub fn access(&mut self) -> &mut State {
            static mut STATE: State = None;
            unsafe { &mut STATE }
        }
    }

    type State = Option<String>;
}

use borsh::BorshDeserialize;
use std::io::Result;

struct Wrap(singleton::Handle);

impl BorshDeserialize for Wrap {
    fn deserialize(_buf: &mut &[u8]) -> Result<Self> {
        Ok(Wrap(singleton::Handle::get()))
    }
}

fn main() {
    let mut x = Vec::<Wrap>::deserialize(&mut &[2, 0, 0, 0][..]).unwrap();
    let (first, rest) = x.split_first_mut().unwrap();
    *first.0.access() = Some("...................".to_owned());
    let s = rest[0].0.access().as_mut().unwrap();
    *first.0.access() = None;
    println!("{}", s);
}

The result is segfault or other corruption:

$ cargo run --release
Segmentation fault (core dumped)

$ cargo run
thread 'main' panicked at 'failed printing to stdout: Bad address (os error 14)', library/std/src/io/stdio.rs:940:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

[frol]

Good call! Indeed, this unsafe is not playing well:

https://github.com/near/borsh-rs/blob/b5104293d8e1f6641da1dc1c3c554390e34563b6/borsh/src/de/mod.rs#L274-L284

@ailisp It seems that you have some experience with this area (https://github.com/near/borsh/pull/41). Do you have the capacity to work on this issue?

[evgenykuzyakov]

I think there is no need to optimize zero size structure, so let's just use a loop here to create it. Obviously it would add an ability to create an a very large loop that wouldn't be limited by the input data size.

[MaksymZavershynskyi]

Can this be closed @frol @matklad ?

[dtolnay]

Given that the repro code in https://github.com/near/borsh-rs/issues/19#issue-831009862 still compiles and still segfaults, and the bad unsafe in https://github.com/near/borsh-rs/issues/19#issuecomment-799594411 is still present in master, I would say no.

[frol]

Nope, we still need to address it

[westy92]

FYI this issue is now in the GitHub Advisory Database with Moderate severity: https://github.com/advisories/GHSA-fjx5-qpf4-xjf2

Also in RUSTSEC: https://rustsec.org/advisories/RUSTSEC-2023-0033.html

Is there a plan to address this?

[frol]

@westy92 I will be happy to review a pull request. The issue is hypothetical, and you really need to setup yourself into a trouble to exercise it, so given the limited resources (no active contributors to this project), it is not going to be fixed any time soon unless someone from the community contributes the fix.

[frol]

A vector of zero-sized types is pointless in the context of serialization/deserialization in my opinion, so I believe the best thing is to just forbid collections of zero-size elements (https://github.com/near/borsh-rs/pull/136#issuecomment-1561993704). I am wondering if there is a way to do it through the type system (in the worst case, we will do it at runtime).