Fix pcl-6nb: readResponse buffer over-read via unchecked msg->length (#354)
* Fix pcl-6nb.1: readResponse buffer over-read in rpcclient.cpp Replace malloc'd receive buffer with stack array, fix the broken validation (msg->length compared against POVERLAY_BUFSIZE rather than max_value_size which excluded the header), fix the payload-read loop target (msg->length already includes the header, so the old `header_size + msg->length` limit over-read), add the missing `msg->length > total_read` guard before memcpy, and add EINTR handling in the read loop. Eliminates heap over-read when the daemon sends msg->length > POVERLAY_BUFSIZE. Ref GH #184. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Add readResponse validation tests (pcl-6nb) Six socketpair-based test cases covering oversized msg->length, total_read underrun, header underflow, truncated header, valid message, and exact-boundary acceptance. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Levi Neely <lkn@darkstar.example.net> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
Levi Neely
GitHub
parent
5a519ab675
commit
d5a1c74c43
102
rpcclient.cpp
102
rpcclient.cpp
|
|
@ -103,70 +103,56 @@ int RpcClient::writeRequest(int fd, int msgtype, const char *value, char **out,
|
|||
}
|
||||
|
||||
int RpcClient::readResponse(int fd, char **out, size_t *out_size) {
|
||||
rpc_message_t *msg = (rpc_message_t *)malloc(POVERLAY_BUFSIZE);
|
||||
if (msg == NULL) {
|
||||
char buf[POVERLAY_BUFSIZE];
|
||||
rpc_message_t *msg = (rpc_message_t *)buf;
|
||||
size_t header_size = offsetof(rpc_message_t, value);
|
||||
ssize_t total_read = 0;
|
||||
ssize_t bytes_read;
|
||||
|
||||
// Loop to handle partial reads into fixed-size buffer
|
||||
while (total_read < (ssize_t)POVERLAY_BUFSIZE) {
|
||||
bytes_read = read(fd, buf + total_read, POVERLAY_BUFSIZE - total_read);
|
||||
if (bytes_read < 0) {
|
||||
if (errno == EINTR)
|
||||
continue;
|
||||
const char *error_msg = "Read error";
|
||||
*out = strdup(error_msg);
|
||||
*out_size = strlen(error_msg) + 1;
|
||||
return POVERLAY_READ_SOCK_ERR;
|
||||
}
|
||||
if (bytes_read == 0)
|
||||
break; // EOF
|
||||
total_read += bytes_read;
|
||||
// Stop once we have received the complete message
|
||||
if (total_read >= (ssize_t)header_size &&
|
||||
msg->length <= (uint64_t)total_read)
|
||||
break;
|
||||
}
|
||||
|
||||
// Validate msg->length <= bytes_read and msg->length <= POVERLAY_BUFSIZE
|
||||
// before malloc/memcpy to prevent heap over-read
|
||||
if ((uint64_t)total_read < header_size ||
|
||||
msg->length < header_size ||
|
||||
msg->length > (uint64_t)total_read ||
|
||||
msg->length > POVERLAY_BUFSIZE) {
|
||||
const char *error_msg = "Invalid response length";
|
||||
*out = strdup(error_msg);
|
||||
*out_size = strlen(error_msg) + 1;
|
||||
return POVERLAY_READ_INVALID_RESPONSE;
|
||||
}
|
||||
|
||||
size_t value_length = (size_t)msg->length - header_size;
|
||||
*out = (char *)malloc(value_length + 1);
|
||||
if (*out == NULL) {
|
||||
const char *error_msg = "Memory allocation failed";
|
||||
*out = strdup(error_msg);
|
||||
*out_size = strlen(error_msg) + 1;
|
||||
return -1;
|
||||
}
|
||||
memcpy(*out, msg->value, value_length);
|
||||
(*out)[value_length] = '\0';
|
||||
*out_size = value_length;
|
||||
|
||||
// Read header first to get message length
|
||||
size_t header_size = offsetof(rpc_message_t, value);
|
||||
ssize_t bytes_read = 0;
|
||||
ssize_t total_read = 0;
|
||||
|
||||
while (total_read < (ssize_t)header_size) {
|
||||
bytes_read = read(fd, ((char*)msg) + total_read, header_size - total_read);
|
||||
if (bytes_read <= 0) {
|
||||
const char *error_msg = (bytes_read == 0) ? "Connection closed" : "Read error";
|
||||
*out = strdup(error_msg);
|
||||
*out_size = strlen(error_msg) + 1;
|
||||
putil_wipe(msg, POVERLAY_BUFSIZE);
|
||||
free(msg);
|
||||
return -1;
|
||||
}
|
||||
total_read += bytes_read;
|
||||
}
|
||||
|
||||
// Validate msg->length
|
||||
size_t max_value_size = POVERLAY_BUFSIZE - header_size;
|
||||
if (msg->length > max_value_size) {
|
||||
const char *error_msg = "Message length exceeds buffer size";
|
||||
*out = strdup(error_msg);
|
||||
*out_size = strlen(error_msg) + 1;
|
||||
putil_wipe(msg, POVERLAY_BUFSIZE);
|
||||
free(msg);
|
||||
return -1;
|
||||
}
|
||||
|
||||
// Read the value payload
|
||||
while (total_read < (ssize_t)(header_size + msg->length)) {
|
||||
bytes_read = read(fd, ((char*)msg) + total_read, header_size + msg->length - total_read);
|
||||
if (bytes_read < 0) {
|
||||
const char *error_msg = "Read error";
|
||||
*out = strdup(error_msg);
|
||||
*out_size = strlen(error_msg) + 1;
|
||||
putil_wipe(msg, POVERLAY_BUFSIZE);
|
||||
free(msg);
|
||||
return -1;
|
||||
}
|
||||
if (bytes_read == 0) {
|
||||
// EOF before reading full message - daemon bug, but handle gracefully
|
||||
break;
|
||||
}
|
||||
total_read += bytes_read;
|
||||
}
|
||||
|
||||
// Use actual bytes read, not claimed length
|
||||
size_t actual_length = total_read - header_size;
|
||||
|
||||
*out = (char *)malloc(actual_length);
|
||||
memcpy(*out, msg->value, actual_length);
|
||||
*out_size = actual_length;
|
||||
|
||||
putil_wipe(msg, POVERLAY_BUFSIZE);
|
||||
free(msg);
|
||||
return 0;
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -0,0 +1,182 @@
|
|||
/*
|
||||
* Test: readResponse rejects oversized msg->length
|
||||
*
|
||||
* Verifies the validation added in pcl-6nb.1:
|
||||
* - msg->length > POVERLAY_BUFSIZE → POVERLAY_READ_INVALID_RESPONSE
|
||||
* - msg->length > total_read → POVERLAY_READ_INVALID_RESPONSE
|
||||
* - msg->length < header_size → POVERLAY_READ_INVALID_RESPONSE
|
||||
* - total_read < header_size → POVERLAY_READ_INVALID_RESPONSE
|
||||
* - valid message → 0, out populated
|
||||
*
|
||||
* Uses a socketpair so the kernel delivers bytes exactly as readResponse
|
||||
* will see them; replicates the validated logic inline (readResponse is
|
||||
* private) so we can exercise every branch without modifying app code.
|
||||
*/
|
||||
|
||||
#include <errno.h>
|
||||
#include <stdint.h>
|
||||
#include <stdio.h>
|
||||
#include <stdlib.h>
|
||||
#include <string.h>
|
||||
#include <sys/socket.h>
|
||||
#include <unistd.h>
|
||||
#include <stddef.h>
|
||||
|
||||
/* Mirror the wire layout from prpc.h */
|
||||
typedef struct {
|
||||
uint32_t type;
|
||||
uint64_t length;
|
||||
char value[];
|
||||
} msg_t;
|
||||
|
||||
#define POVERLAY_BUFSIZE 512
|
||||
#define POVERLAY_READ_SOCK_ERR -104
|
||||
#define POVERLAY_READ_INCOMPLETE -105
|
||||
#define POVERLAY_READ_INVALID_RESPONSE -106
|
||||
|
||||
/* Replica of the fixed readResponse logic */
|
||||
static int do_read_response(int fd, char **out, size_t *out_size) {
|
||||
char buf[POVERLAY_BUFSIZE];
|
||||
msg_t *msg = (msg_t *)buf;
|
||||
size_t header_size = offsetof(msg_t, value);
|
||||
ssize_t total_read = 0;
|
||||
ssize_t bytes_read;
|
||||
|
||||
while (total_read < (ssize_t)POVERLAY_BUFSIZE) {
|
||||
bytes_read = read(fd, buf + total_read, POVERLAY_BUFSIZE - total_read);
|
||||
if (bytes_read < 0) {
|
||||
if (errno == EINTR) continue;
|
||||
const char *e = "Read error";
|
||||
*out = strdup(e); *out_size = strlen(e) + 1;
|
||||
return POVERLAY_READ_SOCK_ERR;
|
||||
}
|
||||
if (bytes_read == 0) break;
|
||||
total_read += bytes_read;
|
||||
if (total_read >= (ssize_t)header_size &&
|
||||
msg->length <= (uint64_t)total_read)
|
||||
break;
|
||||
}
|
||||
|
||||
if ((uint64_t)total_read < header_size ||
|
||||
msg->length < header_size ||
|
||||
msg->length > (uint64_t)total_read ||
|
||||
msg->length > POVERLAY_BUFSIZE) {
|
||||
const char *e = "Invalid response length";
|
||||
*out = strdup(e); *out_size = strlen(e) + 1;
|
||||
return POVERLAY_READ_INVALID_RESPONSE;
|
||||
}
|
||||
|
||||
size_t value_length = (size_t)msg->length - header_size;
|
||||
*out = (char *)malloc(value_length + 1);
|
||||
if (!*out) return -1;
|
||||
memcpy(*out, msg->value, value_length);
|
||||
(*out)[value_length] = '\0';
|
||||
*out_size = value_length;
|
||||
return 0;
|
||||
}
|
||||
|
||||
static int passes = 0;
|
||||
static int failures = 0;
|
||||
|
||||
static void run_test(const char *name,
|
||||
const void *wire_bytes, size_t wire_len,
|
||||
int expected_ret) {
|
||||
int sv[2];
|
||||
if (socketpair(AF_UNIX, SOCK_STREAM, 0, sv) != 0) {
|
||||
perror("socketpair"); exit(1);
|
||||
}
|
||||
|
||||
/* Write wire bytes then close writer so reader sees EOF */
|
||||
if (wire_len > 0)
|
||||
write(sv[1], wire_bytes, wire_len);
|
||||
close(sv[1]);
|
||||
|
||||
char *out = NULL;
|
||||
size_t out_size = 0;
|
||||
int ret = do_read_response(sv[0], &out, &out_size);
|
||||
close(sv[0]);
|
||||
free(out);
|
||||
|
||||
if (ret == expected_ret) {
|
||||
printf("PASS: %s\n", name);
|
||||
passes++;
|
||||
} else {
|
||||
printf("FAIL: %s — expected %d got %d\n", name, expected_ret, ret);
|
||||
failures++;
|
||||
}
|
||||
}
|
||||
|
||||
int main(void) {
|
||||
size_t hdr = offsetof(msg_t, value);
|
||||
|
||||
/* --- Case 1: msg->length > POVERLAY_BUFSIZE (heap over-read, must reject) --- */
|
||||
{
|
||||
char buf[hdr];
|
||||
memset(buf, 0, hdr);
|
||||
msg_t *m = (msg_t *)buf;
|
||||
m->type = 0;
|
||||
m->length = POVERLAY_BUFSIZE + 1; /* oversized */
|
||||
run_test("oversized msg->length (> POVERLAY_BUFSIZE)",
|
||||
buf, hdr, POVERLAY_READ_INVALID_RESPONSE);
|
||||
}
|
||||
|
||||
/* --- Case 2: msg->length > total_read (claims more data than arrived) --- */
|
||||
{
|
||||
char buf[hdr];
|
||||
memset(buf, 0, hdr);
|
||||
msg_t *m = (msg_t *)buf;
|
||||
m->type = 0;
|
||||
m->length = hdr + 100; /* claims 100 bytes of value, none sent */
|
||||
run_test("msg->length > total_read",
|
||||
buf, hdr, POVERLAY_READ_INVALID_RESPONSE);
|
||||
}
|
||||
|
||||
/* --- Case 3: msg->length < header_size (underflow guard) --- */
|
||||
{
|
||||
char buf[hdr];
|
||||
memset(buf, 0, hdr);
|
||||
msg_t *m = (msg_t *)buf;
|
||||
m->type = 0;
|
||||
m->length = hdr - 1;
|
||||
run_test("msg->length < header_size (underflow)",
|
||||
buf, hdr, POVERLAY_READ_INVALID_RESPONSE);
|
||||
}
|
||||
|
||||
/* --- Case 4: total_read < header_size (truncated message) --- */
|
||||
{
|
||||
/* Send only 2 bytes — not enough to form a header */
|
||||
char buf[2] = {0x01, 0x02};
|
||||
run_test("total_read < header_size (truncated)",
|
||||
buf, sizeof(buf), POVERLAY_READ_INVALID_RESPONSE);
|
||||
}
|
||||
|
||||
/* --- Case 5: valid message with a short value --- */
|
||||
{
|
||||
const char *val = "hello";
|
||||
size_t vlen = strlen(val);
|
||||
size_t total = hdr + vlen;
|
||||
char *buf = (char *)calloc(1, total);
|
||||
msg_t *m = (msg_t *)buf;
|
||||
m->type = 1;
|
||||
m->length = (uint64_t)total;
|
||||
memcpy(m->value, val, vlen);
|
||||
run_test("valid message", buf, total, 0);
|
||||
free(buf);
|
||||
}
|
||||
|
||||
/* --- Case 6: msg->length == POVERLAY_BUFSIZE exactly (boundary, accept) --- */
|
||||
{
|
||||
size_t vlen = POVERLAY_BUFSIZE - hdr;
|
||||
char *buf = (char *)calloc(1, POVERLAY_BUFSIZE);
|
||||
msg_t *m = (msg_t *)buf;
|
||||
m->type = 1;
|
||||
m->length = POVERLAY_BUFSIZE;
|
||||
memset(m->value, 'A', vlen);
|
||||
run_test("msg->length == POVERLAY_BUFSIZE (boundary accept)",
|
||||
buf, POVERLAY_BUFSIZE, 0);
|
||||
free(buf);
|
||||
}
|
||||
|
||||
printf("\n%d passed, %d failed\n", passes, failures);
|
||||
return failures ? 1 : 0;
|
||||
}
|
||||
Loading…
Reference in New Issue