Refactor the replay code to fix the reported vulnerability -- simply use mmap()

instead of a buffer.

Bump PORTREVISION.

While here, fix warnings -- well enough for gcc48 to be happy with ``-Wall -W''.

Approved by:	maintainer timeout (since February).
This commit is contained in:
Mikhail Teterin 2013-05-21 16:29:19 +00:00
parent 7377997f9c
commit a4d51a600a
Notes: svn2git 2021-03-31 03:12:20 +00:00
svn path=/head/; revision=318714
7 changed files with 266 additions and 44 deletions

View file

@ -7,7 +7,7 @@
PORTNAME= sudosh3 PORTNAME= sudosh3
PORTVERSION= 3.2.0 PORTVERSION= 3.2.0
PORTREVISION= 2 PORTREVISION= 3
CATEGORIES= security CATEGORIES= security
MASTER_SITES= SF/${PORTNAME} MASTER_SITES= SF/${PORTNAME}
@ -15,12 +15,11 @@ MAINTAINER= cy@FreeBSD.org
COMMENT= Third version of the sudo shell COMMENT= Third version of the sudo shell
CONFLICTS= sudosh-* CONFLICTS= sudosh-*
FORBIDDEN= Secunia Advisory SA38292, ISS X-Force sudosh-replay-bo (55903), replay() function buffer overflow.
RUN_DEPENDS= sudo:${PORTSDIR}/security/sudo RUN_DEPENDS= sudo:${PORTSDIR}/security/sudo
GNU_CONFIGURE= yes GNU_CONFIGURE= yes
EXTRACT_AFTER_ARGS= |${TAR} -xpf - --exclude 'getopt.*'
CONFIGURE_ARGS= --bindir="${PREFIX}/bin" CONFIGURE_ARGS= --bindir="${PREFIX}/bin"
CONFIGURE_ARGS+= --sysconfdir="${PREFIX}/etc" CONFIGURE_ARGS+= --sysconfdir="${PREFIX}/etc"
CONFIGURE_ARGS+= --program-transform-name='' CONFIGURE_ARGS+= --program-transform-name=''

View file

@ -1,11 +0,0 @@
--- src/getopt.c.orig 2007-12-21 13:03:26.000000000 -0800
+++ src/getopt.c 2008-09-26 13:45:11.958473185 -0700
@@ -195,6 +195,8 @@
/* gcc with -traditional declares the built-in strlen to return int,
and has done so at least since version 2.4.5. -- rms. */
extern int strlen(const char *);
+#else
+#include <string.h>
#endif /* not __STDC__ */
#endif /* __GNUC__ */

View file

