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

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.

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?

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.

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.

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.

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.

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.

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

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().

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?

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

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?

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

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.

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

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

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.

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.

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

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.

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.

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.

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