From owner-freebsd-security@FreeBSD.ORG Tue May 4 17:39:11 2004 Return-Path: Delivered-To: freebsd-security@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 2828816A4CF for ; Tue, 4 May 2004 17:39:11 -0700 (PDT) Received: from sccrmhc11.comcast.net (sccrmhc11.comcast.net [204.127.202.55]) by mx1.FreeBSD.org (Postfix) with ESMTP id 622AB43D5C for ; Tue, 4 May 2004 17:39:10 -0700 (PDT) (envelope-from cristjc@comcast.net) Received: from blossom.cjclark.org (c-24-6-187-112.client.comcast.net[24.6.187.112]) by comcast.net (sccrmhc11) with ESMTP id <2004050500390901100d30r3e>; Wed, 5 May 2004 00:39:09 +0000 Received: from blossom.cjclark.org (localhost. [127.0.0.1]) by blossom.cjclark.org (8.12.9p2/8.12.8) with ESMTP id i450d88B081667 for ; Tue, 4 May 2004 17:39:08 -0700 (PDT) (envelope-from cristjc@comcast.net) Received: (from cjc@localhost) by blossom.cjclark.org (8.12.9p2/8.12.9/Submit) id i450d7v3081666 for freebsd-security@freebsd.org; Tue, 4 May 2004 17:39:07 -0700 (PDT) (envelope-from cristjc@comcast.net) X-Authentication-Warning: blossom.cjclark.org: cjc set sender to cristjc@comcast.net using -f Date: Tue, 4 May 2004 17:39:07 -0700 From: "Crist J. Clark" To: freebsd-security@freebsd.org Message-ID: <20040505003907.GA80906@blossom.cjclark.org> References: <20040504054909.GA3119@lame.novel.ru> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20040504054909.GA3119@lame.novel.ru> User-Agent: Mutt/1.4.2.1i X-URL: http://people.freebsd.org/~cjc/ Subject: Re: ctags(1) command execution vulnerability X-BeenThere: freebsd-security@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list Reply-To: "Crist J. Clark" List-Id: Security issues [members-only posting] List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 05 May 2004 00:39:11 -0000 On Tue, May 04, 2004 at 09:49:09AM +0400, Roman Bogorodskiy wrote: > Hello, > > ctags(1) uses external application sort(1) for sorting the tags file. > It calls it via system(3) function. > > Look at the /usr/src/usr.bin/ctags/ctags.c file, there are such lines > here: > > if (uflag) { > (void)asprintf(&cmd, "sort -o %s %s", > outfile, outfile); > if (cmd == NULL) > err(1, "out of space"); > system(cmd); > free(cmd); > cmd = NULL; > } Ick. There's another system(3) call above this one too, for (step = 0; step < argc; step++) { (void)asprintf(&cmd, "mv %s OTAGS; fgrep -v '\t%s\t' OTAGS >%s; rm OTAGS", outfile, argv[step], outfile); if (cmd == NULL) err(1, "out of space"); system(cmd); free(cmd); cmd = NULL; } Also bad. > This code will be executed when "-u" arg was given. So, if we'll execute > ctags in a such way: > > ctags -u -f ';echo hi' *.c > > we get the following: > > Syntax error: ";" unexpected > sort: option requires an argument -- o > Try `sort --help' for more information. > hi > hi The two 'hi's are from each system(3) call. > Solution: As has been pointed out, the problem here is user supplied data to a system(3) call that we really cannot sanitize without filtering a lot of valid file names. The Right Thing is to get rid of system(3). This seems to work. Fixing the sort is trivial. Adding the regex checks to the program adds a little complexity, but not a lot. Anyone who actually uses ctags(1) want to try them out some more to see if they hold up? Index: ctags.c =================================================================== RCS file: /export/freebsd/ncvs/src/usr.bin/ctags/ctags.c,v retrieving revision 1.18 diff -u -r1.18 ctags.c --- ctags.c 28 Jul 2002 15:50:38 -0000 1.18 +++ ctags.c 5 May 2004 00:27:09 -0000 @@ -44,11 +44,13 @@ #endif #include +#include __FBSDID("$FreeBSD: src/usr.bin/ctags/ctags.c,v 1.18 2002/07/28 15:50:38 dwmalone Exp $"); #include #include #include +#include #include #include #include @@ -164,16 +166,37 @@ put_entries(head); else { if (uflag) { + FILE *oldf; + regex_t *regx; + + if ((oldf = fopen(outfile, "r")) == NULL) + err(1, "opening %s", outfile); + if (unlink(outfile)) + err(1, "unlinking %s", outfile); + if ((outf = fopen(outfile, "w")) == NULL) + err(1, "recreating %s", outfile); + if ((regx = calloc(argc, sizeof(regex_t))) == NULL) + err(1, "RE alloc"); for (step = 0; step < argc; step++) { - (void)asprintf(&cmd, - "mv %s OTAGS; fgrep -v '\t%s\t' OTAGS >%s; rm OTAGS", - outfile, argv[step], outfile); - if (cmd == NULL) - err(1, "out of space"); - system(cmd); - free(cmd); - cmd = NULL; + (void)strcpy(lbuf, "\t"); + (void)strlcat(lbuf, argv[step], LINE_MAX); + (void)strlcat(lbuf, "\t", LINE_MAX); + if (regcomp(regx + step, lbuf, + REG_NOSPEC)) + warn("RE compilation failed"); + } +nextline: + while (fgets(lbuf, LINE_MAX, oldf)) { + for (step = 0; step < argc; step++) + if (regexec(regx + step, + lbuf, 0, NULL, 0) == 0) + goto nextline; + fputs(lbuf, outf); } + for (step = 0; step < argc; step++) + regfree(regx + step); + fclose(oldf); + fclose(outf); ++aflag; } if (!(outf = fopen(outfile, aflag ? "a" : "w"))) @@ -181,13 +204,15 @@ put_entries(head); (void)fclose(outf); if (uflag) { - (void)asprintf(&cmd, "sort -o %s %s", - outfile, outfile); - if (cmd == NULL) - err(1, "out of space"); - system(cmd); - free(cmd); - cmd = NULL; + pid_t pid; + if ((pid = fork()) == -1) + err(1, "fork failed"); + else if (pid == 0) { + execlp("sort", "sort", "-o", outfile, + outfile, NULL); + /* NOT REACHED */ + err(1, "exec of sort failed"); + } } } } -- Crist J. Clark | cjclark@alum.mit.edu | cjclark@jhu.edu http://people.freebsd.org/~cjc/ | cjc@freebsd.org