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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

Attachment: signature.asc
Description: PGP signature

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

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux