Page MenuHomeFreeBSD

dumpon: Fix -v causing error when configuring an encrypted dump
ClosedPublic

Authored by bdrewery on Jul 22 2021, 2:22 AM.

Details

Summary

If -v is specified when adding a new device then a full listing of
configured devices is displayed. This requires sysctl access which
genkey()'s use of capability mode was blocking permission to access.
This leads to both confusing console spam but also incorrectly returning
an error status even if no other had been encountered.

dumpon: Sysctl get 'kern.shutdown.dumpdevname': Operation not permitted

Sponsored by: Dell EMC

Test Plan
# Bad
# dumpon -v -k pubkey off
dumpon: Sysctl get 'kern.shutdown.dumpdevname'
: Operation not permitted
# dumpon -v -k pubkey ada3p3
dumpon: Sysctl get 'kern.shutdown.dumpdevname'
: Operation not permitted

# Fixed
# obj/dumpon -v -k pubkey off
kernel dumps on priority: device
0: /dev/null
# obj/dumpon -v -k pubkey ada3p3
kernel dumps on priority: device
0: ada3p3

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 40618
Build 37507: arc lint + arc unit

Event Timeline

sbin/dumpon/dumpon.c
340

What if -v is used without -k?

  • Fix -v without -k
  • Properly load casper
  • Fallback to proper capability mode if casper is not present

I really wish we had a sysctlfd which allowed one to pre-"open" a sysctl like we are doing here with /dev/netdump...

sbin/dumpon/dumpon.c
255

We only use (and close) capcas if verbose is specified.

sbin/dumpon/dumpon.c
340

Good point.
In the first revision it still worked because caph_enter_casper() silently failed to go into capability mode due to not having WITH_CASPER defined. That also makes cap_sysctlbyname use sysctlbyname. Using caph_enter() would have properly hit the missing channel assertion in cap_sysctlbyname.
The new version could be simpler if cap_sysctlbyname (the whole API) fell back to the normal functions if WITH_CASPER is loaded but a NULL channel is given but it would likely hide programming mistakes. The whole other mess of cap_* that required casper silently failed too. As in returning success and doing nothing. That was more surprising. I would have expected to have to wrap all of this in #ifdef WITH_CASPER rather than it half-way pretend to succeed.

  • capcas isn't needed unless -v is given

I really wish we had a sysctlfd which allowed one to pre-"open" a sysctl like we are doing here with /dev/netdump...

I had wanted the opposite. To allow a specific path to be opened later, avoiding path-specific global fd and only requiring a global path limit context. But consistency would be nice.

I really wish we had a sysctlfd which allowed one to pre-"open" a sysctl like we are doing here with /dev/netdump...

I had wanted the opposite. To allow a specific path to be opened later, avoiding path-specific global fd and only requiring a global path limit context. But consistency would be nice.

This notion of poking holes in the global namespace goes against the design of capsicum, though I agree it'd be more convenient for things like this and the argv.

sbin/dumpon/Makefile
13 ↗(On Diff #92640)

I think you need to make sure that casper is disabled in /rescue/dumpon. IIRC libcasper doesn't work properly in the crunched binary but I can't remember exactly why.

sbin/dumpon/dumpon.c
250–251

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.

sbin/dumpon/dumpon.c
250–251

You'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.
It seems to work. If you agree in principle I'll promote it to the review.
This is from baseline (no casper changes):

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
sbin/dumpon/dumpon.c
250–251

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.

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.

sbin/dumpon/Makefile
13 ↗(On Diff #92640)

Thats true.

sbin/dumpon/dumpon.c
250–251

I 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.

sbin/dumpon/dumpon.c
250–251

Why pdfork?

  • Switch to using fork rather than casper

Looks ok with the nits fixed.

sbin/dumpon/dumpon.c
250–251

It 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.

351
355

read(2) returns a ssize_t. Same comment above for write().

368
This revision is now accepted and ready to land.Jul 24 2021, 2:19 PM
bdrewery marked 3 inline comments as done.
  • Address feedback
This revision now requires review to proceed.Jul 24 2021, 7:24 PM
This revision is now accepted and ready to land.Jul 25 2021, 4:54 PM