DecOverflowWhenComparing query

README.md

@@ -29,6 +29,8 @@ codeql database analyze database.db --format=sarif-latest --output=./tob.sarif -
 
 | Name | Description | Severity | Precision  |
 | ---  | ----------- | :----:   | :--------: |
+|[BN_CTX_free called before BN_CTX_end](./cpp/src/docs/crypto/BnCtxFreeBeforeEnd.md)|Detects BN_CTX_free called before BN_CTX_end, which violates the required lifecycle|error|medium|
+|[Unbalanced BN_CTX_start and BN_CTX_end pair](./cpp/src/docs/crypto/UnbalancedBnCtx.md)|Detects if one call in the BN_CTX_start/BN_CTX_end pair is missing|warning|medium|
 |[Crypto variable initialized using static key](./cpp/src/docs/crypto/StaticKeyFlow.md)|Finds crypto variables initialized using static keys|error|high|
 |[Crypto variable initialized using static password](./cpp/src/docs/crypto/StaticPasswordFlow.md)|Finds crypto variables initialized using static passwords|error|high|
 |[Crypto variable initialized using weak randomness](./cpp/src/docs/crypto/WeakRandomnessTaint.md)|Finds crypto variables initialized using weak randomness|error|high|
@@ -46,9 +48,10 @@ codeql database analyze database.db --format=sarif-latest --output=./tob.sarif -
 | Name | Description | Severity | Precision  |
 | ---  | ----------- | :----:   | :--------: |
 |[Async unsafe signal handler](./cpp/src/docs/security/AsyncUnsafeSignalHandler/AsyncUnsafeSignalHandler.md)|Async unsafe signal handler (like the one used in CVE-2024-6387)|warning|high|
+|[Decrementation overflow when comparing](./cpp/src/docs/security/DecOverflowWhenComparing/DecOverflowWhenComparing.md)|This query finds unsigned integer overflows resulting from unchecked decrementation during comparison.|error|high|
+|[Find all problematic implicit casts](./cpp/src/docs/security/UnsafeImplicitConversions/UnsafeImplicitConversions.md)|Find all implicit casts that may be problematic. That is, casts that may result in unexpected truncation, reinterpretation or widening of values.|error|high|
 |[Invalid string size passed to string manipulation function](./cpp/src/docs/security/CStrnFinder/CStrnFinder.md)|Finds calls to functions that take as input a string and its size as separate arguments (e.g., `strncmp`, `strncat`, ...) and the size argument is wrong|error|low|
 |[Missing null terminator](./cpp/src/docs/security/NoNullTerminator/NoNullTerminator.md)|This query finds incorrectly initialized strings that are passed to functions expecting null-byte-terminated strings|error|high|
-|[Unsafe implicit integer conversion](./cpp/src/docs/security/UnsafeImplicitConversions/UnsafeImplicitConversions.md)|Finds implicit integer casts that may overflow or be truncated, with false positive reduction via Value Range Analysis|warning|low|
 
 ### Go
 
@@ -63,7 +66,7 @@ codeql database analyze database.db --format=sarif-latest --output=./tob.sarif -
 | Name | Description | Severity | Precision  |
 | ---  | ----------- | :----:   | :--------: |
 |[Invalid file permission parameter](./go/src/docs/security/FilePermsFlaws/FilePermsFlaws.md)|Finds non-octal (e.g., `755` vs `0o755`) and unsupported (e.g., `04666`) literals used as a filesystem permission parameter (`FileMode`)|error|medium|
-|[Missing MinVersion in tls.Config](./go/src/docs/security/MissingMinVersionTLS/MissingMinVersionTLS.md)|This rule finds cases when you do not set the `tls.Config.MinVersion` explicitly for servers. By default version 1.0 is used, which is considered insecure. This rule does not mark explicitly set insecure versions|error|medium|
+|[Missing MinVersion in tls.Config](./go/src/docs/security/MissingMinVersionTLS/MissingMinVersionTLS.md)|Finds uses of tls.Config where MinVersion is not set and the project's minimum Go version (from go.mod) indicates insecure defaults: Go < 1.18 for clients or Go < 1.22 for servers. Does not mark explicitly set versions (including explicitly insecure ones).|error|medium|
 |[Trim functions misuse](./go/src/docs/security/TrimMisuse/TrimMisuse.md)|Finds calls to `string.{Trim,TrimLeft,TrimRight}` with the 2nd argument not being a cutset but a continuous substring to be trimmed|error|low|
 
 ### Java-kotlin
