On Sun, Feb 26, 2023 at 10:17:53AM +0100, Claudio Jeker wrote:
> On Sun, Feb 26, 2023 at 09:52:43AM +0100, p...@delphinusdns.org wrote:
> > >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;
> >
> >
>
> While not pretty there is nothing wrong with the current code.
>
> All 4 variables qdcount, ancount, nscount, arcount are only decremented by
> one and always checked for 0. So there is no way any of the 4 values
> become negative. Also the EXTRACT_16BITS() puts a uint16_t into an int
> which never overflows on OpenBSD. int is always 32bit on OpenBSD.
>
> Still it may make sense to apply the diff just to be explicit about the
> count values.
>
> --
> :wq Claudio
Hi Claudio,
I ask you to look closer. Perhaps I didn't explain the problem best.
Let's take variable qdcount:
603 while (qdcount--) {
After this while() qdcount wraps, and that's fine if it isn't reused! But in
the same function below it is tested again (at which point it is negative).
686 if (qdcount--) {
and
690 while (cp < snapend && qdcount--) {
Best Regards,
-peter