@ -1,6 +1,11 @@
--- src/Makefile.in.orig 2008-02-22 13:11:02.000000000 -0800 --- src/Makefile.in 2008-02-22 16:11:02.000000000 -0500
+++ src/Makefile.in 2010-01-14 20:56:27.381892777 -0800 +++ src/Makefile.in 2013-02-12 16:16:50.000000000 -0500
@@ -55,7 +55,7 @@ @@ -51,11 +51,11 @@
string.$(OBJEXT) util.$(OBJEXT)
sudosh_OBJECTS = $(am_sudosh_OBJECTS)
sudosh_LDADD = $(LDADD)
-am_sudosh_replay_OBJECTS = replay.$(OBJEXT) getopt.$(OBJEXT) \
+am_sudosh_replay_OBJECTS = replay.$(OBJEXT) \
string.$(OBJEXT) parse.$(OBJEXT) util.$(OBJEXT) string.$(OBJEXT) parse.$(OBJEXT) util.$(OBJEXT)
sudosh_replay_OBJECTS = $(am_sudosh_replay_OBJECTS) sudosh_replay_OBJECTS = $(am_sudosh_replay_OBJECTS)
sudosh_replay_LDADD = $(LDADD) sudosh_replay_LDADD = $(LDADD)
@ -9,15 +14,29 @@
depcomp = $(SHELL) $(top_srcdir)/depcomp depcomp = $(SHELL) $(top_srcdir)/depcomp
am__depfiles_maybe = depfiles am__depfiles_maybe = depfiles
COMPILE = $(CC) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) $(AM_CPPFLAGS) \ COMPILE = $(CC) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) $(AM_CPPFLAGS) \
@@ -165,6 +165,7 @@ @@ -165,10 +165,11 @@
target_cpu = @target_cpu@ target_cpu = @target_cpu@
target_os = @target_os@ target_os = @target_os@
target_vendor = @target_vendor@ target_vendor = @target_vendor@
+top_build_prefix = @top_build_prefix@ +top_build_prefix = @top_build_prefix@
top_builddir = @top_builddir@ top_builddir = @top_builddir@
top_srcdir = @top_srcdir@ top_srcdir = @top_srcdir@
sudosh_SOURCES = sudosh.c rand.c parse.c string.c getopt.h struct.h super.h util.c -sudosh_SOURCES = sudosh.c rand.c parse.c string.c getopt.h struct.h super.h util.c
@@ -268,8 +269,8 @@ -sudosh_replay_SOURCES = replay.c getopt.c getopt.h string.c parse.c util.c
+sudosh_SOURCES = sudosh.c rand.c parse.c string.c struct.h super.h util.c
+sudosh_replay_SOURCES = replay.c string.c parse.c util.c
EXTRA_DIST = sudosh.conf
all: all-am
@@ -241,7 +242,6 @@
distclean-compile:
-rm -f *.tab.c
-@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/getopt.Po@am__quote@
@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/parse.Po@am__quote@
@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/rand.Po@am__quote@
@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/replay.Po@am__quote@
@@ -268,8 +268,8 @@
unique=`for i in $$list; do \ unique=`for i in $$list; do \
if test -f "$$i"; then echo $$i; else echo $(srcdir)/$$i; fi; \ if test -f "$$i"; then echo $$i; else echo $(srcdir)/$$i; fi; \
done | \ done | \
@ -28,7 +47,7 @@
mkid -fID $$unique mkid -fID $$unique
tags: TAGS tags: TAGS
@@ -281,8 +282,8 @@ @@ -281,8 +281,8 @@
unique=`for i in $$list; do \ unique=`for i in $$list; do \
if test -f "$$i"; then echo $$i; else echo $(srcdir)/$$i; fi; \ if test -f "$$i"; then echo $$i; else echo $(srcdir)/$$i; fi; \
done | \ done | \
@ -39,7 +58,7 @@
if test -z "$(ETAGS_ARGS)$$tags$$unique"; then :; else \ if test -z "$(ETAGS_ARGS)$$tags$$unique"; then :; else \
test -n "$$unique" || unique=$$empty_fix; \ test -n "$$unique" || unique=$$empty_fix; \
$(ETAGS) $(ETAGSFLAGS) $(AM_ETAGSFLAGS) $(ETAGS_ARGS) \ $(ETAGS) $(ETAGSFLAGS) $(AM_ETAGSFLAGS) $(ETAGS_ARGS) \
@@ -292,13 +293,12 @@ @@ -292,13 +292,12 @@
CTAGS: $(HEADERS) $(SOURCES) $(TAGS_DEPENDENCIES) \ CTAGS: $(HEADERS) $(SOURCES) $(TAGS_DEPENDENCIES) \
$(TAGS_FILES) $(LISP) $(TAGS_FILES) $(LISP)
tags=; \ tags=; \
@ -55,7 +74,7 @@
test -z "$(CTAGS_ARGS)$$tags$$unique" \ test -z "$(CTAGS_ARGS)$$tags$$unique" \
|| $(CTAGS) $(CTAGSFLAGS) $(AM_CTAGSFLAGS) $(CTAGS_ARGS) \ || $(CTAGS) $(CTAGSFLAGS) $(AM_CTAGSFLAGS) $(CTAGS_ARGS) \
$$tags $$unique $$tags $$unique
@@ -442,7 +442,7 @@ @@ -442,7 +441,7 @@
install-sudosh.conf: install-sudosh.conf:
test -z "$(sysconfdir)" || $(mkdir_p) "$(DESTDIR)$(sysconfdir)" test -z "$(sysconfdir)" || $(mkdir_p) "$(DESTDIR)$(sysconfdir)"

View file