@@ -72,7 +75,7 @@ codeql database analyze database.db --format=sarif-latest --output=./tob.sarif -
 
 | Name | Description | Severity | Precision  |
 | ---  | ----------- | :----:   | :--------: |
-|[Recursive functions](./java-kotlin/src/docs/security/Recursion/Recursion.md)|Detects recursive calls|warning|low|
+|[Recursive functions](./java-kotlin/src/docs/security/Recursion/Recursion.md)|Detects possibly unbounded recursive calls|warning|low|
 
 ## Query suites
 

cpp/src/docs/security/DecOverflowWhenComparing/DecOverflowWhenComparing.md

@@ -0,0 +1,72 @@
+# Decrementation overflow when comparing
+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
+
+
+## Recommendation
+Move the decrementation outside of comparison and/or add explicit checks for overflows.
+
+
+## Example
+
+```c
+#include <stdio.h>
+#include <stdint.h>
+#include <stdlib.h>
+#include <string.h>
+
+// 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 domainlen = 0;
+	char *domain = NULL;
+
+	if ((data == NULL) || (datalen == 0)) return NULL;
+
+	while (datalen-- > 0)
+	{
+		uint32_t 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);
+}
+
+```
+The `datalen` variable may overflow to UINT_MAX given a specific input.
+

cpp/src/security/DecOverflowWhenComparing/DecOverflowWhenComparing.c

@@ -0,0 +1,55 @@
+#include <stdio.h>
+#include <stdint.h>
+#include <stdlib.h>
+#include <string.h>
+
+// 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 domainlen = 0;
+	char *domain = NULL;
+
+	if ((data == NULL) || (datalen == 0)) return NULL;
+
+	while (datalen-- > 0)
+	{
+		uint32_t 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);
+}

cpp/src/security/DecOverflowWhenComparing/DecOverflowWhenComparing.qhelp

@@ -0,0 +1,34 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>
+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.
+</p>
+
+<p>
+You can read about a real-world vulnerability here: https://github.com/trailofbits/exploits/tree/main/obts-2025-macos-lpe
+</p>
+</overview>
+
+<recommendation>
+<p>
+Move the decrementation outside of comparison and/or add explicit checks for overflows.
+</p>
+</recommendation>
+
+<example>
+<sample src="DecOverflowWhenComparing.c" />
+
+<p>
+The <code>datalen</code> variable may overflow to UINT_MAX given a specific input.
+</p>
+</example>
+
+</qhelp>
+

cpp/src/security/DecOverflowWhenComparing/DecOverflowWhenComparing.ql

