Fix possible integer overflow, thanks to @disaykin

The value of an arithmetic expression p[1] + (p[0] << 8) is subject to
overflow due to a failure to cast operands to a larger data type before
perfoming arithmetic

src/dynbuf.c

@@ -93,6 +93,11 @@ dynbuf_new_file(const char *filename)
 	}
 	fseek(f, 0, SEEK_END);
 	db->sz = ftell(f);
+	if (db->sz == (size_t)-1) {
+		free(db);
+		fclose(f);
+		return NULL;
+	}
 	fseek(f, 0, SEEK_SET);
 	if (!(db->buf = malloc(db->sz))) {
 		free(db);

src/log.c

@@ -303,7 +303,9 @@ log_masterkey_writecb(UNUSED int level, UNUSED void *fh, UNUSED unsigned long ct
 static void
 log_masterkey_fini(void)
 {
-	close(masterkey_fd);
+	if (masterkey_fd != -1)
+		close(masterkey_fd);
+	masterkey_fd = -1;
 }
 
 /*
@@ -367,7 +369,11 @@ log_connect_writecb(UNUSED int level, UNUSED void *fh, UNUSED unsigned long ctl,
 	size_t n;
 
 	time(&epoch);
-	utc = gmtime(&epoch);
+	if ((utc = gmtime(&epoch)) == NULL) {
+		log_err_level_printf(LOG_CRIT, "Failed to convert time: %s (%i)\n",
+		               strerror(errno), errno);
+		return -1;
+	}
 	n = strftime(timebuf, sizeof(timebuf), "%Y-%m-%d %H:%M:%S UTC ", utc);
 	if (n == 0) {
 		log_err_level_printf(LOG_CRIT, "Error from strftime(): buffer too small\n");
@@ -385,7 +391,9 @@ log_connect_writecb(UNUSED int level, UNUSED void *fh, UNUSED unsigned long ctl,
 static void
 log_connect_fini(void)
 {
-	close(connect_fd);
+	if (connect_fd != -1)
+		close(connect_fd);
+	connect_fd = -1;
 }
 
 static int
@@ -650,8 +658,11 @@ log_content_format_pathspec(const char *logspec,
 				struct tm *utc;
 
 				time(&epoch);
-				utc = gmtime(&epoch);
-				strftime(timebuf, sizeof(timebuf), iso8601, utc);
+				if ((utc = gmtime(&epoch)) == NULL) {
+					snprintf(timebuf, sizeof(timebuf), "gmtime error");
+				} else {
+					strftime(timebuf, sizeof(timebuf), iso8601, utc);
+				}
 
 				elem = timebuf;
 				elem_len = sizeof(timebuf);
@@ -760,7 +771,6 @@ log_content_open(log_content_ctx_t *ctx, global_t *global,
 		dsthost_clean = sys_ip46str_sanitize(dsthost);
 		if (!dsthost_clean) {
 			log_err_level_printf(LOG_CRIT, "Failed to sanitize dsthost\n");
-			free(srchost_clean);
 			goto errout;
 		}
 	}
@@ -1245,7 +1255,11 @@ log_content_file_single_prepcb(void *fh, unsigned long prepflags,
 	}
 	lb = head;
 	time(&epoch);
-	utc = gmtime(&epoch);
+	if ((utc = gmtime(&epoch)) == NULL) {
+		log_err_level_printf(LOG_CRIT, "Failed to convert time\n");
+		logbuf_free(lb);
+		return NULL;
+	}
 	lb->sz = strftime((char*)lb->buf, lb->sz, "%Y-%m-%d %H:%M:%S UTC ",
 	                  utc);
 
@@ -1551,10 +1565,8 @@ log_cert_submit(const char *fn, X509 *crt)
 	if (!(pem = ssl_x509_to_pem(crt)))
 		goto errout2;
 	if (!(lb = logbuf_new(0, pem, strlen(pem), NULL)))
-		goto errout3;
+		goto errout2;
 	return logger_submit(cert_log, fh, 0, lb);
-errout3:
-	free(pem);
 errout2:
 	free(fh);
 errout1:

src/logger.c

@@ -210,11 +210,14 @@ logger_thread(void *arg)
 		if (logbuf_ctl_isset(lb, LBFLAG_REOPEN)) {
 			if (logger->reopen() != 0)
 				e = 1;
+			logbuf_free(lb);
 		} else if (logbuf_ctl_isset(lb, LBFLAG_OPEN)) {
 			if (logger->open(lb->fh) != 0)
 				e = 1;
+			logbuf_free(lb);
 		} else if (logbuf_ctl_isset(lb, LBFLAG_CLOSE)) {
 			logger->close(lb->fh, lb->ctl);
+			logbuf_free(lb);
 		} else {
 			if (logbuf_write_free(lb, logger->write) < 0)
 				e = 1;
@@ -303,7 +306,6 @@ logger_printf(logger_t *logger, void *fh, unsigned long prepflags,
 
 	if (!(lb = logbuf_new(0, NULL, 0, NULL)))
 		return -1;
-	lb->fh = fh;
 	va_start(ap, fmt);
 	lb->sz = vasprintf((char**)&lb->buf, fmt, ap);
 	va_end(ap);
@@ -321,7 +323,6 @@ logger_write(logger_t *logger, void *fh, unsigned long prepflags,
 
 	if (!(lb = logbuf_new_copy(buf, sz, NULL)))
 		return -1;
-	lb->fh = fh;
 	return logger_submit(logger, fh, prepflags, lb);
 }
 int
@@ -332,7 +333,6 @@ logger_print(logger_t *logger, void *fh, unsigned long prepflags,
 
 	if (!(lb = logbuf_new_copy(s, strlen(s), NULL)))
 		return -1;
-	lb->fh = fh;
 	return logger_submit(logger, fh, prepflags, lb);
 }
 int
@@ -343,19 +343,13 @@ logger_write_freebuf(logger_t *logger, int level, void *fh, unsigned long prepfl
 
 	if (!(lb = logbuf_new(level, buf, sz, NULL)))
 		return -1;
-	lb->fh = fh;
 	return logger_submit(logger, fh, prepflags, lb);
 }
 int
 logger_print_freebuf(logger_t *logger, void *fh, unsigned long prepflags,
                      char *s)
 {
-	logbuf_t *lb;
-
-	if (!(lb = logbuf_new(0, s, strlen(s), NULL)))
-		return -1;
-	lb->fh = fh;
-	return logger_submit(logger, fh, prepflags, lb);
+	return logger_write_freebuf(logger, 0, fh, prepflags, s, strlen(s));
 }
 
 /* vim: set noet ft=c: */

src/logpkt.c

@@ -184,6 +184,22 @@ typedef struct __attribute__((packed)) {
 #define CSIN(X)         ((const struct sockaddr_in *)(X))
 #define CSIN6(X)        ((const struct sockaddr_in6 *)(X))
 
+static int
+logpkt_write_all(int fd, const void *data, size_t sz)
+{
+	const char *ptr = data;
+	while (sz > 0) {
+		ssize_t w = write(fd, ptr, sz);
+		if (w == -1 && errno == EINTR)
+			continue;
+		if (w == -1)
+			return -1;
+		ptr += w;
+		sz -= w;
+	}
+	return 0;
+}
+
 /*
  * Write the PCAP file-level header to file descriptor *fd* open for writing,
  * positioned at the beginning of an empty file.
@@ -201,7 +217,7 @@ logpkt_write_global_pcap_hdr(int fd)
 	hdr.version_minor = 4;
 	hdr.snaplen = MAX_PKTSZ;
 	hdr.network = 1;
-	return write(fd, &hdr, sizeof(hdr)) != sizeof(hdr) ? -1 : 0;
+	return logpkt_write_all(fd, &hdr, sizeof(hdr));
 }
 
 /*
@@ -289,12 +305,12 @@ logpkt_pcap_write(const uint8_t *pkt, size_t pktsz, int fd)
 	rec_hdr.ts_usec = tv.tv_usec;
 	rec_hdr.orig_len = rec_hdr.incl_len = pktsz;
 
-	if (write(fd, &rec_hdr, sizeof(rec_hdr)) != sizeof(rec_hdr)) {
+	if (logpkt_write_all(fd, &rec_hdr, sizeof(rec_hdr)) == -1) {
 		log_err_printf("Error writing pcap record hdr: %s\n",
 		               strerror(errno));
 		return -1;
 	}
-	if (write(fd, pkt, pktsz) != (ssize_t)pktsz) {
+	if (logpkt_write_all(fd, pkt, pktsz) == -1) {
 		log_err_printf("Error writing pcap record: %s\n",
 		               strerror(errno));
 		return -1;
@@ -386,7 +402,7 @@ logpkt_pcap_build(uint8_t *pkt,
 		                 sizeof(ip6_hdr->src_addr));
 		CHKSUM_ADD_RANGE(sum, ip6_hdr->dst_addr,
 		                 sizeof(ip6_hdr->dst_addr));
-		CHKSUM_ADD_UINT32(sum, ip6_hdr->len);
+		CHKSUM_ADD_UINT32(sum, (uint32_t)ip6_hdr->len);
 		CHKSUM_ADD_UINT16(sum, htons(IPPROTO_TCP));
 	}
 	tcp_hdr->seq = htonl(seq);
@@ -456,8 +472,7 @@ logpkt_mirror_build(libnet_t *libnet,
 	} else {
 		ptag = libnet_build_ipv6(0,             /* traffic class */
 		                         0,             /* flow label */
-		                         LIBNET_IPV6_H + LIBNET_TCP_H +
-		                         payloadlen,
+		                         LIBNET_TCP_H + payloadlen,
 		                         IPPROTO_TCP,
 		                         255,           /* hop limit */
 		                         *(struct libnet_in6_addr *)
@@ -544,7 +559,7 @@ logpkt_write_packet(logpkt_ctx_t *ctx, int fd, int direction, char flags,
 			                         payload, payloadlen);
 		} else {
 			rv = logpkt_mirror_build(ctx->libnet,
-			                         ctx->src_ether, ctx->dst_ether,
+			                         ctx->dst_ether, ctx->src_ether,
 			                         CSA(&ctx->dst_addr),
 			                         CSA(&ctx->src_addr),
 			                         flags,

src/opts.c

@@ -109,6 +109,7 @@ opts_load_cert_chain_key(const char *filename)
 		log_err_level_printf(LOG_CRIT, "Cert does not match key in PEM file "
 		                "'%s':\n", filename);
 		ERR_print_errors_fp(stderr);
+		cert_free(cert);
 		return NULL;
 	}
 

src/privsep.c

@@ -232,6 +232,7 @@ privsep_server_openfile(const char *fn, int mkpath)
 		log_err_level_printf(LOG_CRIT, "Failed to seek on '%s': %s (%i)\n",
 		               fn, strerror(errno), errno);
 		errno = tmp;
+		close(fd);
 		return -1;
 	}
 	return fd;

src/protohttp.c

@@ -152,7 +152,6 @@ protohttp_log_connect(pxy_conn_ctx_t *ctx)
 	}
 	if (ctx->global->connectlog) {
 		if (log_connect_print_free(msg) == -1) {
-			free(msg);
 			log_err_level_printf(LOG_WARNING, "Connection logging failed\n");
 		}
 	} else {

src/pxyconn.c

@@ -529,7 +529,6 @@ pxy_log_connect_nonhttp(pxy_conn_ctx_t *ctx)
 	}
 	if (ctx->global->connectlog) {
 		if (log_connect_print_free(msg) == -1) {
-			free(msg);
 			log_err_level_printf(LOG_WARNING, "Connection logging failed\n");
 		}
 	} else {

src/ssl.c

@@ -1979,6 +1979,16 @@ ssl_is_ocspreq(const unsigned char *buf, size_t sz)
 	return 1;
 }
 
+static uint16_t
+len2(uint8_t p0, uint8_t p1) {
+	return (uint16_t)p1 + ((uint16_t)p0 << 8);
+}
+
+static uint32_t
+len3(uint8_t p0, uint8_t p1, uint8_t p2) {
+	return (uint32_t)p2 + ((uint32_t)p1 << 8) + ((uint32_t)p0 << 16);
+}
+
 /*
  * Ugly hack to manually parse a clientHello message from a memory buffer.
  * This is needed in order to be able to support SNI and STARTTLS.
@@ -2037,13 +2047,13 @@ ssl_tls_clienthello_parse(const unsigned char *buf, ssize_t sz, int search,
 #endif /* !DEBUG_CLIENTHELLO_PARSER */
 	const unsigned char *p = buf;
 	ssize_t n = sz;
-	char *sn = NULL;
 
 	*clienthello = NULL;
 
 	DBG_printf("parsing buffer of sz %zd\n", sz);
 
 	do {
+		char *sn = NULL;
 		if (*clienthello) {
 			/*
 			 * Rewind after skipping an invalid ClientHello by
@@ -2052,10 +2062,6 @@ ssl_tls_clienthello_parse(const unsigned char *buf, ssize_t sz, int search,
 			 */
 			p = (*clienthello) + 1;
 			n = sz - (p - buf);
-			if (sn) {
-				free(sn);
-				sn = NULL;
-			}
 		}
 
 		if (search) {
@@ -2114,15 +2120,15 @@ ssl_tls_clienthello_parse(const unsigned char *buf, ssize_t sz, int search,
 			p += 2; n -= 2;
 
 			DBG_printf("cipher-spec-len: %02x %02x\n", p[0], p[1]);
-			ssize_t cipherspec_len = p[0] << 8 | p[1];
+			ssize_t cipherspec_len = len2(p[0], p[1]);
 			p += 2; n -= 2;
 
 			DBG_printf("session-id-len: %02x %02x\n", p[0], p[1]);
-			ssize_t sessionid_len = p[0] << 8 | p[1];
+			ssize_t sessionid_len = len2(p[0], p[1]);
 			p += 2; n -= 2;
 
 			DBG_printf("challenge-len: %02x %02x\n", p[0], p[1]);
-			ssize_t challenge_len = p[0] << 8 | p[1];
+			ssize_t challenge_len = len2(p[0], p[1]);
 			p += 2; n -= 2;
 			if (challenge_len < 16 || challenge_len > 32)
 				continue;
@@ -2165,7 +2171,7 @@ ssl_tls_clienthello_parse(const unsigned char *buf, ssize_t sz, int search,
 			return 1;
 		}
 		DBG_printf("length: %02x %02x\n", p[0], p[1]);
-		ssize_t recordlen = p[1] + (p[0] << 8);
+		ssize_t recordlen = len2(p[0], p[1]);
 		DBG_printf("recordlen=%zd\n", recordlen);
 		p += 2; n -= 2;
 		if (recordlen < 36) /* arbitrary size too small for a c-h */
@@ -2190,7 +2196,8 @@ ssl_tls_clienthello_parse(const unsigned char *buf, ssize_t sz, int search,
 		if (n < 3)
 			continue;
 		DBG_printf("message len: %02x %02x %02x\n", p[0], p[1], p[2]);
-		ssize_t msglen = p[2] + (p[1] << 8) + (p[0] << 16);
+
+		ssize_t msglen = len3(p[0], p[1], p[2]);
 		DBG_printf("msglen=%zd\n", msglen);
 		p += 3; n -= 3;
 		if (msglen < 32) /* arbitrary size too small for a c-h */
@@ -2232,7 +2239,8 @@ ssl_tls_clienthello_parse(const unsigned char *buf, ssize_t sz, int search,
 			continue;
 		DBG_printf("clienthello cipher suites length %02x %02x\n",
 		           p[0], p[1]);
-		ssize_t suiteslen = p[1] + (p[0] << 8);
+
+		ssize_t suiteslen = len2(p[0], p[1]);
 		p += 2; n -= 2;
 		if (n < suiteslen)
 			continue;
@@ -2261,7 +2269,7 @@ ssl_tls_clienthello_parse(const unsigned char *buf, ssize_t sz, int search,
 		if (n < 2)
 			continue;
 		DBG_printf("tlsexts length %02x %02x\n", p[0], p[1]);
-		ssize_t tlsextslen = p[1] + (p[0] << 8);
+		ssize_t tlsextslen = len2(p[0], p[1]);
 		DBG_printf("tlsextslen %zd\n", tlsextslen);
 		p += 2; n -= 2;
 		if (n < tlsextslen)
@@ -2273,8 +2281,8 @@ ssl_tls_clienthello_parse(const unsigned char *buf, ssize_t sz, int search,
 				goto continue_search;
 			DBG_printf("tlsext type %02x %02x len %02x %02x\n",
 			           p[0], p[1], p[2], p[3]);
-			unsigned short exttype = p[1] + (p[0] << 8);
-			ssize_t extlen = p[3] + (p[2] << 8);
+			unsigned short exttype = len2(p[0], p[1]);
+			ssize_t extlen = len2(p[2], p[3]);
 			p += 4; n -= 4;
 			if (n < extlen)
 				goto continue_search;
@@ -2287,7 +2295,7 @@ ssl_tls_clienthello_parse(const unsigned char *buf, ssize_t sz, int search,
 					goto continue_search;
 				DBG_printf("list length %02x %02x\n",
 				           extp[0], extp[1]);
-				ssize_t namelistlen = extp[1] + (extp[0] << 8);
+				ssize_t namelistlen = len2(extp[0], extp[1]);
 				DBG_printf("namelistlen = %zd\n", namelistlen);
 				extp += 2;
 				extn -= 2;
@@ -2302,7 +2310,7 @@ ssl_tls_clienthello_parse(const unsigned char *buf, ssize_t sz, int search,
 					           " len %02x %02x\n",
 					           extp[0], extp[1], extp[2]);
 					unsigned char sntype = extp[0];
-					ssize_t snlen = extp[2] + (extp[1]<<8);
+					ssize_t snlen = len2(extp[1], extp[2]);
 					extp += 3;
 					extn -= 3;
 					if (snlen > extn)
@@ -2353,16 +2361,13 @@ done_parsing:
 			*servername = sn;
 		return 0;
 continue_search:
-		;
+		if (sn)
+			free(sn);
 	} while (search && n > 0);
 
 	/* No valid ClientHello messages found, not even a truncated one */
 	DBG_printf("===> No match: rv 1, *clienthello NULL\n");
 	*clienthello = NULL;
-	if (sn) {
-		free(sn);
-		sn = NULL;
-	}
 	return 1;
 }
 

tests/testproxy/lp/pxyconn.c

@@ -149,7 +149,6 @@ pxy_log_connect_tcp(pxy_conn_ctx_t *ctx)
 	}
 	if (ctx->opts->connectlog) {
 		if (log_connect_print_free(msg) == -1) {
-			free(msg);
 			log_err_level_printf(LOG_WARNING, "Connection logging failed\n");
 		}
 	} else {