From 2fa337d8533172a2803d29f1a3b727309380cb51 Mon Sep 17 00:00:00 2001 From: Cedric Roux Date: Fri, 25 Apr 2025 11:49:31 +0200 Subject: [PATCH 1/2] T bugfix: include size of 'length' to allocated memory The way allocation is done (granularity of 64KiB) makes a crash more difficult to achieve, but if a trace is exactly 64KiB, we miss 4 bytes in the realloc and then anything may happen. --- common/utils/T/tracer/extract_config.c | 4 ++-- common/utils/T/tracer/hacks/multi-rru-clean.c | 4 ++-- common/utils/T/tracer/multi.c | 4 ++-- common/utils/T/tracer/record.c | 4 ++-- common/utils/T/tracer/replay.c | 4 ++-- 5 files changed, 10 insertions(+), 10 deletions(-) diff --git a/common/utils/T/tracer/extract_config.c b/common/utils/T/tracer/extract_config.c index b908e60a83..73640cd3c1 100644 --- a/common/utils/T/tracer/extract_config.c +++ b/common/utils/T/tracer/extract_config.c @@ -46,8 +46,8 @@ int main(int n, char **v) /* read event from file */ if (fread(&length, 4, 1, in) != 1) break; - if (ebuf.omaxsize < length) { - ebuf.omaxsize = (length + 65535) & ~65535; + if (ebuf.omaxsize < length + 4) { + ebuf.omaxsize = (length + 4 + 65535) & ~65535; ebuf.obuf = realloc(ebuf.obuf, ebuf.omaxsize); if (ebuf.obuf == NULL) { printf("out of memory\n"); exit(1); } } diff --git a/common/utils/T/tracer/hacks/multi-rru-clean.c b/common/utils/T/tracer/hacks/multi-rru-clean.c index ec7c8f9d1b..1887c9bb2b 100644 --- a/common/utils/T/tracer/hacks/multi-rru-clean.c +++ b/common/utils/T/tracer/hacks/multi-rru-clean.c @@ -157,8 +157,8 @@ int main(int n, char **v) /* read event from file */ if (fread(&length, 4, 1, in) != 1) break; - if (ebuf.omaxsize < length) { - ebuf.omaxsize = (length + 65535) & ~65535; + if (ebuf.omaxsize < length + 4) { + ebuf.omaxsize = (length + 4 + 65535) & ~65535; ebuf.obuf = realloc(ebuf.obuf, ebuf.omaxsize); if (ebuf.obuf == NULL) { printf("out of memory\n"); exit(1); } } diff --git a/common/utils/T/tracer/multi.c b/common/utils/T/tracer/multi.c index 0b2e48ec7a..f10c520250 100644 --- a/common/utils/T/tracer/multi.c +++ b/common/utils/T/tracer/multi.c @@ -114,8 +114,8 @@ int read_tracee(int s, OBUF *ebuf, int *_type, int32_t *_length) int vpos = 0; if (fullread(s, &length, 4) == -1) return -1; - if (ebuf->omaxsize < length) { - ebuf->omaxsize = (length + 65535) & ~65535; + if (ebuf->omaxsize < length + 4) { + ebuf->omaxsize = (length + 4 + 65535) & ~65535; ebuf->obuf = realloc(ebuf->obuf, ebuf->omaxsize); if (ebuf->obuf == NULL) { printf("out of memory\n"); exit(1); } } diff --git a/common/utils/T/tracer/record.c b/common/utils/T/tracer/record.c index 9e3c4c41a3..d9f9f0837e 100644 --- a/common/utils/T/tracer/record.c +++ b/common/utils/T/tracer/record.c @@ -133,8 +133,8 @@ int main(int n, char **v) int vpos = 0; if (fullread(socket, &length, 4) == -1) goto read_error; - if (ebuf.omaxsize < length) { - ebuf.omaxsize = (length + 65535) & ~65535; + if (ebuf.omaxsize < length + 4) { + ebuf.omaxsize = (length + 4 + 65535) & ~65535; ebuf.obuf = realloc(ebuf.obuf, ebuf.omaxsize); if (ebuf.obuf == NULL) { printf("out of memory\n"); exit(1); } } diff --git a/common/utils/T/tracer/replay.c b/common/utils/T/tracer/replay.c index d9a505520a..10aec1588e 100644 --- a/common/utils/T/tracer/replay.c +++ b/common/utils/T/tracer/replay.c @@ -146,8 +146,8 @@ int main(int n, char **v) /* read event from file */ if (fread(&length, 4, 1, in) != 1) break; - if (ebuf.omaxsize < length) { - ebuf.omaxsize = (length + 65535) & ~65535; + if (ebuf.omaxsize < length + 4) { + ebuf.omaxsize = (length + 4 + 65535) & ~65535; ebuf.obuf = realloc(ebuf.obuf, ebuf.omaxsize); if (ebuf.obuf == NULL) { printf("out of memory\n"); exit(1); } } From 7a77ee2f432e367f919e9090492715bbb16d461f Mon Sep 17 00:00:00 2001 From: Cedric Roux Date: Fri, 25 Apr 2025 12:08:09 +0200 Subject: [PATCH 2/2] T: validate input data a bit more When data is generated by "normal" T tracers, it should be fine. But it might come from unknown sources and then content might be "strange". So let's validate input data a bit more. --- common/utils/T/tracer/event.c | 21 ++++++++++++++++++++- common/utils/T/tracer/multi.c | 2 ++ common/utils/T/tracer/record.c | 2 ++ 3 files changed, 24 insertions(+), 1 deletion(-) diff --git a/common/utils/T/tracer/event.c b/common/utils/T/tracer/event.c index 61fb12dd2a..b7459ddcb8 100644 --- a/common/utils/T/tracer/event.c +++ b/common/utils/T/tracer/event.c @@ -24,9 +24,11 @@ event get_event(int socket, OBUF *event_buffer, void *database) again: if (fullread(socket, &length, 4) == -1) goto read_error; #ifdef T_SEND_TIME + if (length < sizeof(struct timespec)) goto error; if (fullread(socket, &t, sizeof(struct timespec)) == -1) goto read_error; length -= sizeof(struct timespec); #endif + if (length < sizeof(int)) goto error; if (fullread(socket, &type, sizeof(int)) == -1) goto read_error; length -= sizeof(int); if (event_buffer->omaxsize < length) { @@ -48,6 +50,9 @@ again: return new_event(type, length, event_buffer->obuf, database); #endif +error: + printf("error: bad data in get_event()\n"); + read_error: return (event){.type = -1}; } @@ -64,6 +69,9 @@ event new_event(int type, int length, char *buffer, void *database) int i; int offset; + /* arbitrary limit to 1GiB (to simplify size checks) */ + if (length < 0 || length > 1024 * 1024 * 1024) goto fatal; + #ifdef T_SEND_TIME e.sending_time = sending_time; #endif @@ -87,26 +95,33 @@ event new_event(int type, int length, char *buffer, void *database) for (i = 0; i < f.count; i++) { //e.e[i].offset = offset; if (!strcmp(f.type[i], "int")) { + if (offset + 4 > length) goto fatal; e.e[i].type = EVENT_INT; e.e[i].i = *(int *)(&buffer[offset]); offset += 4; } else if (!strcmp(f.type[i], "ulong")) { + if (offset + sizeof(unsigned long) > length) goto fatal; e.e[i].type = EVENT_ULONG; e.e[i].ul = *(unsigned long *)(&buffer[offset]); offset += sizeof(unsigned long); } else if (!strcmp(f.type[i], "float")) { + if (offset + sizeof(float) > length) goto fatal; e.e[i].type = EVENT_FLOAT; e.e[i].f = *(float *)(&buffer[offset]); offset += sizeof(float); } else if (!strcmp(f.type[i], "string")) { + if (offset + 1 > length) goto fatal; e.e[i].type = EVENT_STRING; e.e[i].s = &buffer[offset]; - while (buffer[offset]) offset++; + while (offset < length && buffer[offset]) offset++; + if (offset == length) goto fatal; offset++; } else if (!strcmp(f.type[i], "buffer")) { + if (offset + sizeof(int) > length) goto fatal; int len; e.e[i].type = EVENT_BUFFER; len = *(int *)(&buffer[offset]); + if (len <= 0 || offset + len + sizeof(int) > length) goto fatal; e.e[i].bsize = len; e.e[i].b = &buffer[offset+sizeof(int)]; offset += len+sizeof(int); @@ -119,4 +134,8 @@ event new_event(int type, int length, char *buffer, void *database) if (e.ecount==0) { printf("FORMAT not set in event %d\n", type); abort(); } return e; + +fatal: + printf("fatal: bad buffer in new_event\n"); + abort(); } diff --git a/common/utils/T/tracer/multi.c b/common/utils/T/tracer/multi.c index f10c520250..8e49ff43a1 100644 --- a/common/utils/T/tracer/multi.c +++ b/common/utils/T/tracer/multi.c @@ -123,10 +123,12 @@ int read_tracee(int s, OBUF *ebuf, int *_type, int32_t *_length) memcpy(v+vpos, &length, 4); vpos += 4; #ifdef T_SEND_TIME + if (length < sizeof(struct timespec)) return -1; if (fullread(s,v+vpos,sizeof(struct timespec))==-1) return -1; vpos += sizeof(struct timespec); length -= sizeof(struct timespec); #endif + if (length < sizeof(int)) return -1; if (fullread(s, &type, sizeof(int)) == -1) return -1; memcpy(v+vpos, &type, sizeof(int)); vpos += sizeof(int); diff --git a/common/utils/T/tracer/record.c b/common/utils/T/tracer/record.c index d9f9f0837e..f9d9dece64 100644 --- a/common/utils/T/tracer/record.c +++ b/common/utils/T/tracer/record.c @@ -142,10 +142,12 @@ int main(int n, char **v) memcpy(v+vpos, &length, 4); vpos += 4; #ifdef T_SEND_TIME + if (length < sizeof(struct timespec)) goto read_error; if (fullread(socket,v+vpos,sizeof(struct timespec))==-1) goto read_error; vpos += sizeof(struct timespec); length -= sizeof(struct timespec); #endif + if (length < sizeof(int)) goto read_error; if (fullread(socket, &type, sizeof(int)) == -1) goto read_error; memcpy(v+vpos, &type, sizeof(int)); vpos += sizeof(int);