Re: Overflow in banner(1)
On Tue, 30 Nov 1999, Brian Fundakowski Feldman wrote: kind of thing I'd rather have. I like the precomputed one (add all argv[] strlen's and malloc that) better, anyway. ..and as soon as warner reviews my revised patch, it will be committed :-) Kris To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: Overflow in banner(1)
On Wed, 24 Nov 1999, David O'Brien wrote: On Wed, Nov 24, 1999 at 09:58:51AM +0200, John Hay wrote: Well the original line is plain wrong if Brian's patch is being used, because there message is a pointer and the size of a pointer is 4. Yes, yes, yes. Warner and I are *not* that stupid WRT C. We were both commenting on the *original* proposed patch. Geez. Now rather than try to call us stupid, Kris (and only Kris) could say, "well, I've decided to go with a dynamically allocated buffer, so of course I can't use sizeof() any more". Hey, hey, I'm also not stupid, but thanks for implying that :P If I had intended to commit what I whipped up quickly, I would be checking it pretty carefully. It seems I'm catching a lot of flak on not having "audited" what was basically a "proof of concept", showing the kind of thing I'd rather have. I like the precomputed one (add all argv[] strlen's and malloc that) better, anyway. -- -- David([EMAIL PROTECTED]) -- Brian Fundakowski Feldman \ FreeBSD: The Power to Serve! / [EMAIL PROTECTED]`--' To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: Overflow in banner(1)
On Wed, Nov 24, 1999 at 09:58:51AM +0200, John Hay wrote: Well the original line is plain wrong if Brian's patch is being used, because there message is a pointer and the size of a pointer is 4. Yes, yes, yes. Warner and I are *not* that stupid WRT C. We were both commenting on the *original* proposed patch. Geez. Now rather than try to call us stupid, Kris (and only Kris) could say, "well, I've decided to go with a dynamically allocated buffer, so of course I can't use sizeof() any more". -- -- David([EMAIL PROTECTED]) To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: Overflow in banner(1)
I've never done this myself, but I've always been under the impression that sizeof(*buf) would work for dynamically allocated buffers. sizeof() is an operator whose value is determined at compile time. sizeof(*buf) gives the size of what buf points to. This would be `1' if buf were a char*, or `4' if buf were an int* [on the i386]. -- -- David([EMAIL PROTECTED]) To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: Overflow in banner(1)
| sizeof() is an operator whose value is determined at compile time. | sizeof(*buf) gives the size of what buf points to. This would be `1' if | buf were a char*, or `4' if buf were an int* [on the i386]. Ahh, so I've probably seen this concept used only on structures then. -- Dan Moschuk ([EMAIL PROTECTED]) "Cure for global warming: One giant heatsink and dual fans!" To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Overflow in banner(1)
In the spirit of the newly-formed FreeBSD Auditing Project, I present:
% banner `perl -e 'print "a"x2000'`
Segmentation fault(core dumped)
-
The problem is a trivial one. From /usr/src/usr.bin/banner/banner.c:
/*
* banner - prints large signs
* banner [-w#] [-d] [-t] message ...
*/
#define MAXMSG 1024
...
charmessage[MAXMSG];
...
/* Have now read in the data. Next get the message to be printed. */
if (*argv) {
strcpy(message, *argv);
while (*++argv) {
strcat(message, " ");
strcat(message, *argv);
}
nchars = strlen(message);
} else {
Bzzzt! Wrong!
OpenBSD were never vulnerable to this because they seem to use a different
banner(1) than we do. The issue of whether or not this is likely to be a
serious security risk is left as an exercise to the reader :-)
I'll commit this tomorrow (just wanted to get in a 'first post!' :-)..
Kris
Index: banner.c
===
RCS file: /home/ncvs/src/usr.bin/banner/banner.c,v
retrieving revision 1.6
diff -u -r1.6 banner.c
--- banner.c1999/04/19 04:05:25 1.6
+++ banner.c1999/12/23 10:18:50
@@ -1058,15 +1058,15 @@
/* Have now read in the data. Next get the message to be printed. */
if (*argv) {
- strcpy(message, *argv);
+ strncpy(message, *argv, MAXMSG);
while (*++argv) {
- strcat(message, " ");
- strcat(message, *argv);
+ strlcat(message, " ", MAXMSG);
+ strlcat(message, *argv, MAXMSG);
}
nchars = strlen(message);
} else {
fprintf(stderr,"Message: ");
- (void)fgets(message, sizeof(message), stdin);
+ (void)fgets(message, MAXMSG, stdin);
nchars = strlen(message);
message[nchars--] = '\0'; /* get rid of newline */
}
Cthulhu for President! For when you're tired of choosing the _lesser_ of
two evils..
To Unsubscribe: send mail to [EMAIL PROTECTED]
with "unsubscribe freebsd-current" in the body of the message
Re: Overflow in banner(1)
On Tue, Nov 23, 1999 at 09:15:35PM -0800, Kris Kennaway wrote: - (void)fgets(message, sizeof(message), stdin); + (void)fgets(message, MAXMSG, stdin); There is nothing wrong with the original line here. Please don't change things that are fine just to change them. We don't want to ofuscate the fix. -- -- David([EMAIL PROTECTED]) To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: Overflow in banner(1)
On Tue, 23 Nov 1999, David O'Brien wrote: On Tue, Nov 23, 1999 at 09:15:35PM -0800, Kris Kennaway wrote: - (void)fgets(message, sizeof(message), stdin); + (void)fgets(message, MAXMSG, stdin); There is nothing wrong with the original line here. Please don't change things that are fine just to change them. We don't want to ofuscate the fix. Obviously not, but I didn't see the point in making it inconsistent. Kris Cthulhu for President! For when you're tired of choosing the _lesser_ of two evils.. To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: Overflow in banner(1)
- (void)fgets(message, sizeof(message), stdin); + (void)fgets(message, MAXMSG, stdin); There is nothing wrong with the original line here. Please don't change things that are fine just to change them. We don't want to ofuscate Obviously not, but I didn't see the point in making it inconsistent. You could make the "MAXMSG" you added "sizeof(message)" and been consistent. :) -- -- David([EMAIL PROTECTED]) To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: Overflow in banner(1)
In message [EMAIL PROTECTED] "David O'Brien" writes: : On Tue, Nov 23, 1999 at 09:15:35PM -0800, Kris Kennaway wrote: : - (void)fgets(message, sizeof(message), stdin); : + (void)fgets(message, MAXMSG, stdin); : : There is nothing wrong with the original line here. Please don't change : things that are fine just to change them. We don't want to ofuscate the fix. In fact, the original line is safer than the replaced line. It is safer because message's size might change form MAXMSG to MAXBUF or 24. If you hardwire MAXMSG like this, painful experience has shown that you will get burned. Warner To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: Overflow in banner(1)
In message [EMAIL PROTECTED] Kris Kennaway
writes:
: I'll commit this tomorrow (just wanted to get in a 'first post!' :-)..
Please don't. Please use a proper fix instead.
: /* Have now read in the data. Next get the message to be printed. */
: if (*argv) {
: - strcpy(message, *argv);
: + strncpy(message, *argv, MAXMSG);
: while (*++argv) {
: - strcat(message, " ");
: - strcat(message, *argv);
: + strlcat(message, " ", MAXMSG);
: + strlcat(message, *argv, MAXMSG);
Can you precompute the length, malloc the buffer and go from there?
wouldn't that be better?
: }
: nchars = strlen(message);
: } else {
: fprintf(stderr,"Message: ");
: - (void)fgets(message, sizeof(message), stdin);
: + (void)fgets(message, MAXMSG, stdin);
This is bad style. Don't make this change.
: nchars = strlen(message);
: message[nchars--] = '\0'; /* get rid of newline */
: }
Warner
To Unsubscribe: send mail to [EMAIL PROTECTED]
with "unsubscribe freebsd-current" in the body of the message
Re: Overflow in banner(1)
Hmmm, but now that you have changed message to be a pointer, the
sizeof(message) at the end of the patch will return the size of
a pointer which is 4 and probably not what you want. :-)
I think we should be carefull when we make our security fixes so
that we don't introduce new bugs, which was also the problem that
I had the other day with doscmd.
John
--
John Hay -- [EMAIL PROTECTED]
I'd prefer something like this that I've attached. The move over the
years has been away from artificial limits...
--
Brian Fundakowski Feldman \ FreeBSD: The Power to Serve! /
[EMAIL PROTECTED]`--'
Index: banner.c
===
RCS file: /usr2/ncvs/src/usr.bin/banner/banner.c,v
retrieving revision 1.6
diff -u -r1.6 banner.c
--- banner.c 1999/04/19 04:05:25 1.6
+++ banner.c 1999/11/24 05:41:35
@@ -1018,7 +1018,7 @@
};
char line[DWIDTH];
-char message[MAXMSG];
+char *message;
char print[DWIDTH];
int debug, i, j, linen, max, nchars, pc, term, trace, x, y;
int width = DWIDTH; /* -w option: scrunch letters to 80 columns */
@@ -1058,14 +1058,24 @@
/* Have now read in the data. Next get the message to be printed. */
if (*argv) {
- strcpy(message, *argv);
+ message = strdup(*argv);
+ if (message == NULL)
+ err(1, "strdup");
while (*++argv) {
- strcat(message, " ");
- strcat(message, *argv);
+ char *omessage;
+
+ omessage = message;
+ asprintf(message, "%s %s", message, *argv);
+ if (message == NULL)
+ err(1, "asprintf");
+ free(omessage);
}
nchars = strlen(message);
} else {
fprintf(stderr,"Message: ");
+ message = malloc(MAXMSG);
+ if (message == NULL)
+ err(1, "malloc");
(void)fgets(message, sizeof(message), stdin);
nchars = strlen(message);
message[nchars--] = '\0'; /* get rid of newline */
To Unsubscribe: send mail to [EMAIL PROTECTED]
with "unsubscribe freebsd-current" in the body of the message
Re: Overflow in banner(1)
In message [EMAIL PROTECTED] "David O'Brien" writes: : On Tue, Nov 23, 1999 at 09:15:35PM -0800, Kris Kennaway wrote: : - (void)fgets(message, sizeof(message), stdin); : + (void)fgets(message, MAXMSG, stdin); : : There is nothing wrong with the original line here. Please don't change : things that are fine just to change them. We don't want to ofuscate the fix. In fact, the original line is safer than the replaced line. It is safer because message's size might change form MAXMSG to MAXBUF or 24. If you hardwire MAXMSG like this, painful experience has shown that you will get burned. Well the original line is plain wrong if Brian's patch is being used, because there message is a pointer and the size of a pointer is 4. John -- John Hay -- [EMAIL PROTECTED] To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
