Skip to content

Commit

Permalink
Explain why there is no use after free vulnerability
Browse files Browse the repository at this point in the history
When looking at the code I initially thought that commit
b8f0031 introduced a use after free
vulnerability.  It does not, but this was not clear from reading the
code, so add comments to explain why the buffer (unlike stdin_buf) can
just be uninitialized memory and will always be valid throughout
process_io() until it is freed.

Also avoid allocating two buffers when one will do.
  • Loading branch information
DemiMarie committed May 1, 2024
1 parent a2e0deb commit 8f2539a
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 9 deletions.
11 changes: 2 additions & 9 deletions libqrexec/process_io.c
Original file line number Diff line number Diff line change
Expand Up @@ -110,12 +110,6 @@ int process_io(const struct process_io_request *req) {
};
if (remote_buffer.data == NULL)
handle_vchan_error("remote buffer alloc");
struct buffer stdin_buffer = {
.data = malloc(max_chunk_size),
.buflen = max_chunk_size,
};
if (stdin_buffer.data == NULL)
handle_vchan_error("stdin buffer alloc");

sigemptyset(&pollmask);
sigaddset(&pollmask, SIGCHLD);
Expand Down Expand Up @@ -279,7 +273,7 @@ int process_io(const struct process_io_request *req) {
if (prefix.len > 0 || (stdout_fd >= 0 && fds[FD_STDOUT].revents)) {
switch (handle_input_v2(
vchan, stdout_fd, stdout_msg_type,
&prefix, &stdin_buffer)) {
&prefix, &remote_buffer)) {
case REMOTE_ERROR:
handle_vchan_error("send(handle_input stdout)");
break;
Expand All @@ -291,7 +285,7 @@ int process_io(const struct process_io_request *req) {
if (stderr_fd >= 0 && fds[FD_STDERR].revents) {
switch (handle_input_v2(
vchan, stderr_fd, MSG_DATA_STDERR,
&empty, &stdin_buffer)) {
&empty, &remote_buffer)) {
case REMOTE_ERROR:
handle_vchan_error("send(handle_input stderr)");
break;
Expand Down Expand Up @@ -321,7 +315,6 @@ int process_io(const struct process_io_request *req) {
}

free(remote_buffer.data);
free(stdin_buffer.data);

if (!is_service && remote_status)
return remote_status;
Expand Down
17 changes: 17 additions & 0 deletions libqrexec/remote.h
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,17 @@
* Options:
* replace_chars_stdout, replace_chars_stderr - remove non-printable
* characters from stdout/stderr
*
* stdin_buf is the buffer holding data to be written to stdin, and may
* be modified or reallocated. Its content must be valid data, and on
* return it will be a valid buffer pointing to valid (generally different)
* data.
*
* buffer is scratch space _only_. Its size must be the maximum data
* chunk size. Its contents (the data pointed to) does _not_ need to be
* initialized, and will _not_ be anything meaningful on return. The
* buffer pointer and length will not be changed and will remain valid,
* though. In Rust terms: this is an &mut [MaybeUninit<u8>].
*/
int handle_remote_data_v2(
libvchan_t *data_vchan, int stdin_fd, int *status,
Expand All @@ -56,6 +67,12 @@ int handle_remote_data_v2(
* REMOTE_EOF - EOF received, do not access this FD again
* REMOTE_OK - some data processed, call it again when buffer space and
* more data availabla
*
* buffer is scratch space _only_. Its size must be the maximum data
* chunk size. Its contents (the data pointed to) does _not_ need to be
* initialized, and will _not_ be anything meaningful on return. The
* buffer pointer and length will not be freed or reallocated, though.
* In Rust terms: this is an &mut [MaybeUninit<u8>].
*/
int handle_input_v2(
libvchan_t *vchan, int fd, int msg_type,
Expand Down

0 comments on commit 8f2539a

Please sign in to comment.