@ -0,0 +1,20 @@
Eliminate unused variables.
--- src/parse.c 2009-11-24 07:13:28.000000000 -0500
+++ src/parse.c 2013-02-12 16:12:57.000000000 -0500
@@ -8,9 +8,7 @@
{
FILE *f;
- unsigned int line_number, i;
char line[BUFSIZ];
int leftside;
char key[BUFSIZ], value[BUFSIZ];
- char *arg, *cmt, *opt;
char *p;
struct stat defshell_stat;
@@ -18,5 +16,5 @@
char *shell;
int found = FALSE;
- unsigned int x=0, y=0;
+ unsigned int x=0;
// bzero(c, sizeof (struct s_option));

View file

@ -0,0 +1,49 @@
Use the modern random(3) and device-based seeding.
-mi
--- src/rand.c 2009-11-18 07:23:18.000000000 -0500
+++ src/rand.c 2013-02-12 16:09:01.000000000 -0500
@@ -1,26 +1,18 @@
#include "super.h"
-int myrand(void)
-{
- struct timeval tv;
- unsigned int seed;
-
- gettimeofday(&tv, (struct timezone *) 0);
- seed = (tv.tv_sec % 10000) * 523 + tv.tv_usec * 13 + (getpid() % 1000) * 983;
- srand(seed);
-
- return rand();
-}
-
-char *rand2str(size_t len)
+const char *
+rand2str(size_t len)
{
static char buf[BUFSIZ];
char *ptr = buf;
- char *alphabet =
+ char alphabet[] =
"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789";
- int i;
+ size_t i;
+
+ if (len == 0)
+ return NULL;
- if (len < 0)
- return (char *) 0;
+ if (buf[0] == '\0') /* First time we are here */
+ srandomdev(); /* Seed */
if (len >= BUFSIZ)
@@ -28,5 +20,5 @@
for (i = 0; i < len; i++) {
- int j = (myrand() & 0xffff) % (26 + 26 + 10);
+ int j = random() % (sizeof(alphabet)/sizeof(char) - 1);
ptr[i] = alphabet[j];
}

View file

@ -0,0 +1,110 @@
Address the buffer-overflow problem outlined here:
http://packetstormsecurity.com/files/85687/sa38292.txt
(buffer is of size BUFSIZ, but the check is for 8Mb, which may not be the
same) by not using a buffer at all. Instead we simply mmap the script-file.
The patch also adds closing of the script file at the end and fixes some
compiler warnings (the the -Wall -W levels).
On 32-bit architectures this limits the replayable script-size to 4Gb.
-mi
--- src/replay.c 2009-11-24 09:54:58.000000000 -0500
+++ src/replay.c 2013-02-12 15:51:31.000000000 -0500
@@ -19,4 +19,6 @@
#include "super.h"
#include "struct.h"
+#include <sys/mman.h>
+#include <stdint.h>
#define LL() fprintf(stderr, "[%s, line %i]: ", __FILE__, __LINE__)
@@ -71,5 +73,5 @@
void show_sessions(void);
-int main(int argc, char **argv, char **environ)
+int main(int argc, char *argv[])
{
int c;
@@ -406,14 +408,15 @@
void replay(const char *time, const char *script, int div, int maxwait)
{
- char read_buffer[BUFSIZ];
+ char *read_buffer;
char timebuf[BUFSIZ];
float ftime = 0;
int b = 0;
int bInput = 0;
- int r = 0;
int sec, usec;
char buffer[BUFSIZ];
struct s_file s_time;
struct s_file s_script;
+ struct stat sb;
+ off_t offset;
struct timeval tv;
@@ -432,9 +435,13 @@
}
- if ((s_script.fd = open(script, O_RDONLY)) == -1) {
+ if ((s_script.fd = open(script, O_RDONLY)) == -1 ||
+ fstat(s_script.fd, &sb) ||
+ (read_buffer = mmap(NULL, sb.st_size, PROT_READ, MAP_NOCORE|MAP_SHARED, s_script.fd, 0))
+ == MAP_FAILED) {
LL();
fprintf(stderr, "%s: %s: %s (%i)\n", progname, script, strerror(errno), errno);
exit(EXIT_FAILURE);
}
+ offset = 0;
for (s_time.line = 1; fgets(buffer, BUFSIZ - 1, s_time.f); s_time.line++) {
@@ -478,29 +485,22 @@
tv.tv_usec = (time_t) usec;
- if (b > 1024 * 1024 * 8) { /* 8MB */
+ if (b > sb.st_size - offset) { /* 8MB */
LL();
- fprintf(stderr, "[error]: line %i: wanted to read %i bytes, but the limit is 8MB.\n", s_time.line, b);
- exit(EXIT_FAILURE);
- }
-
- memset(read_buffer, '\0', BUFSIZ);
- r = read(s_script.fd, read_buffer, (size_t) b);
-
- if (r != b) {
- LL();
- fprintf(stderr, "[failure]: read %i bytes out of %i.\n", r, b);
+ fprintf(stderr, "[error]: line %i: wanted to read %i bytes, but only %jd are "
+ "left in %s.\n", s_time.line, b, (intmax_t)sb.st_size - offset, script);
exit(EXIT_FAILURE);
}
select(0, (fd_set *) 0, (fd_set *) 0, (fd_set *) 0, &tv);
- fputs(read_buffer, stdout);
+ fwrite(read_buffer + offset, 1, b, stdout); /* Should we check for error here? XXX */
+ offset += b;
fflush(stdout);
- memset(read_buffer, '\0', BUFSIZ);
}
+ munmap(read_buffer, sb.st_size);
+ close(s_script.fd);
fprintf(stderr, "[info]: EOF\n");
fflush(stderr);
}
-
session *session_malloc(void)
{
@@ -586,5 +586,5 @@
session *sort_list(session * list)
{
- session *p, *q, *e, *tail, *oldhead;
+ session *p, *q, *e, *tail;
int insize, nmerges, psize, qsize, i;
@@ -597,5 +597,4 @@
p = list;
- oldhead = list;
list = (session *) 0;
tail = (session *) 0;

View file

@ -1,7 +1,6 @@
--- src/sudosh.c.orig 2009-11-27 02:19:58.000000000 -0800 --- src/sudosh.c 2009-11-27 05:19:58.000000000 -0500
+++ src/sudosh.c 2009-12-14 17:30:23.000000000 -0800 +++ src/sudosh.c 2013-02-12 16:10:41.000000000 -0500
@@ -27,6 +27,13 @@ @@ -28,4 +28,11 @@
#define WRITE(a, b, c) do_write(a, b, c, __FILE__, __LINE__) #define WRITE(a, b, c) do_write(a, b, c, __FILE__, __LINE__)
+#ifdef __FreeBSD__ +#ifdef __FreeBSD__
@ -13,9 +12,49 @@
+ +
typedef enum {false=0, true=1} bool; typedef enum {false=0, true=1} bool;
static struct termios termorig; @@ -94,5 +101,5 @@
@@ -443,12 +450,32 @@ static int findms (struct pst *);
void mysyslog (int, const char *, ...);
-char *rand2str (size_t len);
+const char *rand2str (size_t len);
int do_write (int, void *, size_t, char *, unsigned int);
@@ -109,10 +116,9 @@
extern int optind;
-int main (int argc, char *argv[], char *environ[])
+int main(int argc, char *argv[])
{ {
int result = EXIT_SUCCESS;
// int n = 1;
int valid = -1;
- int found = FALSE;
// char iobuf[BUFSIZ];
char sysconfdir[BUFSIZ];
@@ -120,6 +126,5 @@
char c_command[BUFSIZ];
char *p = NULL;
- char *c_args = NULL;
- char *rand = rand2str (16);
+ const char *rand = rand2str (16);
time_t now = time ((time_t *) NULL);
struct stat s;
@@ -178,13 +183,11 @@
strncpy (c_str, optarg, BUFSIZ - 1);
strncpy (c_command, optarg, BUFSIZ -1);
- c_args = (char *) strchr (optarg, ' ');
p=strchr(c_str, ' ');
if (p) {
p[0]=0;
- // fprintf(stderr,"args=%s\n",c_args);
}
- if (c_str) {
+ {
// fprintf(stderr,"Testing c\n");
// Make sure that c_str is in argallow
@@ -444,10 +447,30 @@
char *sname; char *sname;
+#ifdef __FreeBSD__ +#ifdef __FreeBSD__
@ -46,9 +85,7 @@
+#endif +#endif
} }
(void) unlockpt (p->mfd); @@ -516,7 +539,12 @@
@@ -515,9 +542,14 @@
for (i = 3; i < 100; ++i)
close (i); close (i);
+#ifdef __FreeBSD__ +#ifdef __FreeBSD__
@ -61,9 +98,7 @@
+#endif +#endif
(void) ioctl (0, TIOCSWINSZ, &winorig); (void) ioctl (0, TIOCSWINSZ, &winorig);
setuid (getuid ()); @@ -672,4 +700,11 @@
@@ -671,12 +703,20 @@
{
static struct termios termnew; static struct termios termnew;
+#ifdef __FreeBSD__ +#ifdef __FreeBSD__
@ -75,16 +110,13 @@
+#else +#else
#ifdef TCGETS #ifdef TCGETS
if (ioctl (ttyfd, TCGETS, &termorig) == -1) { if (ioctl (ttyfd, TCGETS, &termorig) == -1) {
perror ("ioctl TCGETS failed"); @@ -678,4 +713,5 @@
exit (EXIT_FAILURE);
} }
#endif #endif
+#endif +#endif
if (ioctl (ttyfd, TIOCGWINSZ, &winorig) == -1) { if (ioctl (ttyfd, TIOCGWINSZ, &winorig) == -1) {
// perror ("ioctl TIOCGWINSZ failed"); @@ -686,4 +722,9 @@
@@ -685,6 +725,11 @@
winorig.ws_col = 80;
} }
+#ifdef __FreeBSD__ +#ifdef __FreeBSD__
@ -94,9 +126,7 @@
+#else +#else
termnew.c_cc[VEOF] = 1; termnew.c_cc[VEOF] = 1;
termnew.c_iflag = BRKINT | ISTRIP | IXON | IXANY; termnew.c_iflag = BRKINT | ISTRIP | IXON | IXANY;
termnew.c_oflag = 0; @@ -695,11 +736,16 @@
@@ -694,13 +739,18 @@
#ifdef TCSETS
(void) ioctl (ttyfd, TCSETS, &termnew); (void) ioctl (ttyfd, TCSETS, &termnew);
#endif #endif
+#endif +#endif
@ -113,4 +143,10 @@
+#endif +#endif
close (timing.fd); close (timing.fd);
close (script.fd); @@ -713,5 +759,5 @@
}
-static void newwinsize (int signum)
+static void newwinsize (int signum __unused)
{
int fd;