diff --git a/cpp/src/security/DecOverflowWhenComparing/DecOverflowWhenComparing.c b/cpp/src/security/DecOverflowWhenComparing/DecOverflowWhenComparing.c new file mode 100644 index 0000000..465ad8a --- /dev/null +++ b/cpp/src/security/DecOverflowWhenComparing/DecOverflowWhenComparing.c @@ -0,0 +1,62 @@ +#include +#include +#include +#include + +// from https://github.com/apple-oss-distributions/Libinfo/blob/9fce29e5c5edc15d3ecea55116ca17d3f6350603/lookup.subproj/mdns_module.c#L1033C1-L1079C2 +char* _mdns_parse_domain_name(const uint8_t *data, uint32_t datalen) +{ + int i = 0, j = 0; + uint32_t len; + uint32_t domainlen = 0; + char *domain = NULL; + + if ((data == NULL) || (datalen == 0)) return NULL; + + /* + * i: index into input data + * j: index into output string + */ + while (datalen-- > 0) + { + printf("%d\n", len); + len = data[i++]; + domainlen += (len + 1); + domain = reallocf(domain, domainlen); + + if (domain == NULL) return NULL; + + if (len == 0) break; // DNS root (NUL) + + if (j > 0) + { + domain[j++] = datalen ? '.' : '\0'; + } + + while ((len-- > 0) && (0 != datalen--)) + { + if (data[i] == '.') + { + /* special case: escape the '.' with a '\' */ + domain = reallocf(domain, ++domainlen); + if (domain == NULL) return NULL; + + domain[j++] = '\\'; + } + + domain[j++] = data[i++]; + } + } + + domain[j] = '\0'; + + return domain; +} + +int main() { + const uint16_t datalen = 128; + uint8_t data[datalen] = {}; + memcpy(data, "\x04quildu\x03xyz\x00", 11); + _mdns_parse_domain_name(data, datalen); +} + diff --git a/cpp/src/security/DecOverflowWhenComparing/DecOverflowWhenComparing.qhelp b/cpp/src/security/DecOverflowWhenComparing/DecOverflowWhenComparing.qhelp new file mode 100644 index 0000000..c51c425 --- /dev/null +++ b/cpp/src/security/DecOverflowWhenComparing/DecOverflowWhenComparing.qhelp @@ -0,0 +1,34 @@ + + + +

+Finds unsigned integer overflow issues with the following heuristic: +* variable is compared to 0 and decremented +* variable is used after the comparison and decrementation + +In such cases it is likely that the decrementation was not expected. +

+ +

+You can read about a real-world vulnerability here: https://github.com/trailofbits/exploits/tree/main/obts-2025-macos-lpe +

+
+ + +

+Move the decrementation outside of comparison and/or add explicit checks for overflows. +

+
+ + + + +

+The datalen variable may overflow to UINT_MAX given a specific input. +

