Page MenuHomeFreeBSD
Differential D7655 Diff 19711 usr.bin/uudecode/uudecode.c

Changeset View

Standalone View

View Options

usr.bin/uudecode/uudecode.c

Show First 20 Lines • Show All 41 Lines • ▼ Show 20 Lines
__FBSDID("$FreeBSD$"); __FBSDID("$FreeBSD$");
/* /*
* uudecode [file ...] * uudecode [file ...]
* *
* create the specified file, decoding as you go. * create the specified file, decoding as you go.
* used with uuencode. * used with uuencode.
*/ */
#include <sys/capsicum.h>
#include <sys/param.h> #include <sys/param.h>
#include <sys/socket.h> #include <sys/socket.h>
#include <sys/stat.h> #include <sys/stat.h>
#include <netinet/in.h> #include <netinet/in.h>
#include <ctype.h> #include <ctype.h>
#include <err.h> #include <err.h>
#include <errno.h> #include <errno.h>
#include <fcntl.h> #include <fcntl.h>
#include <libgen.h> #include <libgen.h>
#include <pwd.h> #include <pwd.h>
#include <resolv.h> #include <resolv.h>
#include <stdio.h> #include <stdio.h>
#include <stdlib.h> #include <stdlib.h>
#include <string.h> #include <string.h>
#include <unistd.h> #include <unistd.h>
static const char *infile, *outfile; static const char *outfile;
static FILE *infp, *outfp; static FILE *outfp;
static int base64, cflag, iflag, oflag, pflag, rflag, sflag; static int base64, cflag, iflag, oflag, pflag, rflag, sflag, dirfd;
static void usage(void); static void usage(void);
static int decode(void); static int decode(FILE *infp, char *infile);
static int decode2(void); static int decode2(FILE *infp, char *infile);
static int uu_decode(void); static int uu_decode(FILE *infp, char *infile);
static int base64_decode(void); static int base64_decode(FILE *infp, char *infile);
static int
safeoutfp(void)
{
cemUnsubmitted
Not Done
Inline Actions

Why have this at all?

We keep a dirfd around which can open outfile anyway — no need to open the file before entering sandbox.

It duplicates the safe-output-file checking logic below, except for potential time-of-check vs time-of-use attacks. IMO, this whole function can be removed.

cem: Why have this at all? We keep a dirfd around which can open outfile anyway — no need to open…
int flags, fd;
struct stat st;
flags = O_WRONLY | O_CREAT | O_EXCL;
if (lstat(outfile, &st) == 0) {
if (iflag) {
warnc(EEXIST, "%s", outfile);
return (0);
}
switch (st.st_mode & S_IFMT) {
case S_IFREG:
case S_IFLNK:
/* avoid symlink attacks */
if (unlink(outfile) == 0 || errno == ENOENT)
break;
warn("unlink %s", outfile);
return (1);
case S_IFDIR:
warnc(EISDIR, "%s", outfile);
return (1);
default:
if (oflag) {
/* trust command-line names */
flags &= ~O_EXCL;
break;
}
warnc(EEXIST, "%s", outfile);
return (1);
}
} else if (errno != ENOENT) {
warn("stat %s", outfile);
return (1);
}
if ((fd = open(outfile, flags, 0664)) < 0 ||
(outfp = fdopen(fd, "w")) == NULL) {
warn("open %s", outfile);
return (1);
}
return (0);
}
int int
main(int argc, char *argv[]) main(int argc, char *argv[])
{ {
int rval, ch; int rval, ch, fnum, i;
FILE **infps;
char **infiles;
fnum = 0;
rval = 0;
outfp = NULL;
if (strcmp(basename(argv[0]), "b64decode") == 0) if (strcmp(basename(argv[0]), "b64decode") == 0)
base64 = 1; base64 = 1;
while ((ch = getopt(argc, argv, "cimo:prs")) != -1) { while ((ch = getopt(argc, argv, "cimo:prs")) != -1) {
switch (ch) { switch (ch) {
case 'c': case 'c':
if (oflag || rflag) if (oflag || rflag)
usage(); usage();
cflag = 1; /* multiple uudecode'd files */ cflag = 1; /* multiple uudecode'd files */
break; break;
case 'i': case 'i':
iflag = 1; /* ask before override files */ iflag = 1; /* ask before override files */
break; break;
case 'm': case 'm':
base64 = 1; base64 = 1;
break; break;
case 'o': case 'o':
if (cflag || pflag || rflag || sflag) if (cflag || pflag || rflag || sflag)
usage(); usage();
oflag = 1; /* output to the specified file */ oflag = 1; /* output to the specified file */
sflag = 1; /* do not strip pathnames for output */ sflag = 1; /* do not strip pathnames for output */
outfile = optarg; /* set the output filename */ outfile = optarg; /* set the output filename */
/* Precreate the output fd before sandboxing */
rval = safeoutfp();
if (rval != 0)
exit(rval);
break; break;
case 'p': case 'p':
if (oflag) if (oflag)
usage(); usage();
pflag = 1; /* print output to stdout */ pflag = 1; /* print output to stdout */
break; break;
case 'r': case 'r':
if (cflag || oflag) if (cflag || oflag)
usage(); usage();
rflag = 1; /* decode raw data */ rflag = 1; /* decode raw data */
break; break;
case 's': case 's':
if (oflag) if (oflag)
usage(); usage();
sflag = 1; /* do not strip pathnames for output */ sflag = 1; /* do not strip pathnames for output */
break; break;
default: default:
usage(); usage();
} }
} }
argc -= optind; argc -= optind;
argv += optind; argv += optind;
if (*argv != NULL) { if (*argv != NULL) {
rval = 0; infps = malloc(sizeof(FILE*) * argc);
infiles = malloc(sizeof(char*) * argc);
oshogboUnsubmitted
Not Done
Inline Actions

This can fail.

oshogbo: This can fail.
do { do {
infp = fopen(infile = *argv, "r"); infps[fnum] = fopen(*argv, "r");
if (infp == NULL) { if (infps[fnum] == NULL) {
cemUnsubmitted
Not Done
Inline Actions

I think in general we should preopen an array of fds rather than FILE objects, then fdopen as needed. (The concern is that the overhead of FILE objects vs fds is meaningful.)

One potential cost is the size of the stdio buffering. I guess I would be a little shocked if FILE prefetched file contents or allocated much memory on open. But even so a FILE is bulkier than just an fd.

cem: I think in general we should preopen an array of `fd`s rather than `FILE` objects, then…
warn("%s", *argv); warn("%s", *argv);
rval = 1; rval = 1;
continue; continue;
} }
rval |= decode(); infiles[fnum] = *argv;
fclose(infp); fnum++;
} while (*++argv); } while (*++argv);
} else { } else {
infile = "stdin"; infps = malloc(sizeof(FILE*));
infp = stdin; infiles = malloc(sizeof(char*));
oshogboUnsubmitted
Not Done
Inline Actions

malloc can fail.

oshogbo: malloc can fail.
rval = decode(); infps[fnum] = stdin;
infiles[fnum] = strdup("stdin");
oshogboUnsubmitted
Not Done
Inline Actions

strdup as well.

oshogbo: strdup as well.
} }
if ((dirfd = open(".", O_DIRECTORY)) < 0) {
warn("open(%s)", ".");
exit(1);
}
if (cap_enter() < 0) {
oshogboUnsubmitted
Not Done
Inline Actions

This can be done in oneline.

oshogbo: This can be done in oneline.
allanjudeAuthorUnsubmitted
Not Done
Inline Actions

should be place limits on this directory FD?

allanjude: should be place limits on this directory FD?
cemUnsubmitted
Not Done
Inline Actions

Yep. Whatever rights you want openated fds to have, plus CAP_LOOKUP.

cem: Yep. Whatever rights you want `openat`ed fds to have, plus `CAP_LOOKUP`.
cemUnsubmitted
Not Done
Inline Actions

And rights for fstatat and unlinkat.

cem: And rights for `fstatat` and `unlinkat`.
/* Failed to sandbox, fatal if CAPABILITY_MODE enabled */
if (errno != ENOSYS)
err(1, "failed to enter security sandbox");
}
for (i = 0; i < fnum; i++) {
rval |= decode(infps[i], infiles[i]);
fclose(infps[i]);
}
free(infps);
free(infiles);
exit(rval); exit(rval);
} }
static int static int
decode(void) decode(FILE *infp, char *infile)
{ {
int r, v; int r, v;
if (rflag) { if (rflag) {
/* relaxed alternative to decode2() */ /* relaxed alternative to decode2() */
outfile = "/dev/stdout"; outfile = "/dev/stdout";
outfp = stdout; outfp = stdout;
if (base64) if (base64)
return (base64_decode()); return (base64_decode(infp, infile));
else else
return (uu_decode()); return (uu_decode(infp, infile));
} }
v = decode2(); v = decode2(infp, infile);
cemUnsubmitted
Not Done
Inline Actions

I think you could avoid a lot of unnecessary code churn of this patch by setting globals infile, infp, (as is the existing pattern in this code) rather than passing these things as parameters.

cem: I think you could avoid a lot of unnecessary code churn of this patch by setting globals…
if (v == EOF) { if (v == EOF) {
warnx("%s: missing or bad \"begin\" line", infile); warnx("%s: missing or bad \"begin\" line", infile);
return (1); return (1);
} }
for (r = v; cflag; r |= v) { for (r = v; cflag; r |= v) {
v = decode2(); v = decode2(infp, infile);
if (v == EOF) if (v == EOF)
break; break;
} }
return (r); return (r);
} }
static int static int
decode2(void) decode2(FILE *infp, char *infile)
{ {
int flags, fd, mode; int flags, fd, mode;
size_t n, m; size_t n, m;
char *p, *q; char *p, *q;
void *handle; void *handle;
struct passwd *pw; struct passwd *pw;
struct stat st; struct stat st;
char buf[MAXPATHLEN + 1]; char buf[MAXPATHLEN + 1];
▲ Show 20 Lines • Show All 65 Lines • ▼ Show 20 Lines if (p != NULL)
q = p + 1; q = p + 1;
} }
if (!oflag) if (!oflag)
outfile = q; outfile = q;
/* POSIX says "/dev/stdout" is a 'magic cookie' not a special file. */ /* POSIX says "/dev/stdout" is a 'magic cookie' not a special file. */
if (pflag || strcmp(outfile, "/dev/stdout") == 0) if (pflag || strcmp(outfile, "/dev/stdout") == 0)
outfp = stdout; outfp = stdout;
else { else if (oflag && outfp != NULL) {
/* set mode of file created earlier */
if (fchmod(fileno(outfp), mode) != 0)
warn("%s: chmod %s", infile, outfile);
}else {
allanjudeAuthorUnsubmitted
Not Done
Inline Actions

missing space

allanjude: missing space
flags = O_WRONLY | O_CREAT | O_EXCL; flags = O_WRONLY | O_CREAT | O_EXCL;
if (lstat(outfile, &st) == 0) { if (fstatat(dirfd, outfile, &st, AT_SYMLINK_NOFOLLOW) == 0) {
if (iflag) { if (iflag) {
warnc(EEXIST, "%s: %s", infile, outfile); warnc(EEXIST, "%s: %s", infile, outfile);
return (0); return (0);
} }
switch (st.st_mode & S_IFMT) { switch (st.st_mode & S_IFMT) {
case S_IFREG: case S_IFREG:
case S_IFLNK: case S_IFLNK:
/* avoid symlink attacks */ /* avoid symlink attacks */
if (unlink(outfile) == 0 || errno == ENOENT) if (unlinkat(dirfd, outfile, 0) == 0 ||
errno == ENOENT)
break; break;
warn("%s: unlink %s", infile, outfile); warn("%s: unlink %s", infile, outfile);
return (1); return (1);
case S_IFDIR: case S_IFDIR:
warnc(EISDIR, "%s: %s", infile, outfile); warnc(EISDIR, "%s: %s", infile, outfile);
return (1); return (1);
default: default:
if (oflag) { if (oflag) {
/* trust command-line names */ /* trust command-line names */
flags &= ~O_EXCL; flags &= ~O_EXCL;
break; break;
} }
warnc(EEXIST, "%s: %s", infile, outfile); warnc(EEXIST, "%s: %s", infile, outfile);
return (1); return (1);
} }
} else if (errno != ENOENT) { } else if (errno != ENOENT) {
warn("%s: %s", infile, outfile); warn("%s: stat %s", infile, outfile);
return (1); //return (1);
allanjudeAuthorUnsubmitted
Not Done
Inline Actions

leftover debugging

allanjude: leftover debugging
} }
if ((fd = open(outfile, flags, mode)) < 0 || if ((fd = openat(dirfd, outfile, flags, mode)) < 0 ||
allanjudeAuthorUnsubmitted
Not Done
Inline Actions

do we need to limit FDs opened within the sandbox?

allanjude: do we need to limit FDs opened within the sandbox?
cemUnsubmitted
Not Done
Inline Actions

I think openat from a restricted dirfd inherits dirfd's restrictions. Not 100% sure of this.

Even if so, maybe child fds don't need CAP_LOOKUP.

cem: I think `openat` from a restricted dirfd inherits `dirfd`'s restrictions. Not 100% sure of…
(outfp = fdopen(fd, "w")) == NULL) { (outfp = fdopen(fd, "w")) == NULL) {
warn("%s: %s", infile, outfile); warn("%s: open %s", infile, outfile);
return (1); return (1);
} }
} }
if (base64) if (base64)
return (base64_decode()); return (base64_decode(infp, infile));
else else
return (uu_decode()); return (uu_decode(infp, infile));
} }
static int static int
get_line(char *buf, size_t size) get_line(FILE *infp, char *infile, char *buf, size_t size)
{ {
if (fgets(buf, size, infp) != NULL) if (fgets(buf, size, infp) != NULL)
return (2); return (2);
if (rflag) if (rflag)
return (0); return (0);
warnx("%s: %s: short file", infile, outfile); warnx("%s: %s: short file", infile, outfile);
return (1); return (1);
} }
static int static int
checkend(const char *ptr, const char *end, const char *msg) checkend(char *infile, const char *ptr, const char *end, const char *msg)
{ {
size_t n; size_t n;
n = strlen(end); n = strlen(end);
if (strncmp(ptr, end, n) != 0 || if (strncmp(ptr, end, n) != 0 ||
strspn(ptr + n, " \t\r\n") != strlen(ptr + n)) { strspn(ptr + n, " \t\r\n") != strlen(ptr + n)) {
warnx("%s: %s: %s", infile, outfile, msg); warnx("%s: %s: %s", infile, outfile, msg);
return (1); return (1);
} }
if (fclose(outfp) != 0) { if (fclose(outfp) != 0) {
warn("%s: %s", infile, outfile); warn("%s: %s", infile, outfile);
return (1); return (1);
} }
return (0); return (0);
} }
static int static int
uu_decode(void) uu_decode(FILE *infp, char *infile)
{ {
int i, ch; int i, ch;
char *p; char *p;
char buf[MAXPATHLEN+1]; char buf[MAXPATHLEN+1];
/* for each input line */ /* for each input line */
for (;;) { for (;;) {
switch (get_line(buf, sizeof(buf))) { switch (get_line(infp, infile, buf, sizeof(buf))) {
case 0: case 0:
return (0); return (0);
case 1: case 1:
return (1); return (1);
} }
#define DEC(c) (((c) - ' ') & 077) /* single character decode */ #define DEC(c) (((c) - ' ') & 077) /* single character decode */
#define IS_DEC(c) ( (((c) - ' ') >= 0) && (((c) - ' ') <= 077 + 1) ) #define IS_DEC(c) ( (((c) - ' ') >= 0) && (((c) - ' ') <= 077 + 1) )
▲ Show 20 Lines • Show All 42 Lines • ▼ Show 20 Lines for (++p; i > 0; p += 4, i -= 3)
if (!(IS_DEC(*(p + 2)) && if (!(IS_DEC(*(p + 2)) &&
IS_DEC(*(p + 3)))) IS_DEC(*(p + 3))))
OUT_OF_RANGE; OUT_OF_RANGE;
ch = DEC(p[2]) << 6 | DEC(p[3]); ch = DEC(p[2]) << 6 | DEC(p[3]);
putc(ch, outfp); putc(ch, outfp);
} }
} }
} }
switch (get_line(buf, sizeof(buf))) { switch (get_line(infp, infile, buf, sizeof(buf))) {
case 0: case 0:
return (0); return (0);
case 1: case 1:
return (1); return (1);
default: default:
return (checkend(buf, "end", "no \"end\" line")); return (checkend(infile, buf, "end", "no \"end\" line"));
} }
} }
static int static int
base64_decode(void) base64_decode(FILE *infp, char *infile)
{ {
int n, count, count4; int n, count, count4;
char inbuf[MAXPATHLEN + 1], *p; char inbuf[MAXPATHLEN + 1], *p;
unsigned char outbuf[MAXPATHLEN * 4]; unsigned char outbuf[MAXPATHLEN * 4];
char leftover[MAXPATHLEN + 1]; char leftover[MAXPATHLEN + 1];
leftover[0] = '\0'; leftover[0] = '\0';
for (;;) { for (;;) {
strcpy(inbuf, leftover); strcpy(inbuf, leftover);
switch (get_line(inbuf + strlen(inbuf), switch (get_line(infp, infile, inbuf + strlen(inbuf),
sizeof(inbuf) - strlen(inbuf))) { sizeof(inbuf) - strlen(inbuf))) {
case 0: case 0:
return (0); return (0);
case 1: case 1:
return (1); return (1);
} }
count = 0; count = 0;
Show All 15 Lines for (;;) {
inbuf[count4 + 1] = 0; inbuf[count4 + 1] = 0;
n = b64_pton(inbuf, outbuf, sizeof(outbuf)); n = b64_pton(inbuf, outbuf, sizeof(outbuf));
if (n < 0) if (n < 0)
break; break;
fwrite(outbuf, 1, n, outfp); fwrite(outbuf, 1, n, outfp);
} }
return (checkend(inbuf, "====", "error decoding base64 input stream")); return (checkend(infile, inbuf, "====", "error decoding base64 input stream"));
} }
static void static void
usage(void) usage(void)
{ {
(void)fprintf(stderr, (void)fprintf(stderr,
"usage: uudecode [-cimprs] [file ...]\n" "usage: uudecode [-cimprs] [file ...]\n"
" uudecode [-i] -o output_file [file]\n" " uudecode [-i] -o output_file [file]\n"
" b64decode [-cimprs] [file ...]\n" " b64decode [-cimprs] [file ...]\n"
" b64decode [-i] -o output_file [file]\n"); " b64decode [-i] -o output_file [file]\n");
exit(1); exit(1);
} }