freebsd security - Jun 2013 - Request for review: Sandboxing dhclient using Capsicum.

Hi.

I have a series of patches to sandbox dhclient using Capsicum
(capability mode and capability rights for descriptors).

As usual, because chroot and setgid/setuid are not sandboxing
mechanisms, there are many problems with the current sandboxing:
- Access to various global namespaces (like process list, network, etc.).
- Access to RAW UDP socket.
- Read/write access to bpf.
- Access to RAW route socket, which means it can delete, modify or add
  static routes as it pleases.

After the changes RAW route socket is limited to reading only,
write-only bpf descriptor and RAW UDP sockets are moved to privileged
process and eventhough unprivileged process controls destination
addresses still, it cannot change port for example. There is no access
to global namespaces anymore. All descriptors used by unprivileged
process are limited using capability rights (just in case, not really
crucial):
- Descriptor to lease file allows for overwrite only, but doesn't allow
  for other stuff, like reading, fchmod, etc.
- Descriptor to pidfile has no rights, it is just being kept open.
- STDIN descriptor has no rights.
- STDOUT and STDERR descriptors are limited to write only.

The patches are here. Every change has individual description:

	http://people.freebsd.org/~pjd/patches/dhclient_capsicum.patches

I'd appreciate any review, especially security audit of the proposed
changes. The new and most critical function is probably send_packet_priv().

-- 
Pawel Jakub Dawidek                       http://www.wheelsystems.com
FreeBSD committer                         http://www.FreeBSD.org
Am I Evil? Yes, I Am!                     http://mobter.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 196 bytes
Desc: not available
URL:
<http://lists.freebsd.org/pipermail/freebsd-security/attachments/20130609/55f938b7/attachment.sig>

On Sun, Jun 09, 2013 at 12:33:46AM +0200, Pawel Jakub Dawidek
wrote:
> I'd appreciate any review, especially security audit of the proposed
> changes. The new and most critical function is probably send_packet_priv().
I've looked over the diff and not found any significant issues, but have
a few comments in order of most to least important.

In change 229477 using a cached hostname may change behavior if the host
is renamed as a result of dhclient operation.  The new behavior might be
more correct, at least it would be if we reliably restored the host name
on termination.  I think the change is fine, but we should be keep an
eye out for problem reports.

In change 229476 I noticed there is a constant 0x1fff that you've moved
around.  It was already there, but it seems like an unnecessary magic
number.

In the send_packet_* function declarations in dhcpd.h, you have included
variable names which is inconsistent with the surrounding definitions.

I saw a few style/whitespace fixed mixed in that should be batched,
probably before the substantive commits.

-- Brooks