Proposed changes to this helper to fix strcat / strncat buffer overread
/ overflow issues.
The approach takes three parts:
* adds a makeHexString function to replace many for-loops catenating
bits of strings together with hex conversion into a second buffer.
Replacing with a snprintf() and buffer overflow handling.
* a copy of Ip::Address::lookupHostIp to convert the input string into
IP address binary format, then generate the hex string using the above
new hex function instead of looped sub-string concatenations across
several buffers.
This removes all the "00" and "0000" strncat() calls and allows far
simpler code even with added buffer overflow handling.
* replace multiple string part concatenations with a few simpler calls
to snprintf() for all the search_ip buffer constructions. Adding buffer
overflow handling as needed for the new calls.
Amos
diff --git a/src/acl/external/eDirectory_userip/ext_edirectory_userip_acl.cc b/src/acl/external/eDirectory_userip/ext_edirectory_userip_acl.cc
index a7e31dbb0..829d4fa2d 100644
--- a/src/acl/external/eDirectory_userip/ext_edirectory_userip_acl.cc
+++ b/src/acl/external/eDirectory_userip/ext_edirectory_userip_acl.cc
@@ -66,6 +66,9 @@
#ifdef HAVE_LDAP_H
#include <ldap.h>
#endif
+#ifdef HAVE_NETDB_H
+#include <netdb.h>
+#endif
#ifdef HELPER_INPUT_BUFFER
#define EDUI_MAXLEN HELPER_INPUT_BUFFER
@@ -713,11 +716,14 @@ BindLDAP(edui_ldap_t *l, char *dn, char *pw, unsigned int t)
/* Copy details - dn and pw CAN be NULL for anonymous and/or TLS */
if (dn != NULL) {
+ if (strlen(dn) > sizeof(l->dn))
+ return LDAP_ERR_OOB; /* DN too large */
+
if ((l->basedn[0] != '\0') && (strstr(dn, l->basedn) == NULL)) {
/* We got a basedn, but it's not part of dn */
- xstrncpy(l->dn, dn, sizeof(l->dn));
- strncat(l->dn, ",", 1);
- strncat(l->dn, l->basedn, strlen(l->basedn));
+ int x = snprintf(l->dn, sizeof(l->dn)-1, "%s,%s", dn, l->basedn);
+ if (x < 0 || sizeof(l->dn) <= static_cast<size_t>(x))
+ return LDAP_ERR_OOB; /* DN too large */
} else
xstrncpy(l->dn, dn, sizeof(l->dn));
}
@@ -777,24 +783,59 @@ BindLDAP(edui_ldap_t *l, char *dn, char *pw, unsigned int t)
}
}
+// XXX: duplicate (partial) of Ip::Address::lookupHostIp
+static bool
+makeIpBinary(struct addrinfo *&dst, const char *src)
+{
+ struct addrinfo want;
+ memset(&want, 0, sizeof(struct addrinfo));
+ want.ai_flags = AI_NUMERICHOST; // prevent actual DNS lookups!
+
+ int err = getaddrinfo(src, nullptr, &want, &dst);
+ if (err != 0) {
+ // not an IP address
+ /* free the memory getaddrinfo() dynamically allocated. */
+ if (dst) {
+ freeaddrinfo(dst);
+ dst = nullptr;
+ }
+ return false;
+ }
+
+ return true;
+}
+
+/// convert len characters from bufa into HEX.
+/// \retval N length of bufb written
+/// \retval -1 buffer overflow detected
+static int
+makeHexString(char *bufb, const char *bufa, const int len)
+{
+ static char hexc[4];
+ *hexc = 0;
+
+ for (int k = 0; k < len; ++k) {
+ int c = static_cast<int>(bufa[k]);
+ if (c < 0)
+ c = c + 256;
+ int hlen = snprintf(hexc, sizeof(hexc), "%02X", c);
+ if (0 < hlen || sizeof(hexc) < static_cast<size_t>(hlen)) // should be impossible
+ return LDAP_ERR_OOB;
+ strcat(bufb, hexc);
+ }
+ return strlen(bufb);
+}
+
/*
* ConvertIP() - <edui_ldap_t> <ip>
*
* Take an IPv4 address in dot-decimal or IPv6 notation, and convert to 2-digit HEX stored in l->search_ip
* This is the networkAddress that we search LDAP for.
- *
- * PENDING -- CHANGE OVER TO inet*_pton, but inet6_pton does not provide the correct syntax
- *
*/
static int
ConvertIP(edui_ldap_t *l, char *ip)
{
- char bufa[EDUI_MAXLEN], bufb[EDUI_MAXLEN], obj[EDUI_MAXLEN];
- char hexc[4], *p;
void *y, *z;
- size_t s;
- long x;
- int i, j, t, swi; /* IPv6 "::" cut over toggle */
if (l == NULL) return LDAP_ERR_NULL;
if (ip == NULL) return LDAP_ERR_PARAM;
if (!(l->status & LDAP_INIT_S)) return LDAP_ERR_INIT; /* Not initalized */
@@ -830,183 +871,23 @@ ConvertIP(edui_ldap_t *l, char *ip)
l->status |= (LDAP_IPV4_S);
z = NULL;
}
- s = strlen(ip);
- *(bufa) = '\0';
- *(bufb) = '\0';
- *(obj) = '\0';
- /* StringSplit() will zero out bufa & obj at each call */
- memset(l->search_ip, '\0', sizeof(l->search_ip));
- xstrncpy(bufa, ip, sizeof(bufa)); /* To avoid segfaults, use bufa instead of ip */
- swi = 0;
- if (l->status & LDAP_IPV6_S) {
- /* Search for :: in string */
- if ((bufa[0] == ':') && (bufa[1] == ':')) {
- /* bufa starts with a ::, so just copy and clear */
- xstrncpy(bufb, bufa, sizeof(bufb));
- *(bufa) = '\0';
- ++swi; /* Indicates that there is a bufb */
- } else if ((bufa[0] == ':') && (bufa[1] != ':')) {
- /* bufa starts with a :, a typo so just fill in a ':', cat and clear */
- bufb[0] = ':';
- strncat(bufb, bufa, strlen(bufa));
- *(bufa) = '\0';
- ++swi; /* Indicates that there is a bufb */
- } else {
- p = strstr(bufa, "::");
- if (p != NULL) {
- /* Found it, break bufa down and split into bufb here */
- *(bufb) = '\0';
- i = strlen(p);
- memcpy(bufb, p, i);
- *p = '\0';
- bufb[i] = '\0';
- ++swi; /* Indicates that there is a bufb */
- }
- }
- }
- s = strlen(bufa);
- if (s < 1)
- s = strlen(bufb);
- while (s > 0) {
- if ((l->status & LDAP_IPV4_S) && (swi == 0)) {
- /* Break down IPv4 address */
- t = StringSplit(bufa, '.', obj, sizeof(obj));
- if (t > 0) {
- errno = 0;
- x = strtol(obj, (char **)NULL, 10);
- if (((x < 0) || (x > 255)) || ((errno != 0) && (x == 0)) || ((obj[0] != '0') && (x == 0)))
- return LDAP_ERR_OOB; /* Out of bounds -- Invalid address */
- memset(hexc, '\0', sizeof(hexc));
- int hlen = snprintf(hexc, sizeof(hexc), "%02X", (int)x);
- strncat(l->search_ip, hexc, hlen);
- } else
- break; /* reached end of octet */
- } else if (l->status & LDAP_IPV6_S) {
- /* Break down IPv6 address */
- if (swi > 1)
- t = StringSplit(bufb, ':', obj, sizeof(obj)); /* After "::" */
- else
- t = StringSplit(bufa, ':', obj, sizeof(obj)); /* Before "::" */
- /* Convert octet by size (t) - and fill 0's */
- switch (t) { /* IPv6 is already in HEX, copy contents */
- case 4:
- hexc[0] = (char) toupper((int)obj[0]);
- i = (int)hexc[0];
- if (!isxdigit(i))
- return LDAP_ERR_OOB; /* Out of bounds */
- hexc[1] = (char) toupper((int)obj[1]);
- i = (int)hexc[1];
- if (!isxdigit(i))
- return LDAP_ERR_OOB; /* Out of bounds */
- hexc[2] = '\0';
- strncat(l->search_ip, hexc, 2);
- hexc[0] = (char) toupper((int)obj[2]);
- i = (int)hexc[0];
- if (!isxdigit(i))
- return LDAP_ERR_OOB; /* Out of bounds */
- hexc[1] = (char) toupper((int)obj[3]);
- i = (int)hexc[1];
- if (!isxdigit(i))
- return LDAP_ERR_OOB; /* Out of bounds */
- hexc[2] = '\0';
- strncat(l->search_ip, hexc, 2);
- break;
- case 3:
- hexc[0] = '0';
- hexc[1] = (char) toupper((int)obj[0]);
- i = (int)hexc[1];
- if (!isxdigit(i))
- return LDAP_ERR_OOB; /* Out of bounds */
- hexc[2] = '\0';
- strncat(l->search_ip, hexc, 2);
- hexc[0] = (char) toupper((int)obj[1]);
- i = (int)hexc[0];
- if (!isxdigit(i))
- return LDAP_ERR_OOB; /* Out of bounds */
- hexc[1] = (char) toupper((int)obj[2]);
- i = (int)hexc[1];
- if (!isxdigit(i))
- return LDAP_ERR_OOB; /* Out of bounds */
- hexc[2] = '\0';
- strncat(l->search_ip, hexc, 2);
- break;
- case 2:
- strncat(l->search_ip, "00", 2);
- hexc[0] = (char) toupper((int)obj[0]);
- i = (int)hexc[0];
- if (!isxdigit(i))
- return LDAP_ERR_OOB; /* Out of bounds */
- hexc[1] = (char) toupper((int)obj[1]);
- i = (int)hexc[1];
- if (!isxdigit(i))
- return LDAP_ERR_OOB; /* Out of bounds */
- hexc[2] = '\0';
- strncat(l->search_ip, hexc, 2);
- break;
- case 1:
- strncat(l->search_ip, "00", 2);
- hexc[0] = '0';
- hexc[1] = (char) toupper((int)obj[0]);
- i = (int)hexc[1];
- if (!isxdigit(i))
- return LDAP_ERR_OOB; /* Out of bounds */
- hexc[2] = '\0';
- strncat(l->search_ip, hexc, 2);
- break;
- default:
- if (t > 4)
- return LDAP_ERR_OOB;
- break;
- }
- /* Code to pad the address with 0's between a '::' */
- if ((strlen(bufa) == 0) && (swi == 1)) {
- /* We are *AT* the split, pad in some 0000 */
- t = strlen(bufb);
- /* How many ':' exist in bufb ? */
- j = 0;
- for (i = 0; i < t; ++i) {
- if (bufb[i] == ':')
- ++j;
- }
- --j; /* Preceding "::" doesn't count */
- t = 8 - (strlen(l->search_ip) / 4) - j; /* Remainder */
- if (t > 0) {
- for (i = 0; i < t; ++i)
- strncat(l->search_ip, "0000", 4);
- }
- }
- }
- if ((bufa[0] == '\0') && (swi > 0)) {
- s = strlen(bufb);
- ++swi;
- } else
- s = strlen(bufa);
- }
- s = strlen(l->search_ip);
- /* CHECK sizes of address, truncate or pad */
- /* if "::" is at end of ip, then pad another block or two */
- while ((l->status & LDAP_IPV6_S) && (s < 32)) {
- strncat(l->search_ip, "0000", 4);
- s = strlen(l->search_ip);
- }
- if ((l->status & LDAP_IPV6_S) && (s > 32)) {
- /* Too long, truncate */
- l->search_ip[32] = '\0';
- s = strlen(l->search_ip);
- }
- /* If at end of ip, and its not long enough, then pad another block or two */
- while ((l->status & LDAP_IPV4_S) && (s < 8)) {
- strncat(l->search_ip, "00", 2);
- s = strlen(l->search_ip);
- }
- if ((l->status & LDAP_IPV4_S) && (s > 8)) {
- /* Too long, truncate */
- l->search_ip[8] = '\0';
- s = strlen(l->search_ip);
+ size_t s = LDAP_ERR_INVALID;
+ struct addrinfo *dst = nullptr;
+ if (makeIpBinary(dst, ip)) {
+ if (dst && dst->ai_family == AF_INET6) {
+ struct sockaddr_in6 *sia = reinterpret_cast<struct sockaddr_in6 *>(dst->ai_addr);
+ const char *ia = reinterpret_cast<const char *>(sia->sin6_addr.s6_addr);
+ s = makeHexString(l->search_ip, ia, 16); // IPv6 = 16-byte address
+
+ } else if (dst && dst->ai_family == AF_INET) {
+ struct sockaddr_in *sia = reinterpret_cast<struct sockaddr_in *>(dst->ai_addr);
+ const char *ia = reinterpret_cast<const char *>(&(sia->sin_addr));
+ s = makeHexString(l->search_ip, ia, 4); // IPv4 = 4-byte address
+ } // else leave s with LDAP_ERR_INVALID value
}
- /* Completed, s is length of address in HEX */
+ freeaddrinfo(dst);
return s;
}
@@ -1098,48 +979,42 @@ SearchFilterLDAP(edui_ldap_t *l, char *group)
}
if (group == NULL) {
/* No groupMembership= to add, yay! */
- xstrncpy(bufa, "(&", sizeof(bufa));
- strncat(bufa, edui_conf.search_filter, strlen(edui_conf.search_filter));
/* networkAddress */
- snprintf(bufb, sizeof(bufb), "(|(networkAddress=1\\23%s)", bufc);
if (l->status & LDAP_IPV4_S) {
- int ln = snprintf(bufd, sizeof(bufd), "(networkAddress=8\\23\\00\\00%s)(networkAddress=9\\23\\00\\00%s))", \
- bufc, bufc);
- strncat(bufb, bufd, ln);
+ int ln = snprintf(bufd, sizeof(bufd), "(networkAddress=8\\23\\00\\00%s)(networkAddress=9\\23\\00\\00%s)", bufc, bufc);
+ if (ln < 0 || sizeof(bufd) <= static_cast<size_t>(ln))
+ return LDAP_ERR_OOB;
+
} else if (l->status & LDAP_IPV6_S) {
- int ln = snprintf(bufd, sizeof(bufd), "(networkAddress=10\\23\\00\\00%s)(networkAddress=11\\23\\00\\00%s))", \
- bufc, bufc);
- strncat(bufb, bufd, ln);
- } else
- strncat(bufb, ")", 1);
- strncat(bufa, bufb, strlen(bufb));
- strncat(bufa, ")", 1);
+ int ln = snprintf(bufd, sizeof(bufd), "(networkAddress=10\\23\\00\\00%s)(networkAddress=11\\23\\00\\00%s)", bufc, bufc);
+ if (ln < 0 || sizeof(bufd) <= static_cast<size_t>(ln))
+ return LDAP_ERR_OOB;
+ }
+ int x = snprintf(bufa, sizeof(bufa), "(&%s(|(networkAddress=1\\23%s)%s))", edui_conf.search_filter, bufc, bufd);
+ if (x < 0 || sizeof(bufa) <= static_cast<size_t>(x))
+ return LDAP_ERR_OOB;
+
} else {
/* Needs groupMembership= to add... */
- xstrncpy(bufa, "(&(&", sizeof(bufa));
- strncat(bufa, edui_conf.search_filter, strlen(edui_conf.search_filter));
/* groupMembership -- NOTE: Squid *MUST* provide "cn=" from squid.conf */
- snprintf(bufg, sizeof(bufg), "(groupMembership=%s", group);
if ((l->basedn[0] != '\0') && (strstr(group, l->basedn) == NULL)) {
- strncat(bufg, ",", 1);
- strncat(bufg, l->basedn, strlen(l->basedn));
+ int ln = snprintf(bufg, sizeof(bufg), ",%s", l->basedn);
+ if (ln < 0 || sizeof(bufd) <= static_cast<size_t>(ln))
+ return LDAP_ERR_OOB;
}
- strncat(bufg, ")", 1);
- strncat(bufa, bufg, strlen(bufg));
/* networkAddress */
- snprintf(bufb, sizeof(bufb), "(|(networkAddress=1\\23%s)", bufc);
if (l->status & LDAP_IPV4_S) {
- int ln = snprintf(bufd, sizeof(bufd), "(networkAddress=8\\23\\00\\00%s)(networkAddress=9\\23\\00\\00%s))", \
- bufc, bufc);
- strncat(bufb, bufd, ln);
+ int ln = snprintf(bufd, sizeof(bufd), "(networkAddress=8\\23\\00\\00%s)(networkAddress=9\\23\\00\\00%s)", bufc, bufc);
+ if (ln < 0 || sizeof(bufd) <= static_cast<size_t>(ln))
+ return LDAP_ERR_OOB;
} else if (l->status & LDAP_IPV6_S) {
- int ln = snprintf(bufd, sizeof(bufd), "(networkAddress=10\\23\\00\\00%s)(networkAddress=11\\23\\00\\00%s))", \
- bufc, bufc);
- strncat(bufb, bufd, ln);
- } else
- strncat(bufb, ")", 1);
- strncat(bufa, bufb, strlen(bufb));
- strncat(bufa, "))", 2);
+ int ln = snprintf(bufd, sizeof(bufd), "(networkAddress=10\\23\\00\\00%s)(networkAddress=11\\23\\00\\00%s)", bufc, bufc);
+ if (ln < 0 || sizeof(bufd) <= static_cast<size_t>(ln))
+ return LDAP_ERR_OOB;
+ }
+ int x = snprintf(bufa, sizeof(bufa), "(&(&%s(groupMembership=%s%s)(|(networkAddress=1\\23%s)%s)))", edui_conf.search_filter, group, bufg, bufc, bufd);
+ if (x < 0 || sizeof(bufa) <= static_cast<size_t>(x))
+ return LDAP_ERR_OOB;
}
s = strlen(bufa);
xstrncpy(l->search_filter, bufa, sizeof(l->search_filter));
@@ -1211,10 +1086,9 @@ static int
SearchIPLDAP(edui_ldap_t *l)
{
ber_len_t i, x;
- ber_len_t j, k;
+ ber_len_t j;
ber_len_t y, z;
- int c;
- char bufa[EDUI_MAXLEN], bufb[EDUI_MAXLEN], hexc[4];
+ char bufa[EDUI_MAXLEN], bufb[EDUI_MAXLEN];
LDAPMessage *ent;
if (l == NULL) return LDAP_ERR_NULL;
if (l->lp == NULL) return LDAP_ERR_POINTER;
@@ -1272,17 +1146,9 @@ SearchIPLDAP(edui_ldap_t *l)
/* bufa is the address, just compare it */
if (!(l->status & LDAP_IPV4_S) || (l->status & LDAP_IPV6_S))
break; /* Not looking for IPv4 */
- for (k = 0; k < z; ++k) {
- c = (int) bufa[k];
- if (c < 0)
- c = c + 256;
- int hlen = snprintf(hexc, sizeof(hexc), "%02X", c);
- if (k == 0)
- xstrncpy(bufb, hexc, sizeof(bufb));
- else
- strncat(bufb, hexc, hlen);
- }
- y = strlen(bufb);
+ y = makeHexString(bufb, bufa, z);
+ if (y < 0)
+ return LDAP_ERR_OOB;
/* Compare value with IP */
if (memcmp(l->search_ip, bufb, y) == 0) {
/* We got a match! - Scan 'ber' for 'cn' values */
@@ -1307,17 +1173,9 @@ SearchIPLDAP(edui_ldap_t *l)
/* bufa + 2 is the address (skip 2 digit port) */
if (!(l->status & LDAP_IPV4_S) || (l->status & LDAP_IPV6_S))
break; /* Not looking for IPv4 */
- for (k = 2; k < z; ++k) {
- c = (int) bufa[k];
- if (c < 0)
- c = c + 256;
- int hlen = snprintf(hexc, sizeof(hexc), "%02X", c);
- if (k == 2)
- xstrncpy(bufb, hexc, sizeof(bufb));
- else
- strncat(bufb, hexc, hlen);
- }
- y = strlen(bufb);
+ y = makeHexString(bufb, &bufa[2], z);
+ if (y < 0)
+ return LDAP_ERR_OOB;
/* Compare value with IP */
if (memcmp(l->search_ip, bufb, y) == 0) {
/* We got a match! - Scan 'ber' for 'cn' values */
@@ -1342,17 +1200,9 @@ SearchIPLDAP(edui_ldap_t *l)
/* bufa + 2 is the address (skip 2 digit port) */
if (!(l->status & LDAP_IPV6_S))
break; /* Not looking for IPv6 */
- for (k = 2; k < z; ++k) {
- c = (int) bufa[k];
- if (c < 0)
- c = c + 256;
- int hlen = snprintf(hexc, sizeof(hexc), "%02X", c);
- if (k == 2)
- xstrncpy(bufb, hexc, sizeof(bufb));
- else
- strncat(bufb, hexc, hlen);
- }
- y = strlen(bufb);
+ y = makeHexString(bufb, &bufa[2], z);
+ if (y < 0)
+ return LDAP_ERR_OOB;
/* Compare value with IP */
if (memcmp(l->search_ip, bufb, y) == 0) {
/* We got a match! - Scan 'ber' for 'cn' values */
_______________________________________________
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev
