syslogd: Log messages using libcasper

[jfree]

Summary

Some logging operations require access to external resources to
complete. Logging to F_WALL requires on-demand access to the user
accounting database. Logging to F_CONSOLE requires access to the
console. Logging to F_PIPE prompts execution of a command outside
of capability mode.

These operations cannot be performed in capability mode, so the
"p_open", "ttymsg", and "wallmsg" commands may be sent to libcasper to
circumvent these limitations.

[jfree]

Add cap_ttymsg() and update to avoid rebasing conflicts

[jfree]

Use cap_xfer_nvlist() to transfer nvlists in cap_wallmsg(). Previously, cap_send_nvlist() was used without cap_recv_nvliust(), leaving an extra nvlist on libcasper's queue. This resulted in future nvlist transfers to retreive the wrong result.

Use logerror() to log errors instead of err(). This makes debugging significantly easier.

[markj]

usr.sbin/syslogd/syslogd_cap_log.c

49

There's no need to malloc this temporary copy. You can return the iovec by value.

222

In principle we should avoid trusting the values syslogd gives us, since syslogd might be compromised somehow. In particular, we shouldn assume that iovcnt * sizeof(*iov) could overflow. A simple solution here is to use calloc() instead.

[jfree]

usr.sbin/syslogd/syslogd_cap_log.c

222

In principle we should avoid trusting the values syslogd gives us, since syslogd might be compromised somehow. In particular, we shouldn assume that iovcnt * sizeof(*iov) could overflow. A simple solution here is to use calloc() instead.

If syslogd manipulates iovcnt to be less than the true number of iovecs, then we will convert iovcnt number of nvlists into iovecs and have some extra, unused space from the memory allocation. It doesn't matter if that unused space is unitialized because libcasper just passes the same iovcnt into wallmsg(). That extra space would never be touched.

When syslogd manipulates iovcnt to be more than the true number of iovecs, libcasper will allocate more memory to store more iovecs, but we will end up indexing out of bounds with nvl_iov[]. We would just end up fetching data from non-existent nvlists, which would likely crash the program.

calloc() fixes situations where the iovecs have been corrupted on syslogd's side and iovcnt has not been manipulated, but there doesn't seem to be a good solution to fixing situations where iovcnt is manipulated. I don't think there is a good solution, but this prompts the question: how far can we go without trusting syslogd?

[jfree]

• Do not allocate a new iovlist in nvlist_to_iovec()
• Use calloc() when allocating memory for the iovlist array.

[markj]

usr.sbin/syslogd/syslogd_cap_log.c

214

nvlist_get_nvlist_array() will tell you how many array elements are present. That is, the line iovcnt = nvlist_get_number(nvlin, "iovec_count"); does nothing, I believe.

222

but we will end up indexing out of bounds with nvl_iov[]. We would just end up fetching data from non-existent nvlists, which would likely crash the program.

Most likely, but not necessarily. This kind of overflow is a classic source of security holes (hence mallocarray() from OpenBSD). We should, at least, make sure that syslogd cannot trigger memory safety bugs in a casper service. The service should crash deterministically or return an error in response to invalid input.

[jfree]

usr.sbin/syslogd/syslogd_cap_log.c

222

Most likely, but not necessarily. This kind of overflow is a classic source of security holes (hence mallocarray() from OpenBSD). We should, at least, make sure that syslogd cannot trigger memory safety bugs in a casper service. The service should crash deterministically or return an error in response to invalid input.

How do you determine if input is invalid? And I am very confident that the program would crash. Anytime an nvlist element is accessed that does not exist, the program will terminate from an assertion. I'm also fairly confident that accessing an uninitialized nvlist would raise some assertions itself.

[markj]

usr.sbin/syslogd/syslogd_cap_log.c

222

How do you determine if input is invalid?

I don't have a general definition. A value for iovcnt where iovcnt * sizeof(*iov) overflows is certainly invalid though.

I'm also fairly confident that accessing an uninitialized nvlist would raise some assertions itself.

Yes, we can be fairly sure that the program will crash if we take a random address and treat it as a pointer to an nvlist, but the point is that we are not certain (what if, for example, that address is a pointer to a recently freed nvlist?), and that uncertainty is where security holes lie. If we handle the possibility of overflow properly, then there's no need to reason about what might happen if the code starts accessing uninitialized memory.

This example is a bit pedantic. My point is just that the boundary between syslogd and its casper services is a privilege boundary, so code which transfers data across that boundary requires extra scrutiny.

[jfree]

Don't manually add the iovec count to the nvlist. The iovec count is fetched when getting the nvlist iovec array.

[jfree]

usr.sbin/syslogd/syslogd_cap_log.c

222

