[libvirt] [PATCH 0/2] libvirt-rust: Fixing bugs in Stream::{send, recv}
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
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
participants (2)
-
Linus Färnstrand -
Martin Kletzander