Page MenuHomeFreeBSD
Differential D7655 Diff 21338 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 <capsicum_helpers.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 *infile, *outfile;
static FILE *infp, *outfp; static FILE *infp, *outfp;
static int base64, cflag, iflag, oflag, pflag, rflag, sflag; static int base64, cflag, iflag, oflag, pflag, rflag, sflag, cwdfd;
static void usage(void); static void usage(void);
static int decode(void); static int decode(void);
static int decode2(void); static int decode2(void);
static int uu_decode(void); static int uu_decode(void);
static int base64_decode(void); static int base64_decode(void);
int int
main(int argc, char *argv[]) main(int argc, char *argv[])
{ {
int rval, ch; int rval, ch, fnum, i;
int *infds;
char **infiles;
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…
cap_rights_t rights_ro;
fnum = 0;
rval = 0;
cap_rights_init(&rights_ro, CAP_READ, CAP_FSTAT, CAP_FCNTL);
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();
Show All 30 Lines while ((ch = getopt(argc, argv, "cimo:prs")) != -1) {
default: default:
usage(); usage();
} }
} }
argc -= optind; argc -= optind;
argv += optind; argv += optind;
if (*argv != NULL) { if (*argv != NULL) {
rval = 0; infds = calloc(argc, sizeof(int));
infiles = calloc(argc, sizeof(char*));
oshogboUnsubmitted
Not Done
Inline Actions

This can fail.

oshogbo: This can fail.
if (infds == NULL || infiles == NULL)
errc(1, ENOMEM, "malloc() fds");
do { do {
infp = fopen(infile = *argv, "r"); infds[fnum] = open(*argv, O_RDONLY);
if (infp == NULL) { if (infds[fnum] == -1) {
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(); if (cap_rights_limit(infds[fnum], &rights_ro) < 0 && errno != ENOSYS)
fclose(infp); err(1, "unable to limit rights on: %s", *argv);
if (cap_fcntls_limit(infds[fnum], CAP_FCNTL_GETFL) < 0 && errno != ENOSYS)
err(1, "unable to limit fcntls on: %s", *argv);
infiles[fnum] = *argv;
fnum++;
} while (*++argv); } while (*++argv);
} else { } else {
infile = "stdin"; infds = malloc(sizeof(int));
infp = stdin; infiles = malloc(sizeof(char*));
oshogboUnsubmitted
Not Done
Inline Actions

malloc can fail.

oshogbo: malloc can fail.
rval = decode(); if (infds == NULL || infiles == NULL)
errc(1, ENOMEM, "malloc() fd");
oshogboUnsubmitted
Not Done
Inline Actions

strdup as well.

oshogbo: strdup as well.
infds[fnum] = STDIN_FILENO;
infiles[fnum] = strdup("stdin");
if (infiles[fnum] == NULL)
errc(1, ENOMEM, "malloc()");
fnum++;
} }
if ((cwdfd = open(".", O_DIRECTORY)) < 0)
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`.
err(1, "Failed to open the working directory");
caph_cache_catpages();
if (caph_limit_stdio() == -1)
err(1, "unable to limit stdio");
if (cap_enter() < 0 && errno != ENOSYS)
err(1, "failed to enter security sandbox");
for (i = 0; i < fnum; i++) {
infp = fdopen(infds[i], "r");
if (infp == NULL)
err(1, "unable to open: %s", infiles[i]);
infile = infiles[i];
rval |= decode();
fclose(infp);
}
free(infds);
oshogboUnsubmitted
Not Done
Inline Actions

This can be done in oneline.

oshogbo: This can be done in oneline.
free(infiles);
exit(rval); exit(rval);
} }
static int static int
decode(void) decode(void)
{ {
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());
else else
return (uu_decode()); return (uu_decode());
} }
v = decode2(); v = decode2();
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();
if (v == EOF) if (v == EOF)
break; break;
▲ Show 20 Lines • Show All 81 Lines • ▼ Show 20 Linesdecode2(void)
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 {
flags = O_WRONLY | O_CREAT | O_EXCL; flags = O_WRONLY | O_CREAT | O_EXCL;
if (lstat(outfile, &st) == 0) { if (fstatat(cwdfd, outfile, &st, AT_SYMLINK_NOFOLLOW) == 0) {
if (iflag) { if (iflag) {
warnc(EEXIST, "%s: %s", infile, outfile); warnc(EEXIST, "%s: %s", infile, outfile);
allanjudeAuthorUnsubmitted
Not Done
Inline Actions

missing space

allanjude: missing space
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(cwdfd, 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(cwdfd, 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());
else else
return (uu_decode()); return (uu_decode());
▲ Show 20 Lines • Show All 167 Lines • Show Last 20 Lines