When Remote Lengths Break Local Arithmetic
Manual code audits tend to follow a familiar path: identify attacker-controlled input, trace it through program logic, and look for places where that influence turns into a flaw. In network protocol parsers written in memory-unsafe languages, one of the most reliable places to look is at the handling of length-prefixed data. The common [length][value] construct appears across nearly every protocol, and when the length is a remotely supplied integer, the arithmetic around it deserves close scrutiny.
Integer overflow, promotion, and truncation in C trip up even experienced developers. A calculation that appears sound at a glance can collapse under edge cases, and when the integers feeding that calculation come from a network packet, the results can be exploitable. This pattern surfaced recently in the ntop Deep Packet Inspection (nDPI) library, where the SSH protocol dissector contained multiple integer overflow vulnerabilities leading to a controlled remote heap overflow, along with out-of-bounds (OOB) read flaws in both the SSH and Postgres dissectors. These were reported as GHSL-2020-051 and GHSL-2020-052 and have since been fixed.
Integer Wrapping in ssh.c:concat_hash_string
The nDPI SSH dissector processes KEXINIT messages (type 20) in both directions, extracting descriptive string sets such as supported key exchange algorithms. These strings follow the standard [length][data] format, where length is a 32-bit integer. The dissector reads the length, then uses it to pull the string data and update a running offset into the captured packet. That offset is stored as a 16-bit unsigned integer.
To pull key exchange algorithms from a KEXINIT packet, the code performs the following operations:
ssh.c:ndpi_search_ssh_tcp
...
if(msgcode == 20 /* key exchange init */) {
char *hassh_buf = calloc(packet->payload_packet_len, sizeof(char));
...
len = concat_hash_string(packet, hassh_buf, 0 /* server */);
...
ssh.c:concat_hash_string
...
u_int16_t offset = 22, buf_out_len = 0;
if(offset+sizeof(u_int32_t) >= packet->payload_packet_len)
goto invalid_payload;
u_int32_t len = ntohl(*(u_int32_t*)&packet->payload[offset]);
offset += 4;
/* -1 for ';' */
if((offset >= packet->payload_packet_len) || (len >= packet->payload_packet_len-offset-1))
goto invalid_payload;
/* ssh.kex_algorithms [C/S] */
strncpy(buf, (const char *)&packet->payload[offset], buf_out_len = len);
buf[buf_out_len++] = ';';
offset += len;
Two observations stand out immediately. The destination buffer buf is allocated with calloc, sized according to packet->payload_packet_len, which corresponds to the actual captured SSH packet size. The code also attempts to verify that the offset and len values keep accesses within packet->payload_packet_len, with the intent of preventing reads or writes outside the allocated buf region.
A closer look at concat_hash_string reveals the issue:
ssh.c:concat_hash_string
...
[1]
/* ssh.encryption_algorithms_client_to_server [C] */
len = ntohl(*(u_int32_t*)&packet->payload[offset]);
if(client_hash) {
offset += 4;
if((offset >= packet->payload_packet_len) || (len >= packet->payload_packet_len-offset-1))
goto invalid_payload;
strncpy(&buf[buf_out_len], (const char *)&packet->payload[offset], len);
buf_out_len += len;
buf[buf_out_len++] = ';';
offset += len;
} else
[2]
offset += 4 + len;
/* ssh.encryption_algorithms_server_to_client [S] */
len = ntohl(*(u_int32_t*)&packet->payload[offset]);
if(!client_hash) {
offset += 4;
if((offset >= packet->payload_packet_len) || (len >= packet->payload_packet_len-offset-1))
goto invalid_payload;
strncpy(&buf[buf_out_len], (const char *)&packet->payload[offset], len);
buf_out_len += len;
buf[buf_out_len++] = ';';
offset += len;
} else
offset += 4 + len;
[3]
/* ssh.mac_algorithms_client_to_server [C] */
len = ntohl(*(u_int32_t*)&packet->payload[offset]);
if(client_hash) {
offset += 4;
[4]
if((offset >= packet->payload_packet_len) || (len >= packet->payload_packet_len-offset-1))
goto invalid_payload;
[5]
strncpy(&buf[buf_out_len], (const char *)&packet->payload[offset], len);
buf_out_len += len;
buf[buf_out_len++] = ';';
offset += len;
} else
[6]
offset += 4 + len;
The client_hash variable determines packet direction, but the parsing pattern is identical either way, so the !client_hash case suffices for analysis. At [1], a 32-bit unsigned length integer is fully attacker-controlled. At [2], the offset is updated as offset + 4 + len. This arithmetic is performed with 32-bit values, then truncated to 16 bits when the result is assigned back to offset. Because len is fully controlled, this truncation allows offset to wrap to any desired 16-bit value.
To set offset to an arbitrary n, an attacker simply supplies len = 0 - offset - 4 + n. Since offset is checked at [4] to prevent OOB access, wrapping it to a small value passes the intended bounds check. The result is a controlled heap overflow: the code reaches a strncpy at [5] that writes into buf[buf_out_len], where buf_out_len is incremented by the same attacker-controlled length and is never independently bounds-checked.
Building an Overflow Primitive
While offset can be reset at will, the direct size of each copy is constrained: the check controlled_len >= packet->payload_packet_len - offset - 1 prevents a single oversized string from overflowing buf. However, packet->payload_packet_len also controls the calloc size for buf. An attacker can pack one string into the KEXINIT packet, causing an allocation based on that string’s size plus protocol overhead, then repeatedly copy that same string.
Each iteration resets offset to point back at the initial string data, while buf_out_len keeps incrementing past the end of the buffer. Since buf_out_len is never checked, the second copy operation can already write out of bounds, depending on the ratio of string size to protocol overhead—both of which are under remote control. This turns the flaw into a controlled remote heap overflow. Given that nDPI allocates, deallocates, and populates heap memory in direct response to remotely supplied packet data, this is a viable primitive for remote code execution.
Related OOB Reads
A second, less severe but related issue manifests as an OOB read. The following snippet shows the pattern:
ssh.c:concat_hash_string
...
/* ssh.server_host_key_algorithms [None] */
len = ntohl(*(u_int32_t*)&packet->payload[offset]);
[1]
offset += 4 + len;
/* ssh.encryption_algorithms_client_to_server [C] */
[2]
len = ntohl(*(u_int32_t*)&packet->payload[offset]);
...
At [1], offset is computed using the same overflow-prone arithmetic. At [2], the resulting value is immediately used as an index into packet data without any bounds check. Since offset is fully user-controlled via the length integer, this can read past the end of the packet buffer. The same pattern repeats wherever a fresh offset is calculated without an explicit validation step, for example:
/* ssh.encryption_algorithms_client_to_server [C] */
len = ntohl(*(u_int32_t*)&packet->payload[offset]);
if(client_hash) {
offset += 4;
if((offset >= packet->payload_packet_len) || (len >= packet->payload_packet_len-offset-1))
goto invalid_payload;
strncpy(&buf[buf_out_len], (const char *)&packet->payload[offset], len);
buf_out_len += len;
buf[buf_out_len++] = ';';
offset += len;
} else
[1]
offset += 4 + len;
/* ssh.encryption_algorithms_server_to_client [S] */
[2]
len = ntohl(*(u_int32_t*)&packet->payload[offset]);
Here again, the remote len at [1] directly sets offset, which is then used at [2] as a packet data index with no validation. The OOB read can crash the parser, resulting in a denial of service.
Verification via Modified Client
Both issues are triggerable by crafting a malicious KEXINIT SSH message and sending it over a link monitored by nDPI. During testing, the Paramiko SSH library was modified to emit a KEXINIT message that triggers the integer wrap and subsequent memory corruption:
# nDPI/example › gdb ./ndpiReader
...
(gdb) break concat_hash_string
Breakpoint 1 at 0x431c00: file protocols/ssh.c, line 99.
(gdb) r -i wlp1s0
Starting program: /home/bas/repos/nDPI/example/ndpiReader -i wlp1s0
...
Thread 2 "ndpiReader" hit Breakpoint 1, concat_hash_string (
packet=0x7ffff0046130, buf=buf@entry=0x7ffff0047340 "",
client_hash=client_hash@entry=0 '\000') at protocols/ssh.c:99
99 if(offset+sizeof(u_int32_t) >= packet->payload_packet_len)
Missing separate debuginfos, use: dnf debuginfo-install libgcc-9.2.1-1.fc31.x86_64 libpcap-1.9.1-2.fc31.x86_64 libstdc++-9.2.1-1.fc31.x86_64
(gdb) p buf
$1 = 0x7ffff0047340 ""
(gdb) p packet->payload_packet_len
$2 = 1064
(gdb) break strncpy
Breakpoint 2 at 0x7ffff7b69110
(gdb) c
Continuing.
Thread 2 "ndpiReader" hit Breakpoint 2, 0x00007ffff7b69110 in __strncpy_avx2
() from /lib64/libc.so.6
(gdb) finish
Run till exit from #0 0x00007ffff7b69110 in __strncpy_avx2 ()
from /lib64/libc.so.6
concat_hash_string (packet=0x7ffff0046130,
buf=buf@entry=0x7ffff0047340 'A' <repeats 200 times>...,
client_hash=client_hash@entry=0 '\000') at protocols/ssh.c:110
110 buf[buf_out_len++] = ';';
(gdb) c
Continuing.
Thread 2 "ndpiReader" hit Breakpoint 2, 0x00007ffff7b69110 in __strncpy_avx2
() from /lib64/libc.so.6
(gdb) finish
Run till exit from #0 0x00007ffff7b69110 in __strncpy_avx2 ()
from /lib64/libc.so.6
concat_hash_string (packet=0x7ffff0046130,
buf=buf@entry=0x7ffff0047340 'A' <repeats 200 times>...,
client_hash=client_hash@entry=0 '\000') at protocols/ssh.c:144
144 buf[buf_out_len++] = ';';
(gdb) p buf_out_len
$3 = 2050
(gdb) p packet->payload_packet_len
$4 = 1064
(gdb) c
Continuing.
free(): invalid next size (normal)
Thread 2 "ndpiReader" received signal SIGABRT, Aborted.
0x00007ffff7a42625 in raise () from /lib64/libc.so.6
(gdb)
After the second strncpy call, buf_out_len has moved past the end of the buf allocation (which tracks packet->payload_packet_len), and the ensuing heap corruption trips a heap integrity check when the ndpiReader application continues running. Both the overflow and the OOB reads stem from the same root cause: trusting attacker-supplied length integers in arithmetic that eventually controls memory access.
Scaling the audit with CodeQL
After manually triaging remote integers across nDPI’s dissectors, we wanted to see whether the same vulnerable pattern repeated elsewhere in the codebase. Rather than starting from scratch, we distilled the essence of our findings into a CodeQL query. The goal was to identify places where a network-supplied integer is both used in arithmetic and subsequently influences a memory access, a combination that previously produced out-of-bounds reads.
The key was asking a pointed question. A broad query for all integer arithmetic would return thousands of results. Instead, we scoped the analysis to taint sources that read from packet->payload via typical nDPI patterns like ntohl, ntohs, and get_u_int32_t. If any such value reaches both an arithmetic operation and an array index, it becomes a candidate for manual review.
/**
* @name Suspicious packet->payload based integer arithmetic
* @description An arithmetic operation influenced array access is suspicious
* if it uses an integer value that is likely to be network-controlled, and
* may require a closer manual audit.
* @kind problem
* @problem.severity warning
* @id cpp/packet-payload-integer-arithmetic
* @tags audit security
*/
import cpp
import semmle.code.cpp.dataflow.TaintTracking
import semmle.code.cpp.rangeanalysis.SimpleRangeAnalysis
/** A source of an integer value that is likely to come from the network.
* This is produced by an invocation of a macro of the form `ntoh*` or `get_u_int*_t`,
* called with `packet->payload` as an argument.
*/
class NetworkMacro extends Macro {
NetworkMacro() { this.getName().regexpMatch("^ntoh(ll|l|s)") }
}
class NetworkIntegerSource extends Expr {
NetworkIntegerSource() {
exists(MacroInvocation mi |
this = mi.getExpr() and
mi.getUnexpandedArgument(0).regexpMatch(".*packet->payload.*") |
// catch all get_u_int*_t(x)
mi.getMacroName().regexpMatch("^get_u_int(64|32|16|8)_t") and
// dedup ntoh*(get_u_int*_t(x)) since we'll catch those in the next case
not mi.getOutermostMacroAccess().getMacro() instanceof NetworkMacro
or
// catch all ntoh*(x) ... this will also catch the nested cases
mi.getMacro() instanceof NetworkMacro
)
}
}
class ArithmeticOperation extends Operation {
ArithmeticOperation() {
this instanceof UnaryArithmeticOperation or this instanceof BinaryArithmeticOperation
}
}
class NetworkToArrayAccess extends TaintTracking::Configuration {
NetworkToArrayAccess() { this = "NetworkToArrayAccess" }
override predicate isSource(DataFlow::Node source) {
source.asExpr() instanceof NetworkIntegerSource
}
override predicate isSink(DataFlow::Node sink) {
exists(ArrayExpr ae | sink.asExpr() = ae.getArrayOffset())
}
}
class NetworkToArithmetic extends TaintTracking::Configuration {
NetworkToArithmetic() { this = "NetworkToArithmetic" }
override predicate isSource(DataFlow::Node source) {
source.asExpr() instanceof NetworkIntegerSource
}
override predicate isSink(DataFlow::Node sink) {
exists (Assignment assign |
sink.asExpr() = assign.getRValue().(ArithmeticOperation) or
sink.asExpr() = assign.(AssignArithmeticOperation)
) or
exists(LocalVariable var |
sink.asExpr() = var.getInitializer().getExpr().(ArithmeticOperation)
)
}
}
// find audit candidates based on suspicious network integer use
from NetworkIntegerSource source, Expr sink1, Expr sink2, NetworkToArithmetic config1, NetworkToArrayAccess config2
where config1.hasFlow(DataFlow::exprNode(source), DataFlow::exprNode(sink1))
// or this if you want integer arithmeric _OR_ array accesses
and config2.hasFlow(DataFlow::exprNode(source), DataFlow::exprNode(sink2))
select source, "Suspicious use of network integer arithmetic."
Running the query against nDPI produced a manageable result set that closely matched our earlier findings, including the SSH dissector, plus a handful of new candidates. After triaging each match and discarding those with explicit bounds checks, two issues in the Postgres dissector stood out.
postgres.c:ndpi_search_postgres_tcp:
...
if (flow->l4.tcp.postgres_stage == 5 && packet->payload[0] == 'R') {
if (ntohl(get_u_int32_t(packet->payload, 1)) == packet->payload_packet_len - 1) {
NDPI_LOG_INFO(ndpi_struct, "found postgres asymmetrically\n");
ndpi_int_postgres_add_connection(ndpi_struct, flow);
return;
}
[1]
size = (u_int16_t)ntohl(get_u_int32_t(packet->payload, 1)) + 1;
if (packet->payload[size - 1] == 'S') {
if ((size + get_u_int32_t(packet->payload, (size + 1))) == packet->payload_packet_len) {
NDPI_LOG_INFO(ndpi_struct, "found postgres asymmetrically\n");
ndpi_int_postgres_add_connection(ndpi_struct, flow);
return;
}
}
[2]
size += get_u_int32_t(packet->payload, (size + 1)) + 1;
if (packet->payload[size - 1] == 'S') {
NDPI_LOG_INFO(ndpi_struct, "found postgres asymmetrically\n");
ndpi_int_postgres_add_connection(ndpi_struct, flow);
return;
}
}
...
The pattern at [1] and [2] mirrors what we saw in SSH: a network-supplied length is used in arithmetic and the result indexes into memory without validation. Both cases were reported and fixed.
Putting the query into CI
To prevent regressions, we submitted a pull request to ntop/nDPI adding our query as .lgtm/cpp-queries/packet-payload-integer-arithmetic.ql. With that in place, the lgtm system flags alerts for any future commits that repeat this exact pattern.
A second look at the patch
Shortly after the GHSL advisory was published, researcher Ronald Huizer spotted an integer overflow in ntop’s original fix. The patch added a guard to prevent offset += 4 + len from wrapping past UINT32_MAX, which looked correct at first glance.
if(offset+sizeof(u_int32_t) >= packet->payload_packet_len)
goto invalid_payload;
The problem is subtle. The addition of sizeof(u_int32_t) relies on the return type of sizeof(), which is size_t. On 64-bit platforms, that means 64-bit arithmetic and the guard works as intended. But on 32-bit platforms, size_t is 32 bits, so the whole expression stays within 32-bit range. Even though the preceding check prevents offset from exceeding UINT32_MAX, an offset in the range UINT32_MAX - 4 to UINT32_MAX - 1 wraps when 4 is added.
That wraps offset back to a smaller value, effectively recreating the original condition for a heap overflow. The second strncpy then uses a buf_out_len that already points near the end of the buffer.
static u_int16_t concat_hash_string(struct ndpi_packet_struct *packet,
char *buf, u_int8_t client_hash) {
u_int32_t offset = 22, buf_out_len = 0;
if(offset+sizeof(u_int32_t) >= packet->payload_packet_len)
goto invalid_payload;
u_int32_t len = ntohl(*(u_int32_t*)&packet->payload[offset]);
offset += 4;
/* -1 for ';' */
if((offset >= packet->payload_packet_len) || (len >= packet->payload_packet_len-offset-1))
goto invalid_payload;
/* ssh.kex_algorithms [C/S] */
[1]
strncpy(buf, (const char *)&packet->payload[offset], buf_out_len = len);
buf[buf_out_len++] = ';';
offset += len;
if(offset+sizeof(u_int32_t) >= packet->payload_packet_len)
goto invalid_payload;
/* ssh.server_host_key_algorithms [None] */
[2]
len = ntohl(*(u_int32_t*)&packet->payload[offset]);
if (len > UINT32_MAX - 4 - offset)
goto invalid_payload;
offset += 4 + len;
[3]
if(offset+sizeof(u_int32_t) >= packet->payload_packet_len)
goto invalid_payload;
/* ssh.encryption_algorithms_client_to_server [C] */
[4]
len = ntohl(*(u_int32_t*)&packet->payload[offset]);
[5]
offset += 4;
if(client_hash) {
if((offset >= packet->payload_packet_len) || (len >= packet->payload_packet_len-offset-1))
goto invalid_payload;
[6]
strncpy(&buf[buf_out_len], (const char *)&packet->payload[offset], len);
buf_out_len += len;
buf[buf_out_len++] = ';';
}
if (len > UINT32_MAX - offset)
goto invalid_payload;
offset += len;
To exploit this on a 32-bit system, you’d send a packet with a single string so buf_out_len advances close to the buffer limit. At [2] you supply a len that makes the addition of sizeof(u_int32_t) wrap offset to something like 0xffffffff. The next read then pulls data from payload[offset], which lands within the SSH protocol header rather than your original string—reducing your control and repeatability. If the partially controlled len passes the bounds check at [5], the strncpy at [6] overflows the heap buffer.
The deeper issue is that the patch’s correctness depended on an implicit integer promotion from sizeof(), not an explicit intent. A more portable overflow check casts the intermediate value to the width of the operand and checks whether the result is smaller than the original:
if((u_int32_t)(offset+sizeof(u_int32_t)) < offset || (u_int32_t)(offset+sizeof(u_int32_t)) >= packet->payload_packet_len)
That explicit check on unsigned arithmetic works because the C standard defines unsigned overflow as wrapping modulo UINT32_MAX. But signed overflow is undefined behavior, and some compilers will optimize away such checks entirely. Subtraction, multiplication, and division each introduce their own platform-specific caveats. The most robust approach is to verify operands fall within safe value ranges before doing arithmetic.
For the specific case at hand, splitting the check into two conditions avoids overflow entirely:
/* ensure offset is outside of UINT32_MAX wrapping range */
if (offset > UINT32_MAX - sizeof(u_int32_t))
goto invalid_payload;
/* ensure offset is not beyond packet payload boundaries */
if (offset + sizeof(u_int32_t) >= packet->payload_packet_len)
goto invalid_payload;
Or as a single expression:
if (offset > UINT32_MAX - sizeof(u_int32_t) || offset + sizeof(u_int32_t) >= packet->payload_packet_len)
goto invalid_payload;
The ntop maintainers ultimately reworked concat_hash_string to avoid reaching UINT32_MAX boundaries altogether in a subsequent commit.
Lessons from the lava
The recurring theme is that network-supplied integers behave like hot lava: a very common protocol construct—reading a length and later copying that many bytes—becomes dangerous when the integer isn’t treated with suspicion. Even with a clear understanding of the vulnerability, the original patch missed the exact same flaw in the exact same function because it didn’t account for all target platforms.
For security-critical C code, every integer operation involving remote data deserves scrutiny around integer promotions, conversion rank, and usual arithmetic conversions. The safest pattern is to check operands stay within expected bounds before they are used in any arithmetic, and to re-read any proposed fix one more time—especially when a regression would mean a heap overflow.
