In several cases, snprintf result was used without validating its range.
When formatted string would overflow buffer or error happens, snprintf
will return either value larger than buffer size, or -1. In both cases,
if you add this value to pointer (or similar), bad things will happen.
Pattern to watch for: =.*snprintf
I have not verified if any of this is exploitable. In some cases, I was
not sure about proper error handling (watch for XXX comments).
While fixing this error, I noticed typo in Ip::Qos::Config::dumpConfigLine:
markMissMask was used instead of tosMissMask.
Patches compile-tested (however, only on linux/x86/gcc49 and in default
configuration).
Index: squid-3.5.13/src/ip/QosConfig.cc
===================================================================
--- squid-3.5.13.orig/src/ip/QosConfig.cc
+++ squid-3.5.13/src/ip/QosConfig.cc
@@ -392,7 +392,7 @@ Ip::Qos::Config::dumpConfigLine(char *en
if (tosMiss > 0) {
p += snprintf(p, 11, " miss=0x%02X", tosMiss);
if (tosMissMask!=0xFFU) {
- p += snprintf(p, 6, "/0x%02X", markMissMask);
+ p += snprintf(p, 6, "/0x%02X", tosMissMask);
}
}
if (preserveMissTos == 0) {
Index: squid-3.5.13/helpers/digest_auth/file/text_backend.cc
===================================================================
--- squid-3.5.13.orig/helpers/digest_auth/file/text_backend.cc
+++ squid-3.5.13/helpers/digest_auth/file/text_backend.cc
@@ -171,7 +171,7 @@ GetPassword(RequestData * requestData)
if (!hash)
return NULL;
len = snprintf(buf, sizeof(buf), "%s:%s", requestData->user, requestData->realm);
- if (len >= static_cast<int>(sizeof(buf)))
+ if (len < 0 || len >= static_cast<int>(sizeof(buf)))
return NULL;
u = (user_data*)hash_lookup(hash, buf);
if (u)
Index: squid-3.5.13/helpers/external_acl/eDirectory_userip/ext_edirectory_userip_acl.cc
===================================================================
--- squid-3.5.13.orig/helpers/external_acl/eDirectory_userip/ext_edirectory_userip_acl.cc
+++ squid-3.5.13/helpers/external_acl/eDirectory_userip/ext_edirectory_userip_acl.cc
@@ -227,7 +227,7 @@ local_printfx(const char *msg,...)
va_start(ap, msg);
x = vsnprintf(dbuf, sz, msg, ap);
va_end(ap);
- if (x > 0) {
+ if (x > 0 || x >= sz) {
dbuf[x] = '\0';
++x;
fputs(dbuf, stdout);
@@ -879,6 +879,7 @@ ConvertIP(edui_ldap_t *l, char *ip)
return LDAP_ERR_OOB; /* Out of bounds -- Invalid address */
memset(hexc, '\0', sizeof(hexc));
int hlen = snprintf(hexc, sizeof(hexc), "%02X", (int)x);
+ if (hlen < 0 || hlen >= static_cast<int>(sizeof(hexc))) hlen = 0; /* impossible */
strncat(l->search_ip, hexc, hlen);
} else
break; /* reached end of octet */
@@ -1106,10 +1107,12 @@ SearchFilterLDAP(edui_ldap_t *l, char *g
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);
+ if (ln < 0 || ln >= static_cast<int>(sizeof(bufd))) return LDAP_ERR_FAILED;
strncat(bufb, bufd, ln);
} 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);
+ if (ln < 0 || ln >= static_cast<int>(sizeof(bufd))) return LDAP_ERR_FAILED;
strncat(bufb, bufd, ln);
} else
strncat(bufb, ")", 1);
@@ -1132,10 +1135,12 @@ SearchFilterLDAP(edui_ldap_t *l, char *g
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);
+ if (ln < 0 || ln >= static_cast<int>(sizeof(bufd))) return LDAP_ERR_FAILED;
strncat(bufb, bufd, ln);
} 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);
+ if (ln < 0 || ln >= static_cast<int>(sizeof(bufd))) return LDAP_ERR_FAILED;
strncat(bufb, bufd, ln);
} else
strncat(bufb, ")", 1);
@@ -1278,6 +1283,7 @@ SearchIPLDAP(edui_ldap_t *l)
if (c < 0)
c = c + 256;
int hlen = snprintf(hexc, sizeof(hexc), "%02X", c);
+ if (hlen < 0 || hlen >= static_cast<int>(sizeof(hexc))) return LDAP_ERR_FAILED;
if (k == 0)
xstrncpy(bufb, hexc, sizeof(bufb));
else
@@ -1313,6 +1319,7 @@ SearchIPLDAP(edui_ldap_t *l)
if (c < 0)
c = c + 256;
int hlen = snprintf(hexc, sizeof(hexc), "%02X", c);
+ if (hlen < 0 || hlen >= static_cast<int>(sizeof(hexc))) return LDAP_ERR_FAILED;
if (k == 2)
xstrncpy(bufb, hexc, sizeof(bufb));
else
@@ -1348,6 +1355,7 @@ SearchIPLDAP(edui_ldap_t *l)
if (c < 0)
c = c + 256;
int hlen = snprintf(hexc, sizeof(hexc), "%02X", c);
+ if (hlen < 0 || hlen >= static_cast<int>(sizeof(hexc))) return LDAP_ERR_FAILED;
if (k == 2)
xstrncpy(bufb, hexc, sizeof(bufb));
else
Index: squid-3.5.13/src/log/File.cc
===================================================================
--- squid-3.5.13.orig/src/log/File.cc
+++ squid-3.5.13/src/log/File.cc
@@ -104,13 +104,16 @@ logfilePrintf(Logfile * lf, const char *
va_start(args, fmt);
- s = vsnprintf(buf, 8192, fmt, args);
+ s = vsnprintf(buf, sizeof(buf), fmt, args);
- if (s > 8192) {
- s = 8192;
+ if (s < 0) {
+ xstrncpy(buf, "snprintf error in logfilePrintf\n", sizeof(buf));
+ s = strlen(buf);
+ } else if (s >= static_cast<int>(sizeof(buf))) {
+ s = static_cast<int>(sizeof(buf));
if (fmt[strlen(fmt) - 1] == '\n')
- buf[8191] = '\n';
+ buf[sizeof(buf) - 1] = '\n';
}
logfileWrite(lf, buf, (size_t) s);
Index: squid-3.5.13/tools/cachemgr.cc
===================================================================
--- squid-3.5.13.orig/tools/cachemgr.cc
+++ squid-3.5.13/tools/cachemgr.cc
@@ -505,6 +505,7 @@ munge_other_line(const char *buf, cachem
/* start html table */
if (!table_line_num) {
l += snprintf(html + l, sizeof(html) - l, "</pre><table cellpadding=\"2\" cellspacing=\"1\">\n");
+ if (l < 0 || l >= static_cast<int>(sizeof(html))) l = 0;
next_is_header = 0;
}
@@ -515,6 +516,7 @@ munge_other_line(const char *buf, cachem
/* record starts */
l += snprintf(html + l, sizeof(html) - l, "<tr>");
+ if (l < 0 || l >= static_cast<int>(sizeof(html))) l = 0;
/* substitute '\t' */
buf_copy = x = xstrdup(buf);
@@ -535,6 +537,7 @@ munge_other_line(const char *buf, cachem
ttag, column_span,
is_header ? "center" : is_number(cell) ? "right" : "left",
html_quote(cell), ttag);
+ if (l < 0 || l >= static_cast<int>(sizeof(html))) l = 0;
}
xfree(buf_copy);
@@ -850,6 +853,10 @@ process_request(cachemgr_request * req)
req->workers? req->workers : (req->processes ? req->processes: ""),
VERSION,
make_auth_header(req));
+ if (l < 0 || l >= static_cast<int>(sizeof(buf))) {
+ error_html("snprintf error");
+ return 1;
+ }
if (write(s, buf, l) < 0) {
fprintf(stderr,"ERROR: (%d) writing request: '%s'\n", errno, buf);
} else {
@@ -1075,6 +1082,10 @@ make_pub_auth(cachemgr_request * req)
(int) now,
req->user_name ? req->user_name : "",
req->passwd);
+ if (bufLen < 0 || bufLen >= static_cast<int>(sizeof(buf))) {
+ debug("cmgr: snprintf error: 0 <= %d <= %u\n", bufLen, (unsigned)sizeof(buf));
+ return;
+ }
debug("cmgr: pre-encoded for pub: %s\n", buf);
const int encodedLen = base64_encode_len(bufLen);
@@ -1176,6 +1187,11 @@ make_auth_header(const cachemgr_request
req->user_name ? req->user_name : "",
req->passwd);
+ if (bufLen < 0 || bufLen >= static_cast<int>(sizeof(buf))) {
+ debug("cmgr: snprintf error: 0 <= %d <= %u\n", bufLen, (unsigned)sizeof(buf));
+ return ""; // XXX review
+ }
+
int encodedLen = base64_encode_len(bufLen);
if (encodedLen <= 0)
return "";
@@ -1185,7 +1201,10 @@ make_auth_header(const cachemgr_request
stringLength += snprintf(buf, sizeof(buf), "Authorization: Basic %s\r\n", str64);
- assert(stringLength < sizeof(buf));
+ if (!(stringLength < sizeof(buf))) { /* NDEBUG */
+ xfree(str64);
+ return ""; // XXX review
+ }
snprintf(&buf[stringLength], sizeof(buf) - stringLength, "Proxy-Authorization: Basic %s\r\n", str64);
Index: squid-3.5.13/compat/inet_ntop.cc
===================================================================
--- squid-3.5.13.orig/compat/inet_ntop.cc
+++ squid-3.5.13/compat/inet_ntop.cc
@@ -225,7 +225,10 @@ inet_ntop6(const u_char *src, char *dst,
tp += strlen(tp);
break;
}
- tp += snprintf(tp, (tmp + sizeof("ffff:ffff:ffff:ffff:ffff:ffff:255.255.255.255") - tp), "%x", words[i]);
+ int rc = snprintf(tp, (tmp + sizeof(tmp) - tp), "%x", words[i]);
+ if (rc < 0 || rc >= static_cast<int>((tmp + sizeof(tmp) - tp)))
+ return (NULL);
+ tp += rc;
}
/* Was it a trailing run of 0x00's? */
if (best.base != -1 && (best.base + best.len) ==
Index: squid-3.5.13/src/ip/QosConfig.cc
===================================================================
--- squid-3.5.13.orig/src/ip/QosConfig.cc
+++ squid-3.5.13/src/ip/QosConfig.cc
@@ -371,65 +371,67 @@ Ip::Qos::Config::parseConfigLine()
* objects are part of the dump function must be self-contained.
* which means no StoreEntry refrences. Just a basic char* buffer.
*/
+#define sanitized_snprintf(p, len, fmt, ...) \
+ min(static_cast<int>(len)-1, max(0, snprintf(p, len, fmt, ##__VA_ARGS__)))
void
Ip::Qos::Config::dumpConfigLine(char *entry, const char *name) const
{
char *p = entry;
if (isHitTosActive()) {
- p += snprintf(p, 11, "%s", name); // strlen("qos_flows ");
- p += snprintf(p, 4, "%s", "tos");
+ p += sanitized_snprintf(p, 11, "%s", name); // strlen("qos_flows ");
+ p += sanitized_snprintf(p, 4, "%s", "tos");
if (tosLocalHit > 0) {
- p += snprintf(p, 16, " local-hit=0x%02X", tosLocalHit);
+ p += sanitized_snprintf(p, 16, " local-hit=0x%02X", tosLocalHit);
}
if (tosSiblingHit > 0) {
- p += snprintf(p, 18, " sibling-hit=0x%02X", tosSiblingHit);
+ p += sanitized_snprintf(p, 18, " sibling-hit=0x%02X", tosSiblingHit);
}
if (tosParentHit > 0) {
- p += snprintf(p, 17, " parent-hit=0x%02X", tosParentHit);
+ p += sanitized_snprintf(p, 17, " parent-hit=0x%02X", tosParentHit);
}
if (tosMiss > 0) {
- p += snprintf(p, 11, " miss=0x%02X", tosMiss);
+ p += sanitized_snprintf(p, 11, " miss=0x%02X", tosMiss);
if (tosMissMask!=0xFFU) {
- p += snprintf(p, 6, "/0x%02X", tosMissMask);
+ p += sanitized_snprintf(p, 6, "/0x%02X", tosMissMask);
}
}
if (preserveMissTos == 0) {
- p += snprintf(p, 23, " disable-preserve-miss");
+ p += sanitized_snprintf(p, 23, " disable-preserve-miss");
}
if (preserveMissTos && preserveMissTosMask != 0) {
- p += snprintf(p, 16, " miss-mask=0x%02X", preserveMissTosMask);
+ p += sanitized_snprintf(p, 16, " miss-mask=0x%02X", preserveMissTosMask);
}
- p += snprintf(p, 2, "\n");
+ p += sanitized_snprintf(p, 2, "\n");
}
if (isHitNfmarkActive()) {
- p += snprintf(p, 11, "%s", name); // strlen("qos_flows ");
- p += snprintf(p, 5, "%s", "mark");
+ p += sanitized_snprintf(p, 11, "%s", name); // strlen("qos_flows ");
+ p += sanitized_snprintf(p, 5, "%s", "mark");
if (markLocalHit > 0) {
- p += snprintf(p, 22, " local-hit=0x%02X", markLocalHit);
+ p += sanitized_snprintf(p, 22, " local-hit=0x%02X", markLocalHit);
}
if (markSiblingHit > 0) {
- p += snprintf(p, 24, " sibling-hit=0x%02X", markSiblingHit);
+ p += sanitized_snprintf(p, 24, " sibling-hit=0x%02X", markSiblingHit);
}
if (markParentHit > 0) {
- p += snprintf(p, 23, " parent-hit=0x%02X", markParentHit);
+ p += sanitized_snprintf(p, 23, " parent-hit=0x%02X", markParentHit);
}
if (markMiss > 0) {
- p += snprintf(p, 17, " miss=0x%02X", markMiss);
+ p += sanitized_snprintf(p, 17, " miss=0x%02X", markMiss);
if (markMissMask!=0xFFFFFFFFU) {
- p += snprintf(p, 12, "/0x%02X", markMissMask);
+ p += sanitized_snprintf(p, 12, "/0x%02X", markMissMask);
}
}
if (preserveMissMark == false) {
- p += snprintf(p, 23, " disable-preserve-miss");
+ p += sanitized_snprintf(p, 23, " disable-preserve-miss");
}
if (preserveMissMark && preserveMissMarkMask != 0) {
- p += snprintf(p, 22, " miss-mask=0x%02X", preserveMissMarkMask);
+ p += sanitized_snprintf(p, 22, " miss-mask=0x%02X", preserveMissMarkMask);
}
- p += snprintf(p, 2, "\n");
+ p += sanitized_snprintf(p, 2, "\n");
}
}
Index: squid-3.5.13/src/snmp_core.cc
===================================================================
--- squid-3.5.13.orig/src/snmp_core.cc
+++ squid-3.5.13/src/snmp_core.cc
@@ -1056,7 +1056,8 @@ snmpDebugOid(oid * Name, snint Len, MemB
outbuf.init(16, MAX_IPSTRLEN);
for (x = 0; x < Len; ++x) {
- size_t bytes = snprintf(mbuf, sizeof(mbuf), ".%u", (unsigned int) Name[x]);
+ int bytes = snprintf(mbuf, sizeof(mbuf), ".%u", (unsigned int) Name[x]);
+ if (bytes < 0 || bytes >= static_cast<int>(sizeof(mbuf))) bytes = 0; // XXX
outbuf.append(mbuf, bytes);
}
return outbuf.content();
Index: squid-3.5.13/src/url.cc
===================================================================
--- squid-3.5.13.orig/src/url.cc
+++ squid-3.5.13/src/url.cc
@@ -641,10 +641,10 @@ urlMakeAbsolute(const HttpRequest * req,
return (urlbuf);
}
- size_t urllen;
+ int surllen;
if (req->port != urlDefaultPort(req->url.getScheme())) {
- urllen = snprintf(urlbuf, MAX_URL, "%s://%s%s%s:%d",
+ surllen = snprintf(urlbuf, MAX_URL, "%s://%s%s%s:%d",
req->url.getScheme().c_str(),
req->login,
*req->login ? "@" : null_string,
@@ -652,7 +652,7 @@ urlMakeAbsolute(const HttpRequest * req,
req->port
);
} else {
- urllen = snprintf(urlbuf, MAX_URL, "%s://%s%s%s",
+ surllen = snprintf(urlbuf, MAX_URL, "%s://%s%s%s",
req->url.getScheme().c_str(),
req->login,
*req->login ? "@" : null_string,
@@ -660,6 +660,11 @@ urlMakeAbsolute(const HttpRequest * req,
);
}
+ if (surllen < 0 || surllen >= MAX_URL)
+ return NULL; // XXX review
+
+ size_t urllen = static_cast<size_t>(surllen);
+
if (relUrl[0] == '/') {
strncpy(&urlbuf[urllen], relUrl, MAX_URL - urllen - 1);
} else {
_______________________________________________
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev
