Re: [CFR] whois(1) out-of-bound access patch
Peter Pentchev <[EMAIL PROTECTED]> writes:
> As described in PR bin/30968, whois(1) may access invalid data when
> the whois server returns a non-newline-terminated string.
> While it is true that the whois server maintainers should do a better
> job of following standards and such, still the 'be liberal in what
> you accept' mindset might be applied in this case, to fix what is
> ultimately a subtle fgetln(3) use bug :)
>
> Any harm in committing the attached patch? And this - or something
> like this - should be done soon; all FreeBSD whois clients currently
> display weird behavior when querying .biz domains :\
Evil! :) You may want to notify the server administrator, as I tried
using a variety of different whois clients and most of them have
problems with it.
[Over-engineered patch removed.]
Would you please test the attached patch and confirm that it solves
the problem? If it does, I'll commit it today.
Best regards,
Mike Barcroft
whois.20011004.diff
A whois server may return a final line without a new line character.
PR: 30968
Index: whois.c
===
RCS file: /cvs/src/usr.bin/whois/whois.c,v
retrieving revision 1.24
diff -u -r1.24 whois.c
--- whois.c 5 Aug 2001 19:37:12 - 1.24
+++ whois.c 4 Oct 2001 15:57:56 -
@@ -303,7 +303,7 @@
strchr(name, '.') == NULL)
nomatch = 1;
}
- printf("%s\n", buf);
+ printf("%.*s\n", (int)len, buf);
}
/* Do second lookup as needed. */
Re: [CFR] whois(1) out-of-bound access patch
In message <[EMAIL PROTECTED]> so spake Mike Barcroft (mike): > Would you please test the attached patch and confirm that it solves > the problem? If it does, I'll commit it today. I doubt that is sufficient as "buf" is treated as a NUL terminated string in the calls to strstr(). Also note that it is not necessary to copy the buffer each time as in the original patch. You can only get a line w/o a newline as the last line before EOF. - todd To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-net" in the body of the message
Re: [CFR] whois(1) out-of-bound access patch
< said:
> - printf("%s\n", buf);
> + printf("%.*s\n", (int)len, buf);
This is a *much* better patch.
-GAWollman
To Unsubscribe: send mail to [EMAIL PROTECTED]
with "unsubscribe freebsd-net" in the body of the message
Re: [CFR] whois(1) out-of-bound access patch
Todd C. Miller <[EMAIL PROTECTED]> writes: > In message <[EMAIL PROTECTED]> > so spake Mike Barcroft (mike): > > > Would you please test the attached patch and confirm that it solves > > the problem? If it does, I'll commit it today. > > I doubt that is sufficient as "buf" is treated as a NUL terminated > string in the calls to strstr(). Also note that it is not necessary > to copy the buffer each time as in the original patch. You can > only get a line w/o a newline as the last line before EOF. We could always implement strnstr(). I think I prefer it to the malloc(3) the final line kludge. BTW, are you interested in syncing OpenBSD's whois(1) with FreeBSD's at some point? I've added some really useful features, particularly the -c option and recursive IP lookups. Best regards, Mike Barcroft To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-net" in the body of the message
Re: [CFR] whois(1) out-of-bound access patch
On Thu, Oct 04, 2001 at 01:47:10PM -0400, Mike Barcroft wrote:
> Todd C. Miller <[EMAIL PROTECTED]> writes:
> > In message <[EMAIL PROTECTED]>
> > so spake Mike Barcroft (mike):
> >
> > > Would you please test the attached patch and confirm that it solves
> > > the problem? If it does, I'll commit it today.
> >
> > I doubt that is sufficient as "buf" is treated as a NUL terminated
> > string in the calls to strstr(). Also note that it is not necessary
> > to copy the buffer each time as in the original patch. You can
> > only get a line w/o a newline as the last line before EOF.
The buffer is not copied each time, but only when a line w/o a newline
is found. In all other cases, we deal directly with what fgetln(3)
returns.
> We could always implement strnstr(). I think I prefer it to the
> malloc(3) the final line kludge.
strnstr() would not be enough; there are calls to strcspn(), strchr()
and s_asprintf() too, which treat buf as a null-terminated string.
I see no reason to introduce additional processing for *each* input
string, when all we need is to special-case the case of no newline.
The "kludge" is only invoked when a newline-less line is received,
which, as Todd Miller points out, is usually only the last single line.
In all other cases, there is no performance overhead.
On a side note, as Garrett Wollman kindly pointed out in a private
message, the calloc(3) call should probably be replaced by a malloc(3)
and zeroing only the last byte. See the attached revised patch.
G'luck,
Peter
--
You have, of course, just begun reading the sentence that you have just finished
reading.
Index: src/usr.bin/whois/whois.c
===
RCS file: /home/ncvs/src/usr.bin/whois/whois.c,v
retrieving revision 1.24
diff -u -r1.24 whois.c
--- src/usr.bin/whois/whois.c 2001/08/05 19:37:12 1.24
+++ src/usr.bin/whois/whois.c 2001/10/05 11:07:46
@@ -251,7 +251,7 @@
{
FILE *sfi, *sfo;
struct addrinfo *res2;
- char *buf, *nhost, *p;
+ char *abuf, *buf, *nhost, *p;
int i, nomatch, s;
size_t len;
@@ -275,7 +275,16 @@
nhost = NULL;
nomatch = 0;
while ((buf = fgetln(sfi, &len)) != NULL) {
- while (len && isspace(buf[len - 1]))
+ abuf = NULL;
+ if ((len == 0) || !isspace((unsigned char)buf[len - 1])) {
+ abuf = malloc(len + 1);
+ if (abuf == NULL)
+ err(1, "reallocating");
+ memcpy(abuf, buf, len);
+ abuf[len] = '\0';
+ buf = abuf;
+ }
+ while (len && isspace((unsigned char)buf[len - 1]))
buf[--len] = '\0';
if ((flags & WHOIS_RECURSE) && nhost == NULL) {
@@ -304,6 +313,7 @@
nomatch = 1;
}
printf("%s\n", buf);
+ free(abuf);
}
/* Do second lookup as needed. */
To Unsubscribe: send mail to [EMAIL PROTECTED]
with "unsubscribe freebsd-net" in the body of the message
Re: [CFR] whois(1) out-of-bound access patch
On Thu, Oct 04, 2001 at 01:02:56PM -0400, Garrett Wollman wrote:
> < said:
>
> > - printf("%s\n", buf);
> > + printf("%.*s\n", (int)len, buf);
>
> This is a *much* better patch.
..yet it needs more work: strstr() and strcspn() are used on
a non-null-terminated string. And even if those are fixed,
additional work is done for each input line, instead of only for
the lines that actually need it (at most one per session).
G'luck,
Peter
--
This sentence contains exactly threee erors.
To Unsubscribe: send mail to [EMAIL PROTECTED]
with "unsubscribe freebsd-net" in the body of the message
Re: [CFR] whois(1) out-of-bound access patch
Andrey A. Chernov <[EMAIL PROTECTED]> writes:
> On Thu, Oct 04, 2001 at 12:16:40 +0300, Peter Pentchev wrote:
> > + if ((len == 0) || !isspace(buf[len - 1])) {
>
> Must be isspace((unsigned char))
Why and where can I find documentation about this?
Best regards,
Mike Barcroft
To Unsubscribe: send mail to [EMAIL PROTECTED]
with "unsubscribe freebsd-net" in the body of the message
Re: [CFR] whois(1) out-of-bound access patch
Mike Barcroft <[EMAIL PROTECTED]> writes:
> Andrey A. Chernov <[EMAIL PROTECTED]> writes:
> > On Thu, Oct 04, 2001 at 12:16:40 +0300, Peter Pentchev wrote:
> > > + if ((len == 0) || !isspace(buf[len - 1])) {
> > Must be isspace((unsigned char))
> Why and where can I find documentation about this?
If buf[len - 1] is a char (which is signed), non-ASCII characters will
be sign-extended unless you cast to unsigned char. It so happens that
it doesn't make any difference for isspace() because in the character
sets we use, no space characters have the high bit set, but strictly
speaking you still need the cast.
DES
--
Dag-Erling Smorgrav - [EMAIL PROTECTED]
To Unsubscribe: send mail to [EMAIL PROTECTED]
with "unsubscribe freebsd-net" in the body of the message
Re: [CFR] whois(1) out-of-bound access patch
On Sun, Oct 07, 2001 at 13:37:16 -0400, Mike Barcroft wrote: > > > > Must be isspace((unsigned char)) > > Why and where can I find documentation about this? isspace(3) -- Andrey A. Chernov http://ache.pp.ru/ To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-net" in the body of the message
Re: [CFR] whois(1) out-of-bound access patch
On Thu, Oct 04, 2001 at 12:16:40 +0300, Peter Pentchev wrote:
> + if ((len == 0) || !isspace(buf[len - 1])) {
Must be isspace((unsigned char))
--
Andrey A. Chernov
http://ache.pp.ru/
To Unsubscribe: send mail to [EMAIL PROTECTED]
with "unsubscribe freebsd-net" in the body of the message
Re: [CFR] whois(1) out-of-bound access patch
On Thu, Oct 04, 2001 at 12:16:40 +0300, Peter Pentchev wrote:
> + abuf = calloc(1, len + 1);
> + if (abuf == NULL) {
> + errno = ENOMEM;
> + err(1, "reallocating");
> + }
To overwrite errno set by calloc() is wrong.
--
Andrey A. Chernov
http://ache.pp.ru/
To Unsubscribe: send mail to [EMAIL PROTECTED]
with "unsubscribe freebsd-net" in the body of the message
Re: [CFR] whois(1) out-of-bound access patch
On Thu, Oct 04, 2001 at 01:28:02PM +0400, Andrey A. Chernov wrote:
> On Thu, Oct 04, 2001 at 12:16:40 +0300, Peter Pentchev wrote:
> > + if ((len == 0) || !isspace(buf[len - 1])) {
>
> Must be isspace((unsigned char))
On Thu, Oct 04, 2001 at 01:30:42PM +0400, Andrey A. Chernov wrote:
> On Thu, Oct 04, 2001 at 12:16:40 +0300, Peter Pentchev wrote:
> > + abuf = calloc(1, len + 1);
> > + if (abuf == NULL) {
> > + errno = ENOMEM;
> > + err(1, "reallocating");
> > + }
>
> To overwrite errno set by calloc() is wrong.
Oops to both :\
OK, here's an updated patch.
G'luck,
Peter
--
If the meanings of 'true' and 'false' were switched, then this sentence wouldn't be
false.
Index: src/usr.bin/whois/whois.c
===
RCS file: /home/ncvs/src/usr.bin/whois/whois.c,v
retrieving revision 1.24
diff -u -r1.24 whois.c
--- src/usr.bin/whois/whois.c 2001/08/05 19:37:12 1.24
+++ src/usr.bin/whois/whois.c 2001/10/04 14:39:24
@@ -251,7 +251,7 @@
{
FILE *sfi, *sfo;
struct addrinfo *res2;
- char *buf, *nhost, *p;
+ char *abuf, *buf, *nhost, *p;
int i, nomatch, s;
size_t len;
@@ -275,7 +275,15 @@
nhost = NULL;
nomatch = 0;
while ((buf = fgetln(sfi, &len)) != NULL) {
- while (len && isspace(buf[len - 1]))
+ abuf = NULL;
+ if ((len == 0) || !isspace((unsigned char)buf[len - 1])) {
+ abuf = calloc(1, len + 1);
+ if (abuf == NULL)
+ err(1, "reallocating");
+ memcpy(abuf, buf, len);
+ buf = abuf;
+ }
+ while (len && isspace((unsigned char)buf[len - 1]))
buf[--len] = '\0';
if ((flags & WHOIS_RECURSE) && nhost == NULL) {
@@ -304,6 +312,7 @@
nomatch = 1;
}
printf("%s\n", buf);
+ free(abuf);
}
/* Do second lookup as needed. */
To Unsubscribe: send mail to [EMAIL PROTECTED]
with "unsubscribe freebsd-net" in the body of the message
Final Patch for Review (was Re: [CFR] whois(1) out-of-bound access patch)
Todd C. Miller <[EMAIL PROTECTED]> writes:
> In message <[EMAIL PROTECTED]>
> so spake Mike Barcroft (mike):
>
> > Would you please test the attached patch and confirm that it solves
> > the problem? If it does, I'll commit it today.
>
> I doubt that is sufficient as "buf" is treated as a NUL terminated
> string in the calls to strstr(). Also note that it is not necessary
> to copy the buffer each time as in the original patch. You can
> only get a line w/o a newline as the last line before EOF.
After some discussion off the mailing list with Peter, I've come up
with a total solution that is fairly elegant, but requires the new
libc function strnstr(3) I posted earlier. Most of the patch is a
cleanup of the current code.
I'd appreciate reviews of this. I plan on committing it in a few
days. The patch is at the end of this message and also at:
http://people.FreeBSD.org/~mike/patches/whois.20011007.diff
Best regards,
Mike Barcroft
--
whois.20011007.diff
o Treat a buffer as a non-NUL terminated string, because the whois
server may not return a new line character on the final line.
o Remove the whois.networksolutions.com fallback code, which is no
longer needed.
o Instead of determining a hostname by terminating it when we see
whitespace, only allow hostname characters and terminate the string
when it's not such a character.
o Add a small optimization in a for loop.
PR: 30968
MFC after: 4 days
Index: whois.c
===
RCS file: /cvs/src/usr.bin/whois/whois.c,v
retrieving revision 1.24
diff -u -r1.24 whois.c
--- whois.c 5 Aug 2001 19:37:12 - 1.24
+++ whois.c 7 Oct 2001 21:38:04 -
@@ -71,11 +71,11 @@
#defineSNICHOST"whois.6bone.net"
#defineDEFAULT_PORT"whois"
#defineWHOIS_SERVER_ID "Whois Server: "
-#defineNO_MATCH_ID "No match for \""
#define WHOIS_RECURSE 0x01
-#define WHOIS_INIC_FALLBACK0x02
-#define WHOIS_QUICK0x04
+#define WHOIS_QUICK0x02
+
+#define ishost(h) (isalnum((unsigned char)h) || h == '.' || h == '-')
const char *ip_whois[] = { RNICHOST, PNICHOST, NULL };
const char *port = DEFAULT_PORT;
@@ -164,7 +164,7 @@
use_qnichost = 1;
host = NICHOST;
if (!(flags & WHOIS_QUICK))
- flags |= WHOIS_INIC_FALLBACK | WHOIS_RECURSE;
+ flags |= WHOIS_RECURSE;
}
while (argc--) {
if (country != NULL) {
@@ -251,8 +251,8 @@
{
FILE *sfi, *sfo;
struct addrinfo *res2;
- char *buf, *nhost, *p;
- int i, nomatch, s;
+ char *buf, *host, *nhost, *p;
+ int i, s;
size_t len;
for (; res; res = res->ai_next) {
@@ -273,45 +273,34 @@
fprintf(sfo, "%s\r\n", name);
fflush(sfo);
nhost = NULL;
- nomatch = 0;
while ((buf = fgetln(sfi, &len)) != NULL) {
- while (len && isspace(buf[len - 1]))
+ while (len > 0 && isspace((unsigned char)buf[len - 1]))
buf[--len] = '\0';
+ printf("%.*s\n", (int)len, buf);
if ((flags & WHOIS_RECURSE) && nhost == NULL) {
- p = strstr(buf, WHOIS_SERVER_ID);
- if (p != NULL) {
- p += sizeof(WHOIS_SERVER_ID) - 1;
- if ((len = strcspn(p, " \t\n\r")) != 0) {
- s_asprintf(&nhost, "%s", p);
+ host = strnstr(buf, WHOIS_SERVER_ID, len);
+ if (host != NULL) {
+ host += sizeof(WHOIS_SERVER_ID) - 1;
+ for (p = host; p < buf + len; p++) {
+ if (!ishost(*p)) {
+ *p = '\0';
+ break;
+ }
}
+ s_asprintf(&nhost, "%.*s",
+(int)(buf + len - host), host);
} else {
for (i = 0; ip_whois[i] != NULL; i++) {
- if (strstr(buf, ip_whois[i]) == NULL)
+ if (strnstr(buf, ip_whois[i], len) ==
+ NULL)
continue;
s_asprintf(&nhost, "%s", ip_whois[i]);
+ break;
}
}
}
-
- if ((flags & WHOIS_INIC_FALLBACK) && nhost == NULL &&
- !nomatch && (p = strstr(buf, NO_MATCH_ID)
Re: Final Patch for Review (was Re: [CFR] whois(1) out-of-bound access patch)
On Sunday, October 07, 2001, Mike Barcroft wrote:
> for (i = 0; ip_whois[i] != NULL; i++) {
> - if (strstr(buf, ip_whois[i]) == NULL)
> + if (strnstr(buf, ip_whois[i], len) ==
> + NULL)
> continue;
> s_asprintf(&nhost, "%s", ip_whois[i]);
> + break;
Should be
for (i = 0; ip_whois[i] != NULL; i++) {
if (strnstr(buf, ip_whois[i]) != NULL) {
s_asprintf(&nhost, "%s", ip_whois[i]);
break;
}
}
for simplicity's sake.
--
+---+--+
| Chris Costello| To iterate is human; to recurse, divine. |
| [EMAIL PROTECTED] | |
+---+--+
To Unsubscribe: send mail to [EMAIL PROTECTED]
with "unsubscribe freebsd-net" in the body of the message
