libvirt-rust: Fixing bugs in Stream::{send, recv}

[libvirt] [PATCH 0/2] libvirt-rust: Fixing bugs in Stream::{send, recv}

Posted by Linus Färnstrand 4 years, 7 months ago

Hi,

Fixing memory soundness bugs in Stream::recv, and other bugs in both
Stream::recv and Stream::send.

The method Stream::recv takes a size argument that it passes on as the nbytes
argument to virStreamRecv. The problem is that the corresponding data pointer
points to a local stack allocated array with a fixed size of 2048. So calling
Stream::recv with a size larger than 2048 can cause virStreamRecv to write
past the given buffer!

The library calls CStr::from_ptr(data) on the locally allocated buffer after
virStreamRecv returns. CStr::from_ptr will scan the memory from the given
pointer for the terminating null byte. There is no guarantee this buffer will
properly end with a null byte. The buffer is initialized with only nulls.
But since virStreamRecv can overwrite the entire buffer (and beyond!), there
is no guarantee there will be a null byte in the buffer at this point. As a
result, CStr::from_ptr can continue to read past the buffer. This can cause
either reading invalid memory, or it can cause the method to return a string
containing data from the stack way past the data written by virStreamRecv.
Heartbleed style issue.

The code does not check for the -2 error code in the return value of
virStreamRecv, treating it as a success. It looks like it will make it just
return an empty string as result. But a bug nonetheless.

After parsing the buffer as a C string in Stream::recv, it is lossily converted
into a Rust UTF-8 string with CStr::to_string_lossy. It can cause the data to
be modified before it is returned to the caller.
U+FFFD REPLACEMENT CHARACTER will be inserted on invalid UTF-8 codepoints.

The FFI bindings for both virStreamRecv and virStreamSend are wrong. They
specify the nbytes argument as c_int instead of size_t as the C library does.

I don't have access to SMTP on my dev machine. Hope ProtonMail does not
reformat this.

Have a great day
Linus Färnstrand

Linus Färnstrand (2):
  Fix bugs in Stream::recv
  Fix Stream::send

 src/stream.rs | 42 +++++++++++++++++++++---------------------
 1 file changed, 21 insertions(+), 21 deletions(-)

--
2.21.0


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 0/2] libvirt-rust: Fixing bugs in Stream::{send, recv}

Posted by Martin Kletzander 4 years, 7 months ago

On Thu, Sep 19, 2019 at 05:48:21AM +0000, Linus Färnstrand wrote:
>Hi,
>
>Fixing memory soundness bugs in Stream::recv, and other bugs in both
>Stream::recv and Stream::send.
>
>The method Stream::recv takes a size argument that it passes on as the nbytes
>argument to virStreamRecv. The problem is that the corresponding data pointer
>points to a local stack allocated array with a fixed size of 2048. So calling
>Stream::recv with a size larger than 2048 can cause virStreamRecv to write
>past the given buffer!
>
>The library calls CStr::from_ptr(data) on the locally allocated buffer after
>virStreamRecv returns. CStr::from_ptr will scan the memory from the given
>pointer for the terminating null byte. There is no guarantee this buffer will
>properly end with a null byte. The buffer is initialized with only nulls.
>But since virStreamRecv can overwrite the entire buffer (and beyond!), there
>is no guarantee there will be a null byte in the buffer at this point. As a
>result, CStr::from_ptr can continue to read past the buffer. This can cause
>either reading invalid memory, or it can cause the method to return a string
>containing data from the stack way past the data written by virStreamRecv.
>Heartbleed style issue.
>
>The code does not check for the -2 error code in the return value of
>virStreamRecv, treating it as a success. It looks like it will make it just
>return an empty string as result. But a bug nonetheless.
>
>After parsing the buffer as a C string in Stream::recv, it is lossily converted
>into a Rust UTF-8 string with CStr::to_string_lossy. It can cause the data to
>be modified before it is returned to the caller.
>U+FFFD REPLACEMENT CHARACTER will be inserted on invalid UTF-8 codepoints.
>
>The FFI bindings for both virStreamRecv and virStreamSend are wrong. They
>specify the nbytes argument as c_int instead of size_t as the C library does.
>
>I don't have access to SMTP on my dev machine. Hope ProtonMail does not
>reformat this.
>

It's fine.  Just for a future reference, we prefer shallow formatting, basically
what I have is:

  git config format.thread shallow
  git config format.coverLetter auto

and then per-project:

  git config format.subjectprefix "libvirt-rust PATCH"

>Have a great day
>Linus Färnstrand
>
>Linus Färnstrand (2):
>  Fix bugs in Stream::recv
>  Fix Stream::send
>
> src/stream.rs | 42 +++++++++++++++++++++---------------------
> 1 file changed, 21 insertions(+), 21 deletions(-)
>
>--
>2.21.0
>
>
>--
>libvir-list mailing list
>libvir-list@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list