From af0442f15cc966bbc3d7d9322380005ea0ee8340 Mon Sep 17 00:00:00 2001 From: Ralph Little Date: Sun, 26 Apr 2020 13:04:41 -0700 Subject: [PATCH 1/9] magicolor: Added security remediation for pixels_per_line. This implements a security issue reported by GitHub Security Lab. The details are disclosed in GitLab issue #279. The issue relates to an invalid scan parameter block being sent to the backend containing 8 bytes of 0x00 which leads to pixels_per_line being set to 0. Later arithmetic involves the division by this value which causes a div by zero crash. --- backend/magicolor.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/backend/magicolor.c b/backend/magicolor.c index af9fb1af7..5de5923ee 100644 --- a/backend/magicolor.c +++ b/backend/magicolor.c @@ -836,6 +836,26 @@ cmd_get_scanning_parameters(SANE_Handle handle, *data_pixels, *data_pixels, *lines, *lines, *pixels_per_line, *pixels_per_line); + + /* + * SECURITY REMEDIATION + * See gitlab issue #279 - Issue10 SIGFPE in mc_setup_block_mode + * + * pixels_per_line cannot be zero, otherwise a division by zero error can occur later. + * Added checking the parameters to ensure that this issue cannot occur. + * + * Additionally to the reported issue, it makes no sense for any of the values of + * data_pixels, lines or pixels_per_line to be zero. I do not have any knowledge + * of valid maximums for these parameters. + * + */ + if ((*data_pixels == 0) || (*lines == 0) || (*pixels_per_line == 0)) { + DBG (1, "%s: ERROR: Returned image parameters contain invalid " + "bytes. Zero values for these parameters are not rational.\n", + __func__); + dump_hex_buffer_dense (1, rxbuf, 8); + return SANE_STATUS_INVAL; + } } return status; From db9480b09ea807e52029f2334769a55d4b95e45b Mon Sep 17 00:00:00 2001 From: Olaf Meeuwissen Date: Mon, 27 Apr 2020 18:24:56 +0900 Subject: [PATCH 2/9] epsonds: Read only up to seven hexdigits to determine payload size Addresses GHSL-2020-083, re #279. --- backend/epsonds-cmd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/backend/epsonds-cmd.c b/backend/epsonds-cmd.c index 9a4db3080..23327bb18 100644 --- a/backend/epsonds-cmd.c +++ b/backend/epsonds-cmd.c @@ -117,7 +117,7 @@ esci2_check_header(const char *cmd, const char *buf, unsigned int *more) return 0; } - err = sscanf(&buf[5], "%x#", more); + err = sscanf(&buf[5], "%7x#", more); if (err != 1) { DBG(1, "cannot decode length from header\n"); return 0; From b9b0173409df73e235da2aa0dae5edd21fb55967 Mon Sep 17 00:00:00 2001 From: Olaf Meeuwissen Date: Mon, 27 Apr 2020 18:48:29 +0900 Subject: [PATCH 3/9] epsonds: Prevent possible buffer overflow when reading image data Addresses GHSL-2020-084, re #279. --- backend/epsonds-cmd.c | 5 +++++ backend/epsonds.c | 12 +++++++----- backend/epsonds.h | 1 + 3 files changed, 13 insertions(+), 5 deletions(-) diff --git a/backend/epsonds-cmd.c b/backend/epsonds-cmd.c index 9a4db3080..c182aa51a 100644 --- a/backend/epsonds-cmd.c +++ b/backend/epsonds-cmd.c @@ -876,6 +876,11 @@ esci2_img(struct epsonds_scanner *s, SANE_Int *length) return parse_status; } + /* more data than was accounted for in s->buf */ + if (more > s->bsz) { + return SANE_STATUS_IO_ERROR; + } + /* ALWAYS read image data */ if (s->hw->connection == SANE_EPSONDS_NET) { epsonds_net_request_read(s, more); diff --git a/backend/epsonds.c b/backend/epsonds.c index ff5d68106..fb9694a88 100644 --- a/backend/epsonds.c +++ b/backend/epsonds.c @@ -1230,16 +1230,18 @@ sane_start(SANE_Handle handle) if (s->line_buffer == NULL) return SANE_STATUS_NO_MEM; - /* ring buffer for front page, twice bsz */ + /* transfer buffer size, bsz */ /* XXX read value from scanner */ - status = eds_ring_init(&s->front, (65536 * 4) * 2); + s->bsz = (65536 * 4); + + /* ring buffer for front page */ + status = eds_ring_init(&s->front, s->bsz * 2); if (status != SANE_STATUS_GOOD) { return status; } - /* transfer buffer, bsz */ - /* XXX read value from scanner */ - s->buf = realloc(s->buf, 65536 * 4); + /* transfer buffer */ + s->buf = realloc(s->buf, s->bsz); if (s->buf == NULL) return SANE_STATUS_NO_MEM; diff --git a/backend/epsonds.h b/backend/epsonds.h index 0427ef3b4..401b0f32c 100644 --- a/backend/epsonds.h +++ b/backend/epsonds.h @@ -160,6 +160,7 @@ struct epsonds_scanner Option_Value val[NUM_OPTIONS]; SANE_Parameters params; + size_t bsz; /* transfer buffer size */ SANE_Byte *buf, *line_buffer; ring_buffer *current, front, back; From 27ea994d23ee52fe1ec1249c92ebc1080a358288 Mon Sep 17 00:00:00 2001 From: Olaf Meeuwissen Date: Thu, 30 Apr 2020 21:15:45 +0900 Subject: [PATCH 4/9] epsonds: Do not read beyond the end of the token Addresses GHSL-2020-082, re #279. --- backend/epsonds-cmd.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/backend/epsonds-cmd.c b/backend/epsonds-cmd.c index 9a4db3080..7ca660f1f 100644 --- a/backend/epsonds-cmd.c +++ b/backend/epsonds-cmd.c @@ -255,18 +255,20 @@ static int decode_value(char *buf, int len) } /* h000 */ -static char *decode_binary(char *buf) +static char *decode_binary(char *buf, int len) { char tmp[6]; int hl; memcpy(tmp, buf, 4); tmp[4] = '\0'; + len -= 4; if (buf[0] != 'h') return NULL; hl = strtol(tmp + 1, NULL, 16); + if (hl > len) hl = len; if (hl) { char *v = malloc(hl + 1); @@ -279,9 +281,9 @@ static char *decode_binary(char *buf) return NULL; } -static char *decode_string(char *buf) +static char *decode_string(char *buf, int len) { - char *p, *s = decode_binary(buf); + char *p, *s = decode_binary(buf, len); if (s == NULL) return NULL; @@ -326,20 +328,20 @@ static SANE_Status info_cb(void *userdata, char *token, int len) if (strncmp("PRD", token, 3) == 0) { free(s->hw->model); - s->hw->model = decode_string(value); + s->hw->model = decode_string(value, len); s->hw->sane.model = s->hw->model; DBG(1, " product: %s\n", s->hw->model); /* we will free the string later */ } if (strncmp("VER", token, 3) == 0) { - char *v = decode_string(value); + char *v = decode_string(value, len); DBG(1, " version: %s\n", v); free(v); } if (strncmp("S/N", token, 3) == 0) { - char *v = decode_string(value); + char *v = decode_string(value, len); DBG(1, " serial: %s\n", v); free(v); } From fff83e7eacd0f27bb2d71c42488e0fd735c15ac3 Mon Sep 17 00:00:00 2001 From: Olaf Meeuwissen Date: Thu, 30 Apr 2020 18:24:51 +0900 Subject: [PATCH 5/9] epson2: Rewrite network I/O This addresses GHSL-2020-075 as well as all other problematic code uncovered as a result of investigating that. This includes: - buffer overflows due to use of unchecked lengths - integer overflows due to type conversions - potential memory leaks - checking for memory allocation failures Re #279. --- backend/epson2_net.c | 156 +++++++++++++++++++++++++------------------ backend/epson2_net.h | 4 +- 2 files changed, 93 insertions(+), 67 deletions(-) diff --git a/backend/epson2_net.c b/backend/epson2_net.c index 8d0fe9ea7..7f804eea8 100644 --- a/backend/epson2_net.c +++ b/backend/epson2_net.c @@ -32,11 +32,12 @@ #include "sane/sanei_debug.h" -static int +static ssize_t sanei_epson_net_read_raw(Epson_Scanner *s, unsigned char *buf, ssize_t wanted, SANE_Status *status) { - int ready, read = -1; + int ready; + ssize_t read = -1; fd_set readable; struct timeval tv; @@ -62,111 +63,136 @@ sanei_epson_net_read_raw(Epson_Scanner *s, unsigned char *buf, ssize_t wanted, return read; } -int +static ssize_t +sanei_epson_net_read_buf(Epson_Scanner *s, unsigned char *buf, ssize_t wanted, + SANE_Status * status) +{ + ssize_t read = 0; + + DBG(23, "%s: reading up to %lu from buffer at %p, %lu available\n", + __func__, (u_long) wanted, s->netptr, (u_long) s->netlen); + + if ((size_t) wanted > s->netlen) { + *status = SANE_STATUS_IO_ERROR; + wanted = s->netlen; + } + + memcpy(buf, s->netptr, wanted); + read = wanted; + + s->netptr += read; + s->netlen -= read; + + if (s->netlen == 0) { + DBG(23, "%s: freeing %p\n", __func__, s->netbuf); + free(s->netbuf); + s->netbuf = s->netptr = NULL; + s->netlen = 0; + } + + return read; +} + +ssize_t sanei_epson_net_read(Epson_Scanner *s, unsigned char *buf, ssize_t wanted, SANE_Status * status) { - ssize_t size; - ssize_t read = 0; - unsigned char header[12]; - - /* read from buffer, if available */ - if (s->netptr != s->netbuf) { - DBG(23, "reading %lu from buffer at %p, %lu available\n", - (u_long) wanted, s->netptr, (u_long) s->netlen); - - memcpy(buf, s->netptr, wanted); - read = wanted; - - s->netlen -= wanted; - - if (s->netlen == 0) { - DBG(23, "%s: freeing %p\n", __func__, s->netbuf); - free(s->netbuf); - s->netbuf = s->netptr = NULL; - s->netlen = 0; - } - - return read; - } - - /* receive net header */ - size = sanei_epson_net_read_raw(s, header, 12, status); - if (size != 12) { + if (wanted < 0) { + *status = SANE_STATUS_INVAL; return 0; } + size_t size; + ssize_t read = 0; + unsigned char header[12]; + + /* read from remainder of buffer */ + if (s->netptr) { + return sanei_epson_net_read_buf(s, buf, wanted, status); + } + + /* receive net header */ + read = sanei_epson_net_read_raw(s, header, 12, status); + if (read != 12) { + return 0; + } + + /* validate header */ if (header[0] != 'I' || header[1] != 'S') { DBG(1, "header mismatch: %02X %02x\n", header[0], header[1]); *status = SANE_STATUS_IO_ERROR; return 0; } + /* parse payload size */ size = be32atoh(&header[6]); - DBG(23, "%s: wanted = %lu, available = %lu\n", __func__, - (u_long) wanted, (u_long) size); - *status = SANE_STATUS_GOOD; - if (size == wanted) { + if (!s->netbuf) { + DBG(15, "%s: direct read\n", __func__); + DBG(23, "%s: wanted = %lu, available = %lu\n", __func__, + (u_long) wanted, (u_long) size); - DBG(15, "%s: full read\n", __func__); - - read = sanei_epson_net_read_raw(s, buf, size, status); - - if (s->netbuf) { - free(s->netbuf); - s->netbuf = NULL; - s->netlen = 0; + if ((size_t) wanted > size) { + wanted = size; } - if (read < 0) { - return 0; - } - -/* } else if (wanted < size && s->netlen == size) { */ + read = sanei_epson_net_read_raw(s, buf, wanted, status); } else { - DBG(23, "%s: partial read\n", __func__); + DBG(15, "%s: buffered read\n", __func__); + DBG(23, "%s: bufferable = %lu, available = %lu\n", __func__, + (u_long) s->netlen, (u_long) size); - read = sanei_epson_net_read_raw(s, s->netbuf, size, status); - if (read != size) { - return 0; + if (s->netlen > size) { + s->netlen = size; } - s->netlen = size - wanted; - s->netptr += wanted; - read = wanted; + /* fill buffer */ + read = sanei_epson_net_read_raw(s, s->netbuf, s->netlen, status); + s->netptr = s->netbuf; + s->netlen = (read > 0 ? read : 0); - DBG(23, "0,4 %02x %02x\n", s->netbuf[0], s->netbuf[4]); - DBG(23, "storing %lu to buffer at %p, next read at %p, %lu bytes left\n", - (u_long) size, s->netbuf, s->netptr, (u_long) s->netlen); - - memcpy(buf, s->netbuf, wanted); + /* copy wanted part */ + read = sanei_epson_net_read_buf(s, buf, wanted, status); } return read; } - -int +size_t sanei_epson_net_write(Epson_Scanner *s, unsigned int cmd, const unsigned char *buf, size_t buf_size, size_t reply_len, SANE_Status *status) { unsigned char *h1, *h2, *payload; unsigned char *packet = malloc(12 + 8 + buf_size); - /* XXX check allocation failure */ + if (!packet) { + *status = SANE_STATUS_NO_MEM; + return 0; + } h1 = packet; h2 = packet + 12; payload = packet + 12 + 8; if (reply_len) { - s->netbuf = s->netptr = malloc(reply_len); + if (s->netbuf) { + DBG(23, "%s, freeing %p, %ld bytes unprocessed\n", + __func__, s->netbuf, (u_long) s->netlen); + free(s->netbuf); + s->netbuf = s->netptr = NULL; + s->netlen = 0; + } + s->netbuf = malloc(reply_len); + if (!s->netbuf) { + free(packet); + *status = SANE_STATUS_NO_MEM; + return 0; + } s->netlen = reply_len; - DBG(24, "allocated %lu bytes at %p\n", - (u_long) reply_len, s->netbuf); + DBG(24, "%s: allocated %lu bytes at %p\n", __func__, + (u_long) s->netlen, s->netbuf); } DBG(24, "%s: cmd = %04x, buf = %p, buf_size = %lu, reply_len = %lu\n", diff --git a/backend/epson2_net.h b/backend/epson2_net.h index 6aef2b725..7db671bf1 100644 --- a/backend/epson2_net.h +++ b/backend/epson2_net.h @@ -4,9 +4,9 @@ #include #include "../include/sane/sane.h" -extern int sanei_epson_net_read(struct Epson_Scanner *s, unsigned char *buf, ssize_t buf_size, +extern ssize_t sanei_epson_net_read(struct Epson_Scanner *s, unsigned char *buf, ssize_t buf_size, SANE_Status *status); -extern int sanei_epson_net_write(struct Epson_Scanner *s, unsigned int cmd, const unsigned char *buf, +extern size_t sanei_epson_net_write(struct Epson_Scanner *s, unsigned int cmd, const unsigned char *buf, size_t buf_size, size_t reply_len, SANE_Status *status); extern SANE_Status sanei_epson_net_lock(struct Epson_Scanner *s); From 07e3834127f8bcd9dac02b91c17127dc41fbfb5b Mon Sep 17 00:00:00 2001 From: Ralph Little Date: Thu, 30 Apr 2020 23:21:00 -0700 Subject: [PATCH 6/9] magicolor: Added security mediation to device discovery Extraction of values from the SNMP response were not checked. Also fixed a bug that mistakenly matched any SNMP OIDs with the first model in the model list, in function mc_get_device_from_identification(). --- backend/magicolor.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/backend/magicolor.c b/backend/magicolor.c index af9fb1af7..f25661cfb 100644 --- a/backend/magicolor.c +++ b/backend/magicolor.c @@ -114,7 +114,7 @@ static struct MagicolorCap magicolor_cap[] = { /* KONICA MINOLTA magicolor 1690MF, USB ID 0x123b:2089 */ { - 0x2089, "mc1690mf", "KONICA MINOLTA magicolor 1690MF", "1.3.6.1.4.1.183341.1.1.2.1.32.3.2", + 0x2089, "mc1690mf", "KONICA MINOLTA magicolor 1690MF", ".1.3.6.1.4.1.18334.1.1.1.1.1.23.1.1", -1, 0x85, 600, {150, 600, 0}, magicolor_default_resolutions, 3, /* 600 dpi max, 3 resolutions */ 8, magicolor_default_depths, /* color depth 8 default, 1 and 8 possible */ @@ -1525,8 +1525,10 @@ mc_get_device_from_identification (const char*ident) { int n; for (n = 0; n < NELEMS (magicolor_cap); n++) { - if (strcmp (magicolor_cap[n].model, ident) || strcmp (magicolor_cap[n].OID, ident)) + if ((strcmp (magicolor_cap[n].model, ident) == 0) || (strcmp (magicolor_cap[n].OID, ident) == 0)) + { return &magicolor_cap[n]; + } } return NULL; } @@ -1833,9 +1835,9 @@ mc_network_discovery_handle (struct snmp_pdu *pdu, snmp_discovery_data *magic) oid anOID[MAX_OID_LEN]; size_t anOID_len = MAX_OID_LEN; /* Device information variables */ - char ip_addr[1024]; - char model[1024]; - char device[1024]; + char ip_addr[1024] = ""; + char model[1024] = ""; + char device[1024] = ""; /* remote IP detection variables */ netsnmp_indexed_addr_pair *responder = (netsnmp_indexed_addr_pair *) pdu->transport_data; struct sockaddr_in *remote = NULL; @@ -1884,6 +1886,13 @@ mc_network_discovery_handle (struct snmp_pdu *pdu, snmp_discovery_data *magic) DBG (3, "%s: SystemObjectID does not return an OID, device is not a magicolor device\n", __func__); return 0; } + + // Make sure that snprint_objid gives us just numbers, instead of a + // SNMPv2-SMI::enterprises prefix. + netsnmp_ds_set_int(NETSNMP_DS_LIBRARY_ID, + NETSNMP_DS_LIB_OID_OUTPUT_FORMAT, + NETSNMP_OID_OUTPUT_NUMERIC); + snprint_objid (device, sizeof(device), vp->val.objid, value_len); DBG (5, "%s: Device object ID is '%s'\n", __func__, device); @@ -1903,8 +1912,9 @@ mc_network_discovery_handle (struct snmp_pdu *pdu, snmp_discovery_data *magic) read_objid(MAGICOLOR_SNMP_SYSDESCR_OID, anOID, &anOID_len); vp = find_varbind_in_list (varlist, anOID, anOID_len); if (vp) { - memcpy(model,vp->val.string,vp->val_len); - model[vp->val_len] = '\0'; + size_t model_len = min(vp->val_len, sizeof(model) - 1); + memcpy(model, vp->val.string, model_len); + model[model_len] = '\0'; DBG (5, "%s: Found model: %s\n", __func__, model); } From fe08bbee6b238ea0be73af67b560ffc2c47562fd Mon Sep 17 00:00:00 2001 From: Olaf Meeuwissen Date: Mon, 4 May 2020 11:48:46 +0900 Subject: [PATCH 7/9] epsonds: Handle error condition. Re #279, issue 8 --- backend/epsonds-cmd.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/backend/epsonds-cmd.c b/backend/epsonds-cmd.c index 9a4db3080..3187a0f51 100644 --- a/backend/epsonds-cmd.c +++ b/backend/epsonds-cmd.c @@ -193,6 +193,8 @@ static SANE_Status esci2_cmd(epsonds_scanner* s, ssize_t read = eds_recv(s, pbuf, more, &status); if (read != more) { + free(pbuf); + return SANE_STATUS_IO_ERROR; } /* parse the received data block */ From 8682023faa27c61156a354955c89617a3304d66f Mon Sep 17 00:00:00 2001 From: Olaf Meeuwissen Date: Mon, 4 May 2020 11:54:35 +0900 Subject: [PATCH 8/9] sanei_tcp: Address possible integer overflow. Re #279, issue 8 --- include/sane/sanei_tcp.h | 4 ++-- sanei/sanei_tcp.c | 17 ++++++++++++++--- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/include/sane/sanei_tcp.h b/include/sane/sanei_tcp.h index 227e61315..a375c3949 100644 --- a/include/sane/sanei_tcp.h +++ b/include/sane/sanei_tcp.h @@ -31,7 +31,7 @@ extern SANE_Status sanei_tcp_open(const char *host, int port, int *fdp); extern void sanei_tcp_close(int fd); -extern ssize_t sanei_tcp_write(int fd, const u_char * buf, int count); -extern ssize_t sanei_tcp_read(int fd, u_char * buf, int count); +extern ssize_t sanei_tcp_write(int fd, const u_char * buf, size_t count); +extern ssize_t sanei_tcp_read(int fd, u_char * buf, size_t count); #endif /* sanei_tcp_h */ diff --git a/sanei/sanei_tcp.c b/sanei/sanei_tcp.c index 87a73d109..d6f8efe34 100644 --- a/sanei/sanei_tcp.c +++ b/sanei/sanei_tcp.c @@ -45,6 +45,11 @@ #include #include #include +#include + +#ifndef SSIZE_MAX +#define SSIZE_MAX LONG_MAX +#endif #ifdef HAVE_WINSOCK2_H #include @@ -115,15 +120,21 @@ sanei_tcp_close(int fd) } ssize_t -sanei_tcp_write(int fd, const u_char * buf, int count) +sanei_tcp_write(int fd, const u_char * buf, size_t count) { return send(fd, buf, count, 0); } ssize_t -sanei_tcp_read(int fd, u_char * buf, int count) +sanei_tcp_read(int fd, u_char * buf, size_t count) { - ssize_t bytes_recv = 0, rc = 1; + size_t bytes_recv = 0; + ssize_t rc = 1; + + if (count > SSIZE_MAX) { + errno = EINVAL; + return -1; + } while (bytes_recv < count && rc > 0) { From 30b1831a28f24ab2921b9f717c66d37f02bb81cc Mon Sep 17 00:00:00 2001 From: Olaf Meeuwissen Date: Mon, 11 May 2020 21:07:12 +0900 Subject: [PATCH 9/9] epsonds: Mitigate potential network related security issues. Re #279 This pre-empts the possibility of triggering GHSL-2020-079, GHSL-2020-080 and GHSL-2020-081. --- backend/epsonds.conf.in | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/backend/epsonds.conf.in b/backend/epsonds.conf.in index b8b36237a..1967a00fd 100644 --- a/backend/epsonds.conf.in +++ b/backend/epsonds.conf.in @@ -10,7 +10,7 @@ usb # e.g.: # usb 0x4b8 0x14c -# Network +# Network (not yet supported!) # # net 192.168.1.123 -net autodiscovery +#net autodiscovery