Re: possible underflow (wrap) in tcpdump/print-domain.c
> 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
Arghh, you're right! I'm forever shamed :-(. Good call!
Best Regards,
-peter
Re: possible underflow (wrap) in tcpdump/print-domain.c
On 23-02-26 10:29AM, 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(>qdcount);
> > > 584 ancount = EXTRACT_16BITS(>ancount);
> > > 585 nscount = EXTRACT_16BITS(>nscount);
> > > 586 arcount = EXTRACT_16BITS(>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.c24 Jan 2020 22:46:36 - 1.27
> > > +++ print-domain.c26 Feb 2023 08:42:21 -
> > > @@ -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(>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) {
> > > print
Re: possible underflow (wrap) in tcpdump/print-domain.c
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(>qdcount);
> > > 584 ancount = EXTRACT_16BITS(>ancount);
> > > 585 nscount = EXTRACT_16BITS(>nscount);
> > > 586 arcount = EXTRACT_16BITS(>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.c24 Jan 2020 22:46:36 - 1.27
> > > +++ print-domain.c26 Feb 2023 08:42:21 -
> > > @@ -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(>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) {
> > > print
Re: possible underflow (wrap) in tcpdump/print-domain.c
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(>qdcount);
> > 584 ancount = EXTRACT_16BITS(>ancount);
> > 585 nscount = EXTRACT_16BITS(>nscount);
> > 586 arcount = EXTRACT_16BITS(>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 - 1.27
> > +++ print-domain.c 26 Feb 2023 08:42:21 -
> > @@ -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(>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
Re: possible underflow (wrap) in tcpdump/print-domain.c
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(>qdcount);
> 584 ancount = EXTRACT_16BITS(>ancount);
> 585 nscount = EXTRACT_16BITS(>nscount);
> 586 arcount = EXTRACT_16BITS(>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.c24 Jan 2020 22:46:36 - 1.27
> +++ print-domain.c26 Feb 2023 08:42:21 -
> @@ -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(>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 && arc
possible underflow (wrap) in tcpdump/print-domain.c
>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(>qdcount);
584 ancount = EXTRACT_16BITS(>ancount);
585 nscount = EXTRACT_16BITS(>nscount);
586 arcount = EXTRACT_16BITS(>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 - 1.27
+++ print-domain.c 26 Feb 2023 08:42:21 -
@@ -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(>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)
