syslogd: Open forwarding socket descriptors

[jfree]

Summary

Previously, when forwarding a message to a remote address, the target's
addrinfo was saved at config-parse-time. When message-deliver-time came,
the message's addrinfo was passed into sendmsg(2) and delivered by the
first available inet socket.

The use of sendmsg(2) is prohibited in Capsicum capability mode, so
sockets are now opened and connected to their remote peers at
config-parse-time when executing outside of the capability sandbox.

These sockets are then used with send(2), allowing forwarding to be
performed inside of the capability sandbox.

[markj]

This looks good to me, thanks. A couple of minor nits.

usr.sbin/syslogd/syslogd.c

367–372

Hopefully f->f_addr_fds == NULL implies f->f_num_addr_fds == 0, in which case there's no need to check this condition explicitly.

3056

It'd be nicer to use calloc() here IMO.

[jfree]

Address Mark's comments: use calloc() instead of malloc() for
memory allocations. This is done to avoid potential integer overflow
in total allocation size.

[jfree]

usr.sbin/syslogd/syslogd.c

367–372

Yes. f->f_addr_fds == NULL does imply f->f_num_addr_fds == 0

[markj]

usr.sbin/syslogd/syslogd_cap_config.c

218

Same here, it'd be nice to use calloc().

[jfree]

• Use calloc(3) instead of malloc(3) for array allocations.
• Add missing error checking for a memory allocation

[markj]

usr.sbin/syslogd/syslogd.c

1751

I'd suggest writing sockfd = f->f_addr_fds[0] here.

1770

The new getsockopt() and getpeername() calls are only needed to implement dprintf(), but that's a no-op unless debug printing is enabled. IMO it would be better to avoid this extra work unless Debug is true.

2679

3061

usr.sbin/syslogd/syslogd_cap_config.c

223

Not new with this diff, so can be fixed separately, but: can we use nvlist_take_descriptor_array() instead of duping? Same for the F_FILE case.

[jfree]

• Minor style changes
• Only call getsockopt() and getpeername() when Debug is non-zero

[jfree]

usr.sbin/syslogd/syslogd_cap_config.c

223

Not new with this diff, so can be fixed separately, but: can we use nvlist_take_descriptor_array() instead of duping? Same for the F_FILE case.

We could. I addressed this in response to your other comment about it.

I use nvlist_get_descriptor_array() here because nvlist_to_filed() and filed_to_nvlist() do not consume their arguments. i.e. the nvlist passed into nvlist_to_filed() is marked const. Using nvlist_take_descriptor_array() would modify that nvlist.

[markj]

usr.sbin/syslogd/syslogd.c

1807

Sorry I didn't catch this before - I believe you can still use sendmsg() (so as to avoid having to loop over the iovec), so long as msghdr.msg_name = NULL. That would be cleaner IMO.

[jfree]

usr.sbin/syslogd/syslogd.c

1807

Sorry I didn't catch this before - I believe you can still use sendmsg() (so as to avoid having to loop over the iovec), so long as msghdr.msg_name = NULL. That would be cleaner IMO.

Ah! I could've sworn I checked syscalls.master and didn't see CAPENABLED on sendmsg() before doing this. I guess I didn't. I will definitely use sendmsg() then.