Yes, we can be fairly sure that the program will crash if we take a random address and treat it as a pointer to an nvlist, but the point is that we are not certain (what if, for example, that address is a pointer to a recently freed nvlist?), and that uncertainty is where security holes lie. If we handle the possibility of overflow properly, then there's no need to reason about what might happen if the code starts accessing uninitialized memory.

This example is a bit pedantic. My point is just that the boundary between syslogd and its casper services is a privilege boundary, so code which transfers data across that boundary requires extra scrutiny.

Fair enough. Like you said, these are the kinds of uncertainties that can lead to security exploits. I totally acknowledge that the data transfer between syslogd and libcasper is a point of security concern, I just still have no idea how you would validate iovcnt. Even checking for arithmetic overflow like mallocarray() does not ensure that syslogd isn't just lying to us by passing an invalid iovcnt.

[jfree]

Add assertions to guarantee that the iovec count returned by nvlist_get_nvlist_array() is less or equal to than TTYMSG_IOV_MAX, the maximum amount of iovecs allowed by ttymsg().

Also, add assertions to make sure the binary sizes from nvlist_get_binary() are correct.

[markj]

usr.sbin/syslogd/syslogd.c

2091–2092

Now this code is called in the context of the casper service rather than syslogd. Will logerror() behave as expected?

usr.sbin/syslogd/syslogd_cap_log.c

210

Rather than passing the whole filed, syslogd should instead use an integer (say, index in the filed list) to refer to it. Then 1) the whole filed doesn't need to be copied, 2) we don't have to worry about validating the filed structure. Basically, the same thing you did with p_open().

[jfree]

Create a new cap_filed structure and accompanying cfiled SLIST for filed integrity verification in libcasper's cap_p_open().

[jfree]

usr.sbin/syslogd/syslogd_cap_log.c

210

Rather than passing the whole filed, syslogd should instead use an integer (say, index in the filed list) to refer to it. Then 1) the whole filed doesn't need to be copied, 2) we don't have to worry about validating the filed structure. Basically, the same thing you did with p_open().

This would require storing the f_uname and f_type fields in cap_filed. Is this okay?

[jfree]

Bumping this

[markj]

usr.sbin/syslogd/syslogd.c

1830

I wonder if you could write some regression tests which exercise the cases:

1. the pipe process exits at some point, forcing a subsequent log message to re-create the pipe after writev() returns an error
2. the p_open() call fails somehow

2076

Does wallmsg() get called from within this file? Would it make more sense to move it to the casper service file, or into a separate C file?

3436–3440

I would expand this comment to note that there's a casper service which wraps this.

3444

I think the first parameter can simply be dropped. The p_open() wrapper macro for the !CASPER case already drops it, so I think it won't compile anyway.

usr.sbin/syslogd/syslogd_cap.h

52

I don't quite understand what this comment is referring to.

usr.sbin/syslogd/syslogd_cap_log.c

50

Can you use nvlist_take_string() to avoid the extra memory allocation here?

51

What's the purpose of passing a length when we can already get that from the string? I'd probably just avoid pretending that we're passing iovecs around at all. It feels a bit strange, since that sounds like we're passing pointers between processes, which of course doesn't work. Or, at least add a comment explaining that we're abusing the notion of iovec here a bit.

216

assert() can be compiled out. sz is untrusted (it comes from the sandbox), so we should validate it properly. The same applies to some other assertions in this file.

[jfree]

usr.sbin/syslogd/syslogd.c

2076

Does wallmsg() get called from within this file? Would it make more sense to move it to the casper service file, or into a separate C file?

Yes, wallmsg() is called within syslogd.c

3444

I think the first parameter can simply be dropped. The p_open() wrapper macro for the !CASPER case already drops it, so I think it won't compile anyway.

Good catch. Not sure what happened there. Arguments have been fixed.

usr.sbin/syslogd/syslogd_cap.h

52

I don't quite understand what this comment is referring to.

It is referring to the cap_filed structure, which is supposed to hold information that verifies the integrity of a particular filed based on its pipe_cmd and index in the fhead list. Perhaps the wording could be a little bit more clear.

usr.sbin/syslogd/syslogd_cap_log.c

50

The nvlist_to_iovec() routine does not consume/alter the nvlist that is passed in. This is indicated by nvl being const. Using nvlist_take_string() would modify the state of nvl.

51

What's the purpose of passing a length when we can already get that from the string? I'd probably just avoid pretending that we're passing iovecs around at all. It feels a bit strange, since that sounds like we're passing pointers between processes, which of course doesn't work. Or, at least add a comment explaining that we're abusing the notion of iovec here a bit.

I agree. This is incredibly confusing; I even had a hard time understanding what was going on and I wrote it. Changing this now...

[jfree]

Address Mark's less complicated comments