+
+ +
+ diff --git a/cpp/src/security/DecOverflowWhenComparing/DecOverflowWhenComparing.ql b/cpp/src/security/DecOverflowWhenComparing/DecOverflowWhenComparing.ql new file mode 100644 index 0000000..de29b18 --- /dev/null +++ b/cpp/src/security/DecOverflowWhenComparing/DecOverflowWhenComparing.ql @@ -0,0 +1,57 @@ +/** + * @name Decrementation overflow when comparing + * @id tob/cpp/dec-overflow-when-comparing + * @description This query finds unsigned integer overflows resulting from unchecked decrementation during comparison. + * @kind problem + * @tags security + * @problem.severity error + * @precision high + * @security-severity 5.0 + * @group security + */ + +import cpp +import semmle.code.cpp.ir.IR + +from Variable var, VariableAccess varAcc, DecrementOperation dec, + VariableAccess varAccAfterOverflow, ComparisonOperation cmp +where + // get unsigned variable that is decremented + varAcc = var.getAnAccess() and + varAccAfterOverflow = var.getAnAccess() and + varAcc != varAccAfterOverflow and + dec.getOperand() = varAcc.getExplicitlyConverted() and + varAcc.getUnderlyingType().(IntegralType).isUnsigned() and + + // only decrementations inside comparisons + cmp.getAnOperand().getAChild*() = varAcc and + cmp.getAnOperand() instanceof Zero and + + // only if the variable is used after the comparison + cmp.getASuccessor+() = varAccAfterOverflow and + cmp.getAnOperand().getAChild*() != varAccAfterOverflow and + + // skip if the variable is overwritten + // TODO: handle loops correctly + // not exists(VariableAccess varAccLV | varAccLV.isUsedAsLValue() | + // varAccLV = var.getAnAccess() and + // varAccLV != varAcc and + // varAccLV != varAccAfterOverflow and + // cmp.getASuccessor+() = varAccLV and + // varAccAfterOverflow.getAPredecessor+() = varAccLV + // ) and + + // var-- > 0 (0 < var--) then accesses only in false branch + // var-- >= 0 then accesses in all branches + // var-- == 0 then accesses in all branches + // var-- != 0 then accesses in all branches + if ( + (cmp instanceof GTExpr and cmp.getRightOperand() instanceof Zero) + or + (cmp instanceof LTExpr and cmp.getLeftOperand() instanceof Zero) + ) + then + cmp.getAFalseSuccessor().getASuccessor*() = varAccAfterOverflow + else + any() +select cmp, varAccAfterOverflow \ No newline at end of file diff --git a/cpp/test/query-tests/security/DecOverflowWhenComparing/DecOverflowWhenComparing.c b/cpp/test/query-tests/security/DecOverflowWhenComparing/DecOverflowWhenComparing.c new file mode 100644 index 0000000..08bcb71 --- /dev/null +++ b/cpp/test/query-tests/security/DecOverflowWhenComparing/DecOverflowWhenComparing.c @@ -0,0 +1,66 @@ +#ifndef USE_HEADERS +#include "../../../include/libc/string_stubs.h" + +#else +#include +#include +#include +#include +#endif + +// from https://github.com/apple-oss-distributions/Libinfo/blob/9fce29e5c5edc15d3ecea55116ca17d3f6350603/lookup.subproj/mdns_module.c#L1033C1-L1079C2 +char* _mdns_parse_domain_name(const uint8_t *data, uint32_t datalen) +{ + int i = 0, j = 0; + uint32_t len; + uint32_t domainlen = 0; + char *domain = NULL; + + if ((data == NULL) || (datalen == 0)) return NULL; + + /* + * i: index into input data + * j: index into output string + */ + while (datalen-- > 0) + { + len = data[i++]; + domainlen += (len + 1); + domain = reallocf(domain, domainlen); + + if (domain == NULL) return NULL; + + if (len == 0) break; // DNS root (NUL) + + if (j > 0) + { + domain[j++] = datalen ? '.' : '\0'; + } + + while ((len-- > 0) && (0 != datalen--)) + { + if (data[i] == '.') + { + /* special case: escape the '.' with a '\' */ + domain = reallocf(domain, ++domainlen); + if (domain == NULL) return NULL; + + domain[j++] = '\\'; + } + + domain[j++] = data[i++]; + } + } + + domain[j] = '\0'; + + return domain; +} + +int main() { + const uint16_t datalen = 128; + uint8_t data[datalen] = {}; + memcpy(data, "\x04quildu\x03xyz\x00", 11); + _mdns_parse_domain_name(data, datalen); +} + diff --git a/cpp/test/query-tests/security/DecOverflowWhenComparing/DecOverflowWhenComparing.expected b/cpp/test/query-tests/security/DecOverflowWhenComparing/DecOverflowWhenComparing.expected new file mode 100644 index 0000000..e69de29 diff --git a/cpp/test/query-tests/security/DecOverflowWhenComparing/DecOverflowWhenComparing.qlref b/cpp/test/query-tests/security/DecOverflowWhenComparing/DecOverflowWhenComparing.qlref new file mode 100644 index 0000000..154ed3e --- /dev/null +++ b/cpp/test/query-tests/security/DecOverflowWhenComparing/DecOverflowWhenComparing.qlref @@ -0,0 +1 @@ +security/DecOverflowWhenComparing/DecOverflowWhenComparing.ql \ No newline at end of file