>Synopsis: possible underflow (wrap) in tcpdump/print-domain.c
>Category: system
>Environment:
System : OpenBSD 7.2
Details : OpenBSD 7.2 (GENERIC.MP) #2: Thu Nov 24 23:53:03 MST 2022
r...@syspatch-72-arm64.openbsd.org:/usr/src/sys/arch/arm64/compile/GENERIC.MP
Architecture: OpenBSD.arm64
Machine : arm64
>Description:
While code reading and giving signedness scrutiny in print-domain.c I
noticed there being opportunity for variables to underflow into the negative and
thus being positive in testing. Here is a code-snippet of function
ns_print():
572 void
573 ns_print(const u_char *bp, u_int length, int is_mdns)
574 {
575 const HEADER *np;
576 int qdcount, ancount, nscount, arcount;
577 const u_char *cp;
578 u_int16_t b2;
579
580 np = (const HEADER *)bp;
581 TCHECK(*np);
582 /* get the byte-order right */
583 qdcount = EXTRACT_16BITS(&np->qdcount);
584 ancount = EXTRACT_16BITS(&np->ancount);
585 nscount = EXTRACT_16BITS(&np->nscount);
586 arcount = EXTRACT_16BITS(&np->arcount);
notice qdcount,ancount,nscount and arcount can wrap into the negative and
that is being tested like so:
603 while (qdcount--) {
But really what's meant is qdcount-- > 0 as there is no negatives in here. I
personally don't feel comfortable having tcpdump go deeper into this and
print-domain.c is complex(!!) I can only guess that with this it's possible
to print domain names that don't exist with this. I just don't feel all that
comfortable with this, it gives me a bad feeling.
>How-To-Repeat:
code-reading.
>Fix:
? tcpdump.patch
Index: print-domain.c
===================================================================
RCS file: /cvs/src/usr.sbin/tcpdump/print-domain.c,v
retrieving revision 1.27
diff -u -p -u -r1.27 print-domain.c
--- print-domain.c 24 Jan 2020 22:46:36 -0000 1.27
+++ print-domain.c 26 Feb 2023 08:42:21 -0000
@@ -600,7 +600,7 @@ ns_print(const u_char *bp, u_int length,
printf(" [%dq]", qdcount);
/* Print QUESTION section on -vv */
cp = (const u_char *)(np + 1);
- while (qdcount--) {
+ while (qdcount-- > 0) {
if (qdcount < EXTRACT_16BITS(&np->qdcount) - 1)
putchar(',');
if (vflag > 1) {
@@ -614,10 +614,10 @@ ns_print(const u_char *bp, u_int length,
}
}
printf(" %d/%d/%d", ancount, nscount, arcount);
- if (ancount--) {
+ if (ancount-- > 0) {
if ((cp = ns_rprint(cp, bp, is_mdns)) == NULL)
goto trunc;
- while (cp < snapend && ancount--) {
+ while (cp < snapend && ancount-- > 0) {
putchar(',');
if ((cp = ns_rprint(cp, bp, is_mdns)) == NULL)
goto trunc;
@@ -627,11 +627,11 @@ ns_print(const u_char *bp, u_int length,
goto trunc;
/* Print NS and AR sections on -vv */
if (vflag > 1) {
- if (cp < snapend && nscount--) {
+ if (cp < snapend && nscount-- > 0) {
printf(" ns:");
if ((cp = ns_rprint(cp, bp, is_mdns)) == NULL)
goto trunc;
- while (cp < snapend && nscount--) {
+ while (cp < snapend && nscount-- > 0) {
putchar(',');
if ((cp = ns_rprint(cp, bp, is_mdns))
== NULL)
goto trunc;
@@ -639,11 +639,11 @@ ns_print(const u_char *bp, u_int length,
}
if (nscount > 0)
goto trunc;
- if (cp < snapend && arcount--) {
+ if (cp < snapend && arcount-- > 0) {
printf(" ar:");
if ((cp = ns_rprint(cp, bp, is_mdns)) == NULL)
goto trunc;
- while (cp < snapend && arcount--) {
+ while (cp < snapend && arcount-- > 0) {
putchar(',');
if ((cp = ns_rprint(cp, bp, is_mdns))
== NULL)
goto trunc;
@@ -666,28 +666,28 @@ ns_print(const u_char *bp, u_int length,
printf(" [b2&3=0x%x]", b2);
if (DNS_OPCODE(np) == IQUERY) {
- if (qdcount)
+ if (qdcount > 0)
printf(" [%dq]", qdcount);
if (ancount != 1)
printf(" [%da]", ancount);
}
else {
- if (ancount)
+ if (ancount > 0)
printf(" [%da]", ancount);
if (qdcount != 1)
printf(" [%dq]", qdcount);
}
- if (nscount)
+ if (nscount > 0)
printf(" [%dn]", nscount);
- if (arcount)
+ if (arcount > 0)
printf(" [%dau]", arcount);
cp = (const u_char *)(np + 1);
- if (qdcount--) {
+ if (qdcount-- > 0) {
cp = ns_qprint(cp, (const u_char *)np, is_mdns);
if (!cp)
goto trunc;
- while (cp < snapend && qdcount--) {
+ while (cp < snapend && qdcount-- > 0) {
cp = ns_qprint((const u_char *)cp,
(const u_char *)np,
is_mdns);
@@ -700,10 +700,10 @@ ns_print(const u_char *bp, u_int length,
/* Print remaining sections on -vv */
if (vflag > 1) {
- if (ancount--) {
+ if (ancount-- > 0) {
if ((cp = ns_rprint(cp, bp, is_mdns)) == NULL)
goto trunc;
- while (cp < snapend && ancount--) {
+ while (cp < snapend && ancount-- > 0) {
putchar(',');
if ((cp = ns_rprint(cp, bp, is_mdns))
== NULL)
goto trunc;
@@ -711,11 +711,11 @@ ns_print(const u_char *bp, u_int length,
}
if (ancount > 0)
goto trunc;
- if (cp < snapend && nscount--) {
+ if (cp < snapend && nscount-- > 0) {
printf(" ns:");
if ((cp = ns_rprint(cp, bp, is_mdns)) == NULL)
goto trunc;
- while (nscount-- && cp < snapend) {
+ while (nscount-- > 0 && cp < snapend) {
putchar(',');
if ((cp = ns_rprint(cp, bp, is_mdns))
== NULL)
goto trunc;
@@ -723,11 +723,11 @@ ns_print(const u_char *bp, u_int length,
}
if (nscount > 0)
goto trunc;
- if (cp < snapend && arcount--) {
+ if (cp < snapend && arcount-- > 0) {
printf(" ar:");
if ((cp = ns_rprint(cp, bp, is_mdns)) == NULL)
goto trunc;
- while (cp < snapend && arcount--) {
+ while (cp < snapend && arcount-- > 0) {
putchar(',');
if ((cp = ns_rprint(cp, bp, is_mdns))
== NULL)
goto trunc;
dmesg:
no need to send my dmesg here 100x.
