Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Undefined data exposed in DeflateOutput's AsyncRead implementation #1932

Closed
ammaraskar opened this issue Jan 24, 2021 · 0 comments · Fixed by #1933
Closed

Undefined data exposed in DeflateOutput's AsyncRead implementation #1932

ammaraskar opened this issue Jan 24, 2021 · 0 comments · Fixed by #1933

Comments

@ammaraskar
Copy link

Hi there, we (Rust group @sslab-gatech) are scanning crates on crates.io for potential soundness bugs. We noticed that DeflateOutput's implementation of poll_read does the following:

unsafe {
this.read_interm.reserve(256);
this.read_interm.set_len(this.read_interm.capacity());
}
match AsyncRead::poll_read(Pin::new(&mut this.inner), cx, &mut this.read_interm) {

This sets up uninitialized bytes in read_interm and then passes it to the user provided poll_read method. This allows invoking undefined behavior from safe Rust code, reading uninitialized memory.

This issue is described a bit in the documentation for Read:

It is your responsibility to make sure that buf is initialized before calling read. Calling read with an uninitialized buf (of the kind one obtains via MaybeUninit<T>) is not safe, and can lead to undefined behavior.

Here's an example that outputs uninitialized memory using this:

Click to expand
#![forbid(unsafe_code)]

use libp2p_core::upgrade::{InboundUpgrade, ProtocolName};
use libp2p_deflate::DeflateConfig;

use futures::executor;
use futures::io::{AsyncRead, AsyncReadExt, AsyncWrite, Error};
use futures::task::{Context, Poll};
use std::pin::Pin;

struct BrokenReader();

impl AsyncRead for BrokenReader {
    fn poll_read(
        self: Pin<&mut Self>,
        cx: &mut Context,
        buf: &mut [u8],
    ) -> Poll<Result<usize, Error>> {
        // Dump out uninitialized memory
        println!("{:?}", buf);
        assert!(buf[0] == 0);

        return Poll::Ready(Ok(buf.len()));
    }
}

impl AsyncWrite for BrokenReader {
    fn poll_write(
        self: Pin<&mut Self>,
        cx: &mut Context,
        buf: &[u8],
    ) -> Poll<Result<usize, Error>> {
        return Poll::Ready(Ok(buf.len()));
    }

    fn poll_flush(self: Pin<&mut Self>, cx: &mut Context) -> Poll<Result<(), Error>> {
        return Poll::Ready(Ok(()));
    }

    fn poll_close(self: Pin<&mut Self>, cx: &mut Context) -> Poll<Result<(), Error>> {
        return Poll::Ready(Ok(()));
    }
}

fn main() {
    executor::block_on(async {
        let broken_reader = BrokenReader();
        let deflate_config = DeflateConfig::default();

        let mut deflate_output = deflate_config
            .upgrade_inbound(broken_reader, "/test/1".as_bytes())
            .await
            .unwrap();

        let mut buf = [1u8; 256];
        deflate_output.read_exact(&mut buf).await.unwrap();
    });
}
@romanb romanb linked a pull request Jan 25, 2021 that will close this issue
bors bot added a commit to casper-network/casper-node that referenced this issue Jan 28, 2021
820: NO-TICKET: upgrade libp2p deflate r=Fraser999 a=Fraser999

This fixes security advisory on libp2p-deflate by upgrading the version of libp2p. See libp2p/rust-libp2p#1932.

Co-authored-by: Michał Papierski <[email protected]>
Co-authored-by: Fraser Hutchison <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant