From 61c847c09f9df21692c2f8fee0060d764e453412 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Sat, 10 Nov 2018 13:40:04 +0100 Subject: [PATCH 1/2] CVE-2018-1160: libatalk/dsi: avoid double use of variable i Signed-off-by: Ralph Boehme (backported from commit 67256322aa5a1fff01de471d6787d1d862678746) --- libatalk/dsi/dsi_opensess.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/libatalk/dsi/dsi_opensess.c b/libatalk/dsi/dsi_opensess.c index 2d7fd23b84..3a9e95ab6d 100644 --- a/libatalk/dsi/dsi_opensess.c +++ b/libatalk/dsi/dsi_opensess.c @@ -33,7 +33,9 @@ static void dsi_init_buffer(DSI *dsi) /* OpenSession. set up the connection */ void dsi_opensession(DSI *dsi) { - u_int32_t i = 0; /* this serves double duty. it must be 4-bytes long */ + size_t i = 0; + uint32_t servquant; + uint32_t replcsize; int offs; dsi_init_buffer(dsi); @@ -62,21 +64,21 @@ void dsi_opensession(DSI *dsi) dsi->header.dsi_code = 0; /* dsi->header.dsi_command = DSIFUNC_OPEN;*/ - dsi->cmdlen = 2 * (2 + sizeof(i)); /* length of data. dsi_send uses it. */ + dsi->cmdlen = 2 * (2 + sizeof(uint32_t)); /* length of data. dsi_send uses it. */ /* DSI Option Server Request Quantum */ dsi->commands[0] = DSIOPT_SERVQUANT; - dsi->commands[1] = sizeof(i); - i = htonl(( dsi->server_quantum < DSI_SERVQUANT_MIN || + dsi->commands[1] = sizeof(servquant); + servquant = htonl(( dsi->server_quantum < DSI_SERVQUANT_MIN || dsi->server_quantum > DSI_SERVQUANT_MAX ) ? DSI_SERVQUANT_DEF : dsi->server_quantum); - memcpy(dsi->commands + 2, &i, sizeof(i)); + memcpy(dsi->commands + 2, &servquant, sizeof(servquant)); /* AFP replaycache size option */ - offs = 2 + sizeof(i); + offs = 2 + sizeof(replcsize); dsi->commands[offs] = DSIOPT_REPLCSIZE; - dsi->commands[offs+1] = sizeof(i); - i = htonl(REPLAYCACHE_SIZE); - memcpy(dsi->commands + offs + 2, &i, sizeof(i)); + dsi->commands[offs+1] = sizeof(replcsize); + replcsize = htonl(REPLAYCACHE_SIZE); + memcpy(dsi->commands + offs + 2, &replcsize, sizeof(replcsize)); dsi_send(dsi); } -- 2.17.2 From 9b9afa26cc211d532ed408ad3ebbcac40ae56cf2 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Sat, 10 Nov 2018 13:41:43 +0100 Subject: [PATCH 2/2] CVE-2018-1160: libatalk/dsi: add correct bound checking to dsi_opensession The memcpy memcpy(&dsi->attn_quantum, dsi->commands + i + 1, dsi->commands[i]); trusted dsi->commands[i] to specify a size that fits into dsi->attn_quantum. The sizeof attn_quantum is four bytes. A malicious client can send a dsi->command[i] larger than 4 bytes to begin overwriting variables in the DSI struct. dsi->command[i] is a single char in a char array which limits the amount of data the attacker can overwrite in the DSI struct to 0xff. So for this to be useful in an attack there needs to be something within the 0xff bytes that follow attn_quantum. From dsi.h: uint32_t attn_quantum, datasize, server_quantum; uint16_t serverID, clientID; uint8_t *commands; /* DSI recieve buffer */ uint8_t data[DSI_DATASIZ]; /* DSI reply buffer */ The commands pointer is a heap allocated pointer that is reused for every packet received and sent. Using the memcpy, an attacker can overwrite this to point to an address of their choice and then all subsequent AFP packets will be written to that location. If the attacker chose the preauth_switch buffer, overwriting the function pointer there with functions pointers of his choice, he can invoke this functions over the network, Signed-off-by: Ralph Boehme (cherry picked from commit b6895be1cb5b915254ee92c2150e309cd31ebff6) --- libatalk/dsi/dsi_opensess.c | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/libatalk/dsi/dsi_opensess.c b/libatalk/dsi/dsi_opensess.c index 3a9e95ab6d..28282b7836 100644 --- a/libatalk/dsi/dsi_opensess.c +++ b/libatalk/dsi/dsi_opensess.c @@ -11,6 +11,7 @@ #include #include #include +#include #include #include @@ -37,6 +38,8 @@ void dsi_opensession(DSI *dsi) uint32_t servquant; uint32_t replcsize; int offs; + uint8_t cmd; + size_t option_len; dsi_init_buffer(dsi); if (setnonblock(dsi->socket, 1) < 0) { @@ -45,17 +48,32 @@ void dsi_opensession(DSI *dsi) } /* parse options */ - while (i < dsi->cmdlen) { - switch (dsi->commands[i++]) { + while (i + 1 < dsi->cmdlen) { + cmd = dsi->commands[i++]; + option_len = dsi->commands[i++]; + + if (i + option_len > dsi->cmdlen) { + LOG(log_error, logtype_dsi, "option %"PRIu8" too large: %zu", + cmd, option_len); + exit(EXITERR_CLNT); + } + + switch (cmd) { case DSIOPT_ATTNQUANT: - memcpy(&dsi->attn_quantum, dsi->commands + i + 1, dsi->commands[i]); + if (option_len != sizeof(dsi->attn_quantum)) { + LOG(log_error, logtype_dsi, "option %"PRIu8" bad length: %zu", + cmd, option_len); + exit(EXITERR_CLNT); + } + memcpy(&dsi->attn_quantum, &dsi->commands[i], option_len); dsi->attn_quantum = ntohl(dsi->attn_quantum); case DSIOPT_SERVQUANT: /* just ignore these */ default: - i += dsi->commands[i] + 1; /* forward past length tag + length */ break; } + + i += option_len; } /* let the client know the server quantum. we don't use the -- 2.17.2