Changeset View
Standalone View
sbin/dumpon/dumpon.c
Show First 20 Lines • Show All 42 Lines • ▼ Show 20 Lines | ||||||||||
#include <sys/cdefs.h> | #include <sys/cdefs.h> | |||||||||
__FBSDID("$FreeBSD$"); | __FBSDID("$FreeBSD$"); | |||||||||
#include <sys/param.h> | #include <sys/param.h> | |||||||||
#include <sys/capsicum.h> | #include <sys/capsicum.h> | |||||||||
#include <sys/disk.h> | #include <sys/disk.h> | |||||||||
#include <sys/socket.h> | #include <sys/socket.h> | |||||||||
#include <sys/sysctl.h> | #include <sys/sysctl.h> | |||||||||
#include <sys/wait.h> | ||||||||||
#include <assert.h> | #include <assert.h> | |||||||||
#include <capsicum_helpers.h> | #include <capsicum_helpers.h> | |||||||||
#include <err.h> | #include <err.h> | |||||||||
#include <errno.h> | #include <errno.h> | |||||||||
#include <fcntl.h> | #include <fcntl.h> | |||||||||
#include <ifaddrs.h> | #include <ifaddrs.h> | |||||||||
#include <netdb.h> | #include <netdb.h> | |||||||||
▲ Show 20 Lines • Show All 146 Lines • ▼ Show 20 Lines | check_size(int fd, const char *fn) | |||||||||
if (ioctl(fd, DIOCGMEDIASIZE, &mediasize) != 0) | if (ioctl(fd, DIOCGMEDIASIZE, &mediasize) != 0) | |||||||||
err(EX_OSERR, "%s: can't get size", fn); | err(EX_OSERR, "%s: can't get size", fn); | |||||||||
if ((uintmax_t)mediasize < (uintmax_t)physmem) | if ((uintmax_t)mediasize < (uintmax_t)physmem) | |||||||||
errx(EX_IOERR, "%s is smaller than physical memory", fn); | errx(EX_IOERR, "%s is smaller than physical memory", fn); | |||||||||
} | } | |||||||||
#ifdef HAVE_CRYPTO | #ifdef HAVE_CRYPTO | |||||||||
static void | static void | |||||||||
genkey(const char *pubkeyfile, struct diocskerneldump_arg *kdap) | _genkey(const char *pubkeyfile, struct diocskerneldump_arg *kdap) | |||||||||
{ | { | |||||||||
FILE *fp; | FILE *fp; | |||||||||
RSA *pubkey; | RSA *pubkey; | |||||||||
assert(pubkeyfile != NULL); | assert(pubkeyfile != NULL); | |||||||||
assert(kdap != NULL); | assert(kdap != NULL); | |||||||||
fp = NULL; | fp = NULL; | |||||||||
Show All 10 Lines | _genkey(const char *pubkeyfile, struct diocskerneldump_arg *kdap) | |||||||||
*/ | */ | |||||||||
#if OPENSSL_VERSION_NUMBER < 0x10100000L | #if OPENSSL_VERSION_NUMBER < 0x10100000L | |||||||||
{ | { | |||||||||
unsigned char c[1]; | unsigned char c[1]; | |||||||||
RAND_bytes(c, 1); | RAND_bytes(c, 1); | |||||||||
} | } | |||||||||
#endif | #endif | |||||||||
if (caph_enter() < 0) | if (caph_enter() < 0) | |||||||||
markj: We should indeed use caph_enter_casper() if `verbose` is true. Otherwise if someone tries to… | ||||||||||
Not Done Inline ActionsYou're right, currently without casper the sysctl error does display with -v. # truss obj/dumpon -v -k pubkey off 2>&1 | grep kern.shutdown.dumpdevname __sysctlbyname("kern.shutdown.dumpdevname",25,0x7fffffffda50,0x7fffffffd9b0,0x0,0) ERR#1 'Operation not permitted' Sysctl get 'kern.shutdown.dumpdevname' But if we change to caph_enter_casper() it will not enter capability mode and will lie that it was successful. So -v -k results in no capability mode. diff --git sbin/dumpon/Makefile sbin/dumpon/Makefile index fa80cc4efcd4..d0700ab16a89 100644 --- sbin/dumpon/Makefile +++ sbin/dumpon/Makefile @@ -10,12 +10,6 @@ LIBADD= crypto CFLAGS+=-DHAVE_CRYPTO .endif -.if ${MK_CASPER} != "no" -LIBADD+= casper -LIBADD+= cap_sysctl -CFLAGS+= -DWITH_CASPER -.endif - MAN= dumpon.8 .include <bsd.prog.mk> diff --git sbin/dumpon/dumpon.c sbin/dumpon/dumpon.c index 42e4941463ee..3b980f5cfd0e 100644 --- sbin/dumpon/dumpon.c +++ sbin/dumpon/dumpon.c @@ -268,8 +268,9 @@ genkey(const char *pubkeyfile, struct diocskerneldump_arg *kdap) CAP_SYSCTL_READ); if (cap_sysctl_limit(caplimit) < 0) err(1, "Unable to set system.sysctl limits"); - } - if (caph_enter() < 0) + if (caph_enter_casper() < 0) + err(1, "Unable to enter capability mode"); + } else if (cap_enter() < 0) err(1, "Unable to enter capability mode"); pubkey = RSA_new(); # truss obj/dumpon -v -k pubkey off 2>&1 | grep kern.shutdown.dumpdevname __sysctlbyname("kern.shutdown.dumpdevname",25,0x7fffffffda50,0x7fffffffd9b0,0x0,0) = 0 (0x0) I think if we do genkey() in a child process with shared memory or pipe to pass the data back we could avoid the problem. diff --git sbin/dumpon/dumpon.c sbin/dumpon/dumpon.c index 183ce5f08cb3..0378142a976f 100644 --- sbin/dumpon/dumpon.c +++ sbin/dumpon/dumpon.c @@ -48,6 +48,7 @@ __FBSDID("$FreeBSD$"); #include <sys/disk.h> #include <sys/socket.h> #include <sys/sysctl.h> +#include <sys/wait.h> #include <assert.h> #include <capsicum_helpers.h> @@ -210,7 +211,7 @@ check_size(int fd, const char *fn) #ifdef HAVE_CRYPTO static void -genkey(const char *pubkeyfile, struct diocskerneldump_arg *kdap) +_genkey(const char *pubkeyfile, struct diocskerneldump_arg *kdap) { FILE *fp; RSA *pubkey; @@ -305,6 +306,44 @@ genkey(const char *pubkeyfile, struct diocskerneldump_arg *kdap) } RSA_free(pubkey); } + +/* + * Run genkey() in a child so it can use capability mode without affecting + * the rest of the runtime. + */ +static void +genkey(const char *pubkeyfile, struct diocskerneldump_arg *kdap) +{ + pid_t pid; + int error, filedes[2], status; + + if (pipe2(filedes, O_CLOEXEC) != 0) + err(1, "pipe"); + pid = fork(); + switch (pid) { + case -1: + err(1, "fork"); + break; + case 0: + close(filedes[0]); + _genkey(pubkeyfile, kdap); + /* Write the new kdap back to the parent. */ + error = write(filedes[1], kdap, sizeof(*kdap)); + if (error != sizeof(*kdap)) + err(1, "genkey pipe write"); + exit(0); + } + close(filedes[1]); + /* Read in the child's genkey() result into kdap. */ + error = read(filedes[0], kdap, sizeof(*kdap)); + if (error != sizeof(*kdap)) + errx(1, "genkey pipe read"); + waitpid(pid, &status, WEXITED); + if (status != 0) + errx(1, "genkey child failed"); + close(filedes[0]); + return; +} #endif static void bdrewery: You're right, currently without casper the sysctl error does display with `-v`.
```
# truss… | ||||||||||
Not Done Inline Actions
If WITH_CASPER isn't defined, yes. In this case the error is not very important so it might be acceptable to use caph_enter() unconditionally and live with the inconvenience of -v -k raising an error. In general I think it's better to avoid the possibility of ENOTCAPABLE errors as much as possible, i.e., code should be clean with respect to kern.trap_enotcap=1. Moving the sensitive portion of the program into a separate process seems cleaner to me. I'd want @oshogbo and @def to comment on this though. markj: > But if we change to caph_enter_casper() it will not enter capability mode and will lie that… | ||||||||||
Not Done Inline ActionsI like the idea of moving _genkey to sepreate process. For me we - we should avoid of ENOTCAPABLE - if this error is raised it means that we did something wrong. I would suggest using pdfork() instead of fork. oshogbo: I like the idea of moving _genkey to sepreate process.
For me we - we should avoid of… | ||||||||||
Done Inline ActionsWhy pdfork? bdrewery: Why `pdfork`? | ||||||||||
Not Done Inline ActionsIt ensures that the child will be killed if the parent exits unexpectedly. I don't think that's a real concern right, but it's a nicer model to have. markj: It ensures that the child will be killed if the parent exits unexpectedly. I don't think that's… | ||||||||||
err(1, "Unable to enter capability mode"); | err(1, "Unable to enter capability mode"); | |||||||||
pubkey = RSA_new(); | pubkey = RSA_new(); | |||||||||
if (pubkey == NULL) { | if (pubkey == NULL) { | |||||||||
errx(1, "Unable to allocate an RSA structure: %s", | errx(1, "Unable to allocate an RSA structure: %s", | |||||||||
ERR_error_string(ERR_get_error(), NULL)); | ERR_error_string(ERR_get_error(), NULL)); | |||||||||
Not Done Inline ActionsWe only use (and close) capcas if verbose is specified. markj: We only use (and close) `capcas` if `verbose` is specified. | ||||||||||
} | } | |||||||||
pubkey = PEM_read_RSA_PUBKEY(fp, &pubkey, NULL, NULL); | pubkey = PEM_read_RSA_PUBKEY(fp, &pubkey, NULL, NULL); | |||||||||
fclose(fp); | fclose(fp); | |||||||||
fp = NULL; | fp = NULL; | |||||||||
if (pubkey == NULL) | if (pubkey == NULL) | |||||||||
errx(1, "Unable to read data from %s.", pubkeyfile); | errx(1, "Unable to read data from %s.", pubkeyfile); | |||||||||
▲ Show 20 Lines • Show All 44 Lines • ▼ Show 20 Lines | #endif | |||||||||
arc4random_buf(kdap->kda_key, sizeof(kdap->kda_key)); | arc4random_buf(kdap->kda_key, sizeof(kdap->kda_key)); | |||||||||
if (RSA_public_encrypt(sizeof(kdap->kda_key), kdap->kda_key, | if (RSA_public_encrypt(sizeof(kdap->kda_key), kdap->kda_key, | |||||||||
kdap->kda_encryptedkey, pubkey, | kdap->kda_encryptedkey, pubkey, | |||||||||
RSA_PKCS1_OAEP_PADDING) != (int)kdap->kda_encryptedkeysize) { | RSA_PKCS1_OAEP_PADDING) != (int)kdap->kda_encryptedkeysize) { | |||||||||
errx(1, "Unable to encrypt the one-time key: %s", | errx(1, "Unable to encrypt the one-time key: %s", | |||||||||
ERR_error_string(ERR_get_error(), NULL)); | ERR_error_string(ERR_get_error(), NULL)); | |||||||||
} | } | |||||||||
RSA_free(pubkey); | RSA_free(pubkey); | |||||||||
} | ||||||||||
/* | ||||||||||
* Run genkey() in a child so it can use capability mode without affecting | ||||||||||
* the rest of the runtime. | ||||||||||
*/ | ||||||||||
static void | ||||||||||
genkey(const char *pubkeyfile, struct diocskerneldump_arg *kdap) | ||||||||||
{ | ||||||||||
pid_t pid; | ||||||||||
int error, filedes[2], status; | ||||||||||
ssize_t bytes; | ||||||||||
if (pipe2(filedes, O_CLOEXEC) != 0) | ||||||||||
err(1, "pipe"); | ||||||||||
pid = fork(); | ||||||||||
switch (pid) { | ||||||||||
case -1: | ||||||||||
err(1, "fork"); | ||||||||||
break; | ||||||||||
case 0: | ||||||||||
close(filedes[0]); | ||||||||||
_genkey(pubkeyfile, kdap); | ||||||||||
/* Write the new kdap back to the parent. */ | ||||||||||
bytes = write(filedes[1], kdap, sizeof(*kdap)); | ||||||||||
if (bytes != sizeof(*kdap)) | ||||||||||
err(1, "genkey pipe write"); | ||||||||||
Done Inline Actions
markj: | ||||||||||
_exit(0); | ||||||||||
} | ||||||||||
close(filedes[1]); | ||||||||||
/* Read in the child's genkey() result into kdap. */ | ||||||||||
Done Inline Actionsread(2) returns a ssize_t. Same comment above for write(). markj: read(2) returns a ssize_t. Same comment above for write(). | ||||||||||
bytes = read(filedes[0], kdap, sizeof(*kdap)); | ||||||||||
if (bytes != sizeof(*kdap)) | ||||||||||
errx(1, "genkey pipe read"); | ||||||||||
error = waitpid(pid, &status, WEXITED); | ||||||||||
if (error == -1) | ||||||||||
err(1, "waitpid"); | ||||||||||
if (WIFEXITED(status) && WEXITSTATUS(status) != 0) | ||||||||||
errx(1, "genkey child exited with status %d", | ||||||||||
WEXITSTATUS(status)); | ||||||||||
else if (WIFSIGNALED(status)) | ||||||||||
errx(1, "genkey child exited with signal %d", | ||||||||||
WTERMSIG(status)); | ||||||||||
close(filedes[0]); | ||||||||||
Done Inline Actions
markj: | ||||||||||
} | } | |||||||||
#endif | #endif | |||||||||
static void | static void | |||||||||
listdumpdev(void) | listdumpdev(void) | |||||||||
{ | { | |||||||||
static char ip[200]; | static char ip[200]; | |||||||||
char dumpdev[PATH_MAX]; | char dumpdev[PATH_MAX]; | |||||||||
struct diocskerneldump_arg ndconf; | struct diocskerneldump_arg ndconf; | |||||||||
size_t len; | size_t len; | |||||||||
const char *sysctlname = "kern.shutdown.dumpdevname"; | const char *sysctlname = "kern.shutdown.dumpdevname"; | |||||||||
int fd; | int fd; | |||||||||
len = sizeof(dumpdev); | len = sizeof(dumpdev); | |||||||||
if (sysctlbyname(sysctlname, &dumpdev, &len, NULL, 0) != 0) { | if (sysctlbyname(sysctlname, &dumpdev, &len, NULL, 0) != 0) { | |||||||||
Not Done Inline ActionsWhat if -v is used without -k? markj: What if `-v` is used without `-k`? | ||||||||||
Done Inline ActionsGood point. bdrewery: Good point.
In the first revision it still worked because `caph_enter_casper()` silently failed… | ||||||||||
if (errno == ENOMEM) { | if (errno == ENOMEM) { | |||||||||
err(EX_OSERR, "Kernel returned too large of a buffer for '%s'\n", | err(EX_OSERR, "Kernel returned too large of a buffer for '%s'\n", | |||||||||
sysctlname); | sysctlname); | |||||||||
} else { | } else { | |||||||||
err(EX_OSERR, "Sysctl get '%s'\n", sysctlname); | err(EX_OSERR, "Sysctl get '%s'\n", sysctlname); | |||||||||
} | } | |||||||||
} | } | |||||||||
if (strlen(dumpdev) == 0) | if (strlen(dumpdev) == 0) | |||||||||
▲ Show 20 Lines • Show All 271 Lines • Show Last 20 Lines |
We should indeed use caph_enter_casper() if verbose is true. Otherwise if someone tries to build dumpon without casper support, dumpon -v will fail again.