@@ -0,0 +1,146 @@
+/**
+ * @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.rangeanalysis.SimpleRangeAnalysis
+import semmle.code.cpp.controlflow.StackVariableReachability
+
+/**
+ * Holds if `node` overwrites `var` (assignment or declaration with initializer).
+ */
+predicate isDefOf(ControlFlowNode node, Variable var) {
+  node = var.getAnAccess() and node.(VariableAccess).isLValue()
+  or
+  node.(DeclStmt).getADeclaration() = var and exists(var.getInitializer())
+  or
+  node.(Assignment).getLValue().(VariableAccess).getTarget() = var
+}
+
+/**
+ * Identifies an unsigned postfix decrement inside a comparison against zero.
+ */
+pragma[nomagic]
+predicate isDecInComparison(
+  PostfixDecrExpr dec, VariableAccess varAcc,
+  ComparisonOperation cmp, Variable var
+) {
+  varAcc = var.getAnAccess() and
+  dec.getOperand() = varAcc.getExplicitlyConverted() and
+  varAcc.getUnderlyingType().(IntegralType).isUnsigned() and
+  cmp.getAnOperand().getAChild*() = varAcc and
+  cmp.getAnOperand() instanceof Zero and
+  lowerBound(varAcc) = 0
+}
+
+/**
+ * Identifies a non-assignment read of a variable
+ * (i.e., a use that could observe an overflowed value).
+ */
+pragma[nomagic]
+predicate isReadOf(VariableAccess va, Variable var) {
+  va = var.getAnAccess() and
+  not exists(AssignExpr ae | ae.getLValue() = va)
+}
+
+/**
+ * Basic-block-level reachability from a decrement comparison to a use
+ * of the same stack variable, blocked by any definition of the variable.
+ */
+class DecOverflowReach extends StackVariableReachability {
+  DecOverflowReach() { this = "DecOverflowReach" }
+
+  override predicate isSource(ControlFlowNode node, StackVariable v) {
+    isDecInComparison(_, _, node, v)
+  }
+
+  override predicate isSink(ControlFlowNode node, StackVariable v) {
+    isReadOf(node, v)
+  }
+
+  override predicate isBarrier(ControlFlowNode node, StackVariable v) {
+    isDefOf(node, v)
+  }
+}
+
+/**
+ * BB-level reachability for non-stack variables (globals, static locals).
+ * Holds if `sink` is reachable from the entry of `bb` without crossing
+ * a definition of `var`.
+ */
+private predicate nonStackBBEntryReaches(
+  BasicBlock bb, Variable var, ControlFlowNode sink
+) {
+  exists(int n |
+    sink = bb.getNode(n) and
+    isReadOf(sink, var) and
+    not exists(int m | m < n | isDefOf(bb.getNode(m), var))
+  )
+  or
+  not isDefOf(bb.getNode(_), var) and
+  nonStackBBEntryReaches(bb.getASuccessor(), var, sink)
+}
+
+/**
+ * BB-level reachability from `source` to `sink` for non-stack variables,
+ * without crossing a definition of `var`.
+ */
+pragma[nomagic]
+predicate nonStackReaches(
+  ComparisonOperation source, Variable var, ControlFlowNode sink
+) {
+  not var instanceof StackVariable and
+  exists(BasicBlock bb, int i |
+    bb.getNode(i) = source and
+    not bb.isUnreachable()
+  |
+    exists(int j |
+      j > i and
+      sink = bb.getNode(j) and
+      isReadOf(sink, var) and
+      not exists(int k | k in [i + 1 .. j - 1] | isDefOf(bb.getNode(k), var))
+    )
+    or
+    not exists(int k | k > i | isDefOf(bb.getNode(k), var)) and
+    nonStackBBEntryReaches(bb.getASuccessor(), var, sink)
+  )
+}
+
+from
+  Variable var, VariableAccess varAcc, PostfixDecrExpr dec,
+  VariableAccess varAccAfterOverflow, ComparisonOperation cmp
+where
+  isDecInComparison(dec, varAcc, cmp, var) and
+  isReadOf(varAccAfterOverflow, var) and
+  varAcc != varAccAfterOverflow and
+  // reachable without intervening overwrite
+  (
+    any(DecOverflowReach r).reaches(cmp, var, varAccAfterOverflow)
+    or
+    nonStackReaches(cmp, var, varAccAfterOverflow)
+  ) and
+  // exclude accesses that are part of the comparison expression itself
+  not cmp.getAnOperand().getAChild*() = varAccAfterOverflow and
+  // var-- > 0 (0 < var--) then only accesses in false branch matter
+  (
+    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()
+  ) and
+  // skip vendor code
+  not dec.getFile().getAbsolutePath().toLowerCase().matches(["%vendor%", "%third_party%"])
+select dec, "Unsigned decrementation in comparison ($@) - $@", cmp, cmp.toString(),
+  varAccAfterOverflow, varAccAfterOverflow.toString()

cpp/test/include/libc/stdint.h

@@ -0,0 +1,15 @@
+#ifndef USE_HEADERS
+
+#ifndef HEADER_STDINT_STUB_H
+#define HEADER_STDINT_STUB_H
+
+typedef unsigned char uint8_t;
+typedef unsigned short uint16_t;
+typedef unsigned int uint32_t;
+
+#endif
+#else // --- else USE_HEADERS
+
+#include <stdint.h>
+
+#endif // --- end USE_HEADERS

cpp/test/include/libc/stdlib.h

@@ -7,6 +7,8 @@
 extern "C" {
 #endif
 
+extern void *reallocf(void *, unsigned long);
+
 int rand(void) {
   return 42;
 }
@@ -32,4 +34,4 @@ void *malloc(unsigned long);
 
 #include <stdlib.h>
 
-#endif // --- end USE_HEADERS
+#endif // --- end USE_HEADERS

cpp/test/include/libc/string_stubs.h

@@ -12,6 +12,7 @@ extern "C" {
 #endif
 
 typedef int wchar_t;
+extern void *memcpy(void *dst, const void *src, unsigned long n);
 extern char* strcpy_s(char* dst, int max_amount, char* src);
 extern int _mbsncat(char* dst, char* src, int count);
 extern int _mbsncmp(char* dst, char* src, int count);
@@ -50,4 +51,4 @@ extern void closelog(void);
 #define _mbsnbcmp memcmp
 #define mempcpy memcpy
 
-#endif
+#endif