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;
}
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
We can put any command instead of 'echo hi' and it would be executed
(for two times).
I understand that ctags(1) is not a suid application and this
vulnerability probably could not be exploited. Never the less, this is a
bad behavior for any kind of program.
Solution:
--- usr.bin/ctags/ctags.c.orig Tue May 4 09:23:30 2004
+++ usr.bin/ctags/ctags.c Tue May 4 09:25:48 2004
@@ -166,7 +166,7 @@
if (uflag) {
for (step = 0; step < argc; step++) {
(void)asprintf(&cmd,
- "mv %s OTAGS; fgrep -v '\t%s\t' OTAGS >%s; rm
OTAGS",
+ "mv '%s' OTAGS; fgrep -v '\t%s\t' OTAGS
>'%s'; rm OTAGS",
outfile, argv[step], outfile);
if (cmd == NULL)
err(1, "out of space");
@@ -181,7 +181,7 @@
put_entries(head);
(void)fclose(outf);
if (uflag) {
- (void)asprintf(&cmd, "sort -o %s %s",
+ (void)asprintf(&cmd, "sort -o '%s' '%s'",
outfile, outfile);
if (cmd == NULL)
err(1, "out of space");
-Roman Bogorodskiy
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 479 bytes
Desc: not available
Url :
http://lists.freebsd.org/pipermail/freebsd-security/attachments/20040504/cb59d4af/attachment.bin
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.[snip]> 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[snip]> - "mv %s OTAGS; fgrep -v '\t%s\t' OTAGS >%s; rm OTAGS", > + "mv '%s' OTAGS; fgrep -v '\t%s\t' OTAGS >'%s'; rm OTAGS",[snip]> - (void)asprintf(&cmd, "sort -o %s %s", > + (void)asprintf(&cmd, "sort -o '%s' '%s'",Unfortunately, this is still not a complete solution; the following still works, at least for me: ctags -u -f "'; echo hi; '" *.c Filtering the filename characters would be a better idea; possibly something like the attached patch. G'luck, Peter -- Peter Pentchev roam@ringlet.net roam@sbnd.net roam@FreeBSD.org PGP key: http://people.FreeBSD.org/~roam/roam.key.asc Key fingerprint FDBA FD79 C26F 3C51 C95E DF9E ED18 B68D 1619 4553 This sentence contradicts itself - or rather - well, no, actually it doesn't! Index: src/usr.bin/ctags/ctags.c ==================================================================RCS file: /home/ncvs/src/usr.bin/ctags/ctags.c,v retrieving revision 1.18 diff -u -r1.18 ctags.c --- src/usr.bin/ctags/ctags.c 28 Jul 2002 15:50:38 -0000 1.18 +++ src/usr.bin/ctags/ctags.c 4 May 2004 06:27:23 -0000 @@ -46,6 +46,7 @@ #include <sys/cdefs.h> __FBSDID("$FreeBSD: src/usr.bin/ctags/ctags.c,v 1.18 2002/07/28 15:50:38 dwmalone Exp $"); +#include <ctype.h> #include <err.h> #include <limits.h> #include <locale.h> @@ -84,6 +85,7 @@ void init(void); void find_entries(char *); static void usage(void); +static char *validatename(const char *); int main(int argc, char **argv) @@ -118,7 +120,7 @@ dflag++; break; case 'f': - outfile = optarg; + outfile = validatename(optarg); break; case 't': tflag = YES; @@ -201,6 +203,25 @@ exit(1); } +static char * +validatename(const char *fname) +{ + char *n, *q; + const char *p, *end; + size_t len; + + len = strlen(fname); + n = malloc(len + 1); + if (n == NULL) + errx(1, "out of memory"); + end = n + len; + for (p = fname, q = n; *p != '\0' && q != end; p++) + if (isalnum(*p) || strchr("-_./", *p) != NULL) + *q++ = *p; + *q = '\0'; + return (n); +} + /* * init -- * this routine sets up the boolean pseudo-functions which work by -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 187 bytes Desc: not available Url : http://lists.freebsd.org/pipermail/freebsd-security/attachments/20040504/6c3d5a0c/attachment.bin
While I don't think that's much of a vulnerability (you can only really attack your own account), your patch doesn't fix it. You can still executed code with: ctags -u -f "'; echo hi '" *.c To remove this "vulnerability," you'd have to either escape the string, then quote it, or even better, do the system call with a vector. It probably isn't worth the bother, but if you want to patch it, patch it right... Mike Hamburg On May 4, 2004, at 1:49 AM, 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; > } > > 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 > > We can put any command instead of 'echo hi' and it would be executed > (for two times). > > I understand that ctags(1) is not a suid application and this > vulnerability probably could not be exploited. Never the less, this is > a > bad behavior for any kind of program. > > Solution: > > --- usr.bin/ctags/ctags.c.orig Tue May 4 09:23:30 2004 > +++ usr.bin/ctags/ctags.c Tue May 4 09:25:48 2004 > @@ -166,7 +166,7 @@ > if (uflag) { > for (step = 0; step < argc; step++) { > (void)asprintf(&cmd, > - "mv %s OTAGS; fgrep -v '\t%s\t' OTAGS >%s; rm OTAGS", > + "mv '%s' OTAGS; fgrep -v '\t%s\t' OTAGS >'%s'; rm OTAGS", > outfile, argv[step], outfile); > if (cmd == NULL) > err(1, "out of space"); > @@ -181,7 +181,7 @@ > put_entries(head); > (void)fclose(outf); > if (uflag) { > - (void)asprintf(&cmd, "sort -o %s %s", > + (void)asprintf(&cmd, "sort -o '%s' '%s'", > outfile, outfile); > if (cmd == NULL) > err(1, "out of space"); > > > -Roman Bogorodskiy >
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 > hiThe 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 <sys/cdefs.h> +#include <sys/types.h> __FBSDID("$FreeBSD: src/usr.bin/ctags/ctags.c,v 1.18 2002/07/28 15:50:38 dwmalone Exp $"); #include <err.h> #include <limits.h> #include <locale.h> +#include <regex.h> #include <stdio.h> #include <stdlib.h> #include <string.h> @@ -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
Crist wrote:> 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?Using fork() + execlp() instead of system() is a good idea. Your solution works for me. Will this fix be commited? -Roman Bogorodskiy -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 479 bytes Desc: not available Url : http://lists.freebsd.org/pipermail/freebsd-security/attachments/20040507/cdd23b87/attachment.bin