Date: Sun, 2 Mar 2008 14:35:41 +0300 From: Eygene Ryabinkin <rea-fbsd@codelabs.ru> To: Dan Lukes <dan@obluda.cz> Cc: freebsd-security@freebsd.org, security-officer@FreeBSD.org, secteam@FreeBSD.org, sipherr@gmail.com Subject: Re: *BSD user-ppp local root (when conditions permit) Message-ID: <qQhF1AR4cshMP%2BLKcqnBKpfZW9Y@nE9n69L2PrcQKa%2Be6OgU6kZtlVg> In-Reply-To: <KLpb4j3%2BLD85o3V0gHGybAUP%2Bwo@nE9n69L2PrcQKa%2Be6OgU6kZtlVg> References: <20080229163903.3680.qmail@securityfocus.com> <eJwztaR4hgj0LBOZtN1f3kC2qd8@49l6neKHPg6j4SHeejH198Klzys> <47C9F951.3090408@obluda.cz> <KLpb4j3%2BLD85o3V0gHGybAUP%2Bwo@nE9n69L2PrcQKa%2Be6OgU6kZtlVg>
next in thread | previous in thread | raw e-mail | index | archive | help
Me again.
Sun, Mar 02, 2008 at 08:59:53AM +0300, Eygene Ryabinkin wrote:
> >> Could you please test the following rough patch
> >
> > It seems you are going to cut of part of line silently.
> >
> > IMHO - the line shall be rejected as invalid at all or warning needs to be
> > issued at least ...
>
> Yes, I will add the neccessary statements. But first I want to
> verify that the exploitation path is not available anymore.
OK, here we go: the above patch seem to fix all issues with the
InterpretArg function and it properly warns the user that the
expansion was failed due to the lack of the free buffer space.
But if someone will ask me, then I am in mood to check the rest of
the ppp's code: maybe other issues will arise. But it won't be
very quick.
> > Someone may create so long line (unintentionally), it will not work for him
> > with no hint why - it's not so polite ...
>
> May be the buffer should even be dynamically resized -- will look
> into it.
Did not messed with this: too long for now.
Please, test the new patch and report back in any case.
-----
>From dd38bd346faf42a528f32e0f3fb01ad1dbd95b4f Mon Sep 17 00:00:00 2001
From: Eygene Ryabinkin <rea-fbsd@codelabs.ru>
Date: Sun, 2 Mar 2008 10:26:48 +0300
Subject: [PATCH] Fix buffer overflow in the InterpretArg code.
No destination buffer boundary checks were done, so tilde and
environment variable expansions could lead to the buffer overflows.
Ordinary strings were not generally affected, since the length of
the input (uninterpreted) buffer is at most equal to the length of
the destination (interpreted) buffer.
Signed-off-by: Eygene Ryabinkin <rea-fbsd@codelabs.ru>
---
usr.sbin/ppp/command.c | 5 ++++-
usr.sbin/ppp/systems.c | 41 ++++++++++++++++++++++++++++++++++++++++-
2 files changed, 44 insertions(+), 2 deletions(-)
diff --git a/usr.sbin/ppp/command.c b/usr.sbin/ppp/command.c
index ed4866c..5f90888 100644
--- a/usr.sbin/ppp/command.c
+++ b/usr.sbin/ppp/command.c
@@ -1132,7 +1132,10 @@ command_Expand_Interpret(char *buff, int nb, char *argv[MAXARGS], int offset)
{
char buff2[LINE_LEN-offset];
- InterpretArg(buff, buff2);
+ if (InterpretArg(buff, buff2) == NULL) {
+ log_Printf(LogWARN, "Failed to expand command '%s': too long for the destination buffer\n", buff);
+ return -1;
+ }
strncpy(buff, buff2, LINE_LEN - offset - 1);
buff[LINE_LEN - offset - 1] = '\0';
diff --git a/usr.sbin/ppp/systems.c b/usr.sbin/ppp/systems.c
index 77f06a1..1025e02 100644
--- a/usr.sbin/ppp/systems.c
+++ b/usr.sbin/ppp/systems.c
@@ -64,7 +64,14 @@ CloseSecret(FILE *fp)
fclose(fp);
}
-/* Move string from ``from'' to ``to'', interpreting ``~'' and $.... */
+/*
+ * Move string from ``from'' to ``to'', interpreting ``~'' and $....
+ * Returns NULL is string expansion failed due to the lack of
+ * free space.
+ *
+ * NB: destination buffer size is hardcoded, so we rely it to be
+ * no less then LINE_LEN characters.
+ */
const char *
InterpretArg(const char *from, char *to)
{
@@ -82,6 +89,10 @@ InterpretArg(const char *from, char *to)
from++;
while (*from != '\0') {
+ if (to >= endto) {
+ *endto = '\0';
+ return NULL;
+ }
switch (*from) {
case '"':
instring = !instring;
@@ -97,6 +108,10 @@ InterpretArg(const char *from, char *to)
*to++ = '\\'; /* Pass the escapes on, maybe skipping \# */
break;
}
+ if (to >= endto) {
+ *endto = '\0';
+ return NULL;
+ }
*to++ = *from++;
break;
case '$':
@@ -127,9 +142,17 @@ InterpretArg(const char *from, char *to)
*ptr++ = *from;
*ptr = '\0';
}
+ if (to >= endto) {
+ *endto = '\0';
+ return NULL;
+ }
if (*to == '\0')
*to++ = '$';
else if ((env = getenv(to)) != NULL) {
+ if ((int)strlen(env) >= endto - to) {
+ *endto = '\0';
+ return NULL;
+ }
strncpy(to, env, endto - to);
*endto = '\0';
to += strlen(to);
@@ -142,13 +165,25 @@ InterpretArg(const char *from, char *to)
if (len == 0)
pwd = getpwuid(ID0realuid());
else {
+ if (to + len >= endto) {
+ *to = '\0';
+ return NULL;
+ }
strncpy(to, from, len);
to[len] = '\0';
pwd = getpwnam(to);
}
+ if (to >= endto) {
+ *endto = '\0';
+ return NULL;
+ }
if (pwd == NULL)
*to++ = '~';
else {
+ if ((int)strlen(pwd->pw_dir) >= endto - to) {
+ *endto = '\0';
+ return NULL;
+ }
strncpy(to, pwd->pw_dir, endto - to);
*endto = '\0';
to += strlen(to);
@@ -185,6 +220,10 @@ DecodeCtrlCommand(char *line, char *arg)
if (!strncasecmp(line, "include", 7) && issep(line[7])) {
end = InterpretArg(line+8, arg);
+ if (end == NULL) {
+ log_Printf(LogWARN, "Failed to expand command '%s': too long for the destination buffer\n", line);
+ return CTRL_UNKNOWN;
+ }
if (*end && *end != '#')
log_Printf(LogWARN, "usage: !include filename\n");
else
--
1.5.3.8
-----
Thanks!
--
Eygene
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?qQhF1AR4cshMP%2BLKcqnBKpfZW9Y>