From e20e5ff3af71d9a7a77efa36a6cb459f3c1cac3d Mon Sep 17 00:00:00 2001 From: hippoz <10706925-hippoz@users.noreply.gitlab.com> Date: Mon, 26 Dec 2022 23:10:22 +0200 Subject: [PATCH] fix not checking for string null byte --- server.c | 15 +++++++++++++-- wire.c | 8 ++++++++ 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/server.c b/server.c index bc82860..f887148 100644 --- a/server.c +++ b/server.c @@ -474,6 +474,16 @@ int jb_server_turn(struct jb_server *s) static const char auth_data[] = "DATA\r\n"; static const int data_buffer_len = 4096; + // We can keep a padding of null bytes for the data buffer. In the event that, for + // example, a string function is called on a char array without a proper ending null + // byte, we will reach the null bytes here at the end instead, thus preventing a + // crash and potential corruption. While this is a good "last line of defense" + // against such issues, it is much more important for these kinds of bugs to not + // exist in the first place. This mitigation will prevent, for example, ASAN from + // finding such bugs. It's recommended that you disable this padding outside + // of production so that you can find these bugs. + static const int data_buffer_padding = 0; + TRYST(poll(s->fds, s->fd_num, -1)); for (int i = 0; i < s->fd_num; i++) { @@ -511,9 +521,10 @@ int jb_server_turn(struct jb_server *s) continue; } - char data[data_buffer_len]; + // We add padding. See above. + char data[data_buffer_len + data_buffer_padding]; memset(data, 0, sizeof(data)); - ssize_t bytes = recv(fd, data, sizeof(data) - 1, 0); + ssize_t bytes = recv(fd, data, data_buffer_len, 0); if (bytes <= 0) { // error during recv() OR client disconnected, disconnect the client // TODO: should we actually do this? diff --git a/wire.c b/wire.c index aabfcea..28d7564 100644 --- a/wire.c +++ b/wire.c @@ -62,7 +62,15 @@ char *wire_get_string_impl(wire_context_t *c, bool as_signature) return NULL; } + // strings always end with a null byte + if (c->data[c->byte_cursor + len] != '\0') { + return NULL; + } + char *str = (char *)&c->data[c->byte_cursor]; + if (strlen(str) != len) { + return NULL; + } c->byte_cursor = new_cursor;