inn-2.2 inews buffer overflow
Russ Allbery
rra at stanford.edu
Sun Aug 22 06:39:28 UTC 1999
Russ Allbery <rra at Stanford.EDU> writes:
> I'll draft something. I want to go over the rest of inews and make sure
> there aren't any more.
Ugh. inews was clearly overdue for a code audit.
I found a few additional potential exploits. I think that all of the
additional ones that I found would have required a lot more work on the
part of the attacker, and most of them probably weren't possible on most
platforms.
inews makes extensive use of BUFSIZ, which is a stdio constant. If BUFSIZ
< SMBUF, all sorts of bad things could have happened; chances are that
isn't true on any platform INN runs on, but.... That was one whole class
of problems.
Another class of problems would have cropped up if the user had control
over their own GECOS entry (via, for example, chfn). There a few possible
buffer overruns there, although given a sane chfn that would hopefully be
difficult to exploit.
A third class required the user's home directory to be excessively long.
Chances are pretty high that wasn't exploitable, but I checked it just in
case.
A fourth class required user control over inn.conf. I don't remember how
much in the way of environment variables we're honoring these days to
redirect things at a different inn.conf, so I fixed those too.
inews really needs a top-to-bottom rethink, hopefully including yanking
out the header parsing code that's duplicated in two other places in favor
of a standard library routine. But this should do for the time being.
Does anyone here know if inews has any reason at all to be setgid apart
from being able to use the local Unix domain socket for posting? I've
been installing inews without any special permissions on my servers for
years, but I don't make very extensive use of inews. If that's the only
need for it, we may want to consider making installing it setgid an option
and not the default for INN 2.3.
Anyway, the patch I checked in (on top of the earlier one) is attached.
--
Russ Allbery (rra at stanford.edu) <URL:http://www.eyrie.org/~eagle/>
Index: inews.c
===================================================================
RCS file: /dist1/cvs/isc/inn/inn/frontends/inews.c,v
retrieving revision 1.15.2.4
retrieving revision 1.15.2.5
diff -u -r1.15.2.4 -r1.15.2.5
--- inews.c 1999/08/21 04:35:18 1.15.2.4
+++ inews.c 1999/08/22 06:25:10 1.15.2.5
@@ -1,4 +1,4 @@
-/* $Revision: 1.15.2.4 $
+/* $Revision: 1.15.2.5 $
**
** Send an article (prepared by someone on the local site) to the
** master news server.
@@ -342,10 +342,14 @@
*p = '\0';
if (buff[0] == '.' && buff[1] == '\0')
break;
- if (EQn(buff, "Sender:", 7))
- (void)strcpy(remotefrom, TrimSpaces(&buff[7]));
- else if (remotefrom[0] == '\0' && EQn(buff, "From:", 5))
- (void)strcpy(remotefrom, TrimSpaces(&buff[5]));
+ if (EQn(buff, "Sender:", 7)) {
+ strncpy(remotefrom, TrimSpaces(&buff[7]), SMBUF);
+ remotefrom[SMBUF - 1] = '\0';
+ }
+ else if (remotefrom[0] == '\0' && EQn(buff, "From:", 5)) {
+ strncpy(remotefrom, TrimSpaces(&buff[5]), SMBUF);
+ remotefrom[SMBUF - 1] = '\0';
+ }
}
if (remotefrom[0] == '\0') {
if (JustReturn)
@@ -449,7 +453,8 @@
|| EQ(ctrl, "sendsys")
|| EQ(ctrl, "senduuname")
|| EQ(ctrl, "version")) {
- (void)strcpy(name, pwp->pw_name);
+ strncpy(name, pwp->pw_name, SMBUF);
+ name[SMBUF - 1] = '\0';
if (!AnAdministrator(name, (int)pwp->pw_gid)) {
(void)fprintf(stderr,
"Ask your news administrator to do the \"%s\" for you.\n",
@@ -481,10 +486,11 @@
struct passwd *pwp;
char *node;
{
- char buff[BUFSIZ];
char outbuff[SMBUF];
+ char *buff;
char *out;
char *p;
+ int left;
#if !defined(DONT_MUNGE_GETENV)
(void)memset(outbuff, 0, SMBUF);
@@ -497,26 +503,37 @@
#if defined(DONT_MUNGE_GECOS)
- (void)strcpy(outbuff, pwp->pw_gecos);
+ strncpy(outbuff, pwp->pw_gecos, SMBUF);
+ outbuff[SMBUF - 1] = '\0';
#else
+ /* Be very careful here. If we're not, we can potentially overflow our
+ * buffer. Remember that on some Unix systems, the content of the GECOS
+ * field is under (untrusted) user control and we could be setgid. */
p = pwp->pw_gecos;
+ left = SMBUF - 1;
if (*p == '*')
p++;
- for (out = outbuff; *p && !GECOSTERM(*p); p++) {
+ for (out = outbuff; *p && !GECOSTERM(*p) && left; p++) {
if (*p == '&') {
- (void)strcpy(out, pwp->pw_name);
+ strncpy(out, pwp->pw_name, left);
if (CTYPE(islower, *out)
&& (out == outbuff || !isalpha(out[-1])))
*out = toupper(*out);
- while (*out)
+ while (*out) {
out++;
+ left--;
+ }
}
else if (*p == '-'
&& p > pwp->pw_gecos
- && (isdigit(p[-1]) || isspace(p[-1]) || p[-1] == ']'))
+ && (isdigit(p[-1]) || isspace(p[-1]) || p[-1] == ']')) {
out = outbuff;
- else
+ left = SMBUF - 1;
+ }
+ else {
*out++ = *p;
+ left--;
+ }
}
*out = '\0';
#endif /* defined(DONT_MUNGE_GECOS) */
@@ -526,11 +543,16 @@
#endif /* !defined(DONT_MUNGE_GETENV) */
out = TrimSpaces(outbuff);
- if (out[0])
- (void)sprintf(buff, "%s@%s (%s)", pwp->pw_name, node, out);
- else
- (void)sprintf(buff, "%s@%s", pwp->pw_name, node);
- return COPY(buff);
+ if (out[0]) {
+ buff = NEW(char, (strlen(pwp->pw_name) + 1 + strlen(node) + 2
+ + strlen(out) + 2));
+ sprintf(buff, "%s@%s (%s)", pwp->pw_name, node, out);
+ }
+ else {
+ buff = NEW(char, strlen(pwp->pw_name) + 1 + strlen(node) + 1);
+ sprintf(buff, "%s@%s", pwp->pw_name, node);
+ }
+ return buff;
}
@@ -597,6 +619,10 @@
if (HDR(_from) == NULL)
HDR(_from) = FormatUserName(pwp, p);
else {
+ if (strlen(pwp->pw_name) + strlen(p) + 2 > sizeof(buff)) {
+ fprintf(stderr, "Username and host too long\n");
+ QuitServer(1);
+ }
(void)sprintf(buff, "%s@%s", pwp->pw_name, p);
(void)strncpy(from, HDR(_from), SMBUF);
from[SMBUF - 1] = '\0';
@@ -615,7 +641,7 @@
* printf's treat it %02 (two digits wide) .2 (zero-fill to at least
* two digits), while old versions treat it as %02 (zero-fill two
* digits wide) .2 (noise). You might want to check this on your
- * system. */
+ * system. This shouldn't be able to overflow SMBUF... */
if (Now.tzone < 0) {
p = &SIGNS[0];
zone = -Now.tzone;
@@ -773,6 +799,10 @@
/* Open the file. */
*linesp = 0;
+ if (strlen(homedir) > sizeof(buff) - 14) {
+ fprintf(stderr, "Home directory path too long\n");
+ QuitServer(1);
+ }
(void)sprintf(buff, "%s/.signature", homedir);
if ((F = fopen(buff, "r")) == NULL) {
if (errno == ENOENT)
@@ -899,6 +929,10 @@
* in case %s isn't in innconf->mta) and send the headers. */
if (innconf->mta == NULL)
PerrorExit(TRUE, "Can't start mailer - not set");
+ if (strlen(innconf->mta) > sizeof(buff)) {
+ fprintf(stderr, "Mailer command is too long\n");
+ QuitServer(1);
+ }
(void)sprintf(buff, innconf->mta, address);
if ((F = popen(buff, "w")) == NULL)
PerrorExit(TRUE, "Can't start mailer");
@@ -1272,6 +1306,10 @@
if ((p = strchr(buff, '\r')) != NULL)
*p = '\0';
(void)strcpy(SpoolMessage, buff[0] ? buff : NOCONNECT);
+ if (strlen(pwp->pw_dir) > sizeof(buff) - 14) {
+ fprintf(stderr, "Home directory path too long\n");
+ exit(1);
+ }
(void)sprintf(buff, "%s/dead.article", pwp->pw_dir);
deadfile = COPY(buff);
}
More information about the inn-workers
mailing list