On Sun, Feb 26, 2023 at 10:29:34AM +0100, Peter J. Philipp wrote:
> 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--) {
>
>
You also need to have a closer look.
Line 603 is in
if (DNS_QR(np)) {
Line 686 is in the corresponding else block. So there is no way to get
from 603 into 686 and 690.
--
:wq Claudio
