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

[Martin Kletzander]

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 at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190920/93055d4f/attachment-0001.sig>