Subject: Re: bin/34658: [dM] identd truncates queries to first segment
To: None <peter@NetBSD.org, gnats-admin@netbsd.org, netbsd-bugs@netbsd.org,>
From: der Mouse <mouse@Rodents.Montreal.QC.CA>
List: netbsd-bugs
Date: 09/29/2006 15:15:04
The following reply was made to PR bin/34658; it has been noted by GNATS.
From: der Mouse <mouse@Rodents.Montreal.QC.CA>
To: gnats-bugs@NetBSD.org
Cc:
Subject: Re: bin/34658: [dM] identd truncates queries to first segment
Date: Fri, 29 Sep 2006 11:07:03 -0400 (EDT)
>> + while (1) {
>> + if ((n = recv(fd, &buf[qlen], sizeof(buf)-qlen, 0)) < 0) {
>> + fatal("recv");
>> + } else if (n == 0) {
> [...]
>> + }
> [...]
>> + qlen += n;
>> + if ( (qlen >= 2) &&
>> + (buf[qlen-2] == '\r') &&
>> + (buf[qlen-1] == '\n') )
>> + break;
>> }
> This looks like asking for a buffer overflow to me. There's no
> protection that prevents qlen to exceed "sizeof buf".
For that to happen, recv() would have to return a value greater than
its third argument. I was under the impression this was "impossible".
It'd be easy enough to add "if (qlen+n > sizeof(buf)) {...}" as a guard
if it's not impossible.
Come to think of it, there should be a test to see if buf is full, too.
+ while (1) {
+ if (qlen >= sizeof(buf)) {
+ /* buf filled - ridiculously large query */
+ exit(0);
+ }
+ if ((n = recv(fd, &buf[qlen], sizeof(buf)-qlen, 0)) < 0) {
/~\ The ASCII der Mouse
\ / Ribbon Campaign
X Against HTML mouse@rodents.montreal.qc.ca
/ \ Email! 7D C8 61 52 5D E7 2D 39 4E F1 31 3E E8 B3 27 4B