Page MenuHomeFreeBSD

Port openrsync to FreeBSD using capsicum
Needs ReviewPublic

Authored by borako.ozarslan_gmail.com on Feb 28 2019, 6:56 PM.

Details

Reviewers
emaste
Summary

Port openrsync to FreeBSD using capsicum. Note that currently it doesn't work with sending directories because there is no version of fts that's usable under capability mode such as fts_openat.

Submitted by: Bora Ozarslan borako.ozarslan@gmail.com

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

Also note for others arriving at this review: this is not suitable for upstreaming; we intentionally removed the existing OpenBSD pledge/unveil calls because the primary goal with this review is to investigate the approach and differences. If there is an upstream appetite for portable sandboxing we'll look at #ifdefs etc.

emaste added a subscriber: kib.Feb 28 2019, 7:30 PM

We can use caph_enter instead of the cap_enter / ENOSYS dance

mktemp.c
37–39

@kib we should probably #define O_DSYNC O_SYNC in sys/ headers?

emaste added subscribers: markj, cem.Feb 28 2019, 7:31 PM
emaste added a subscriber: oshogbo.Feb 28 2019, 7:34 PM
emaste added inline comments.
Makefile
30

Discussed with @oshogbo, in my opinion it's a bug that we need to -DWITH_CASPER to use our headers -- but an issue to be addressed outside of this review.

kib added inline comments.Feb 28 2019, 7:49 PM
Makefile
30

Having -O0 -g hardcoded is wrong (very wrong). And why not use normal freebsd build ?

mktemp.c
37–39

Formally defining O_DSYNC as O_SYNC is correct according to the POSIX requirements, but it is wrong on its intent. This is why we did not provided fake fdatasync(2) and I implemented real syscall for UFS.

On the other hand, #define O_RSYNC 0 is correct because we use rangelocks and never allow reader to see partial write.

emaste added inline comments.Feb 28 2019, 8:10 PM
Makefile
30

We'd certainly do that if importing into base; this is a patch against upstream's repo. Main goal in this phab review is to present the proposed approach to Capsicum use.

cem added inline comments.Feb 28 2019, 8:55 PM
blocks.c
21

This change is wrong on e.g., Linux.

It would probably make things easier if we just shipped a compatibility <endian.h> in FreeBSD that included sys/endian.h.

flist.c
18

Should this stuff be #ifdef __FreeBSD__?

531–533

What's the purpose of this change? It introduces a couple major behavioral differences:

  1. On error, caller's *fl is freed; it was not before.
  2. memcpy will be a NULL pointer dereference in the error case, where it was not before.
830

So, open + fstat and lstat are slightly different, right? Chiefly, lstat doesn't follow symlinks, but open() does. AFAIK we can't actually open symlinks as pseudo-files, even with openat + O_NOFOLLOW. So we're breaking the S_ISLNK() handling here (and in other places).

It seems what we need is a fileargs_stat() (with flags), or _lstat().

835–836

Rather than adding close(fd); to every early return path after this, I'd suggest breaking the if/else chain:

if ((fd = fileargs...) < 0) {
    ...
    return 0;
}
rc = fstat(fd, ...);
close(fd);
if (rc == -1) {
    ...
} else if ( ... ) {
1027

same issue with lstat != open+fstat here

1031–1035

Same issue with fd leakage here.

main.c
487–499

I don't understand why we replaced the fork + waitpid mechanism with pdfork + kevent here (this is two questions — pdfork doesn't necessitate kqueue).

socket.c
286–287

Seems like we could enter sandbox slightly earlier with CAP_CONNECT on the socket? May not matter too much.

emaste added inline comments.Feb 28 2019, 9:35 PM
blocks.c
21

Yes, I intentionally asked Bora to not worry about compatibility with other systems initially, just to understand the scope of the changes.

Is our sys/endian.h (mostly) equivalent to linux's endian.h?

flist.c
18

See earlier comment - we'll need to work with Kristaps to determine what sort of portability goo is appropriate upstream.

531–533

For this one we either want to add recallocarray to libc, or just provide it in an openbsd compat lib or local file.

cem added inline comments.Feb 28 2019, 10:20 PM
blocks.c
21

Yep, mostly included for the same macros/functions (be32toh, etc). Linux's endian.h lacks our *dec() and *enc() routines.

flist.c
531–533

We already have reallocarray in libc

emaste added inline comments.Feb 28 2019, 10:31 PM
flist.c
531–533

Right, this one is recallocarray - or perhaps you're pointing out the precedent of importing these sort of functions into libc?

main.c
487–499

waitpid is not allowed once you cap_enter. The only other way to waitpid + get the exit code is kevent that I found and I need the process descriptor for that.

cem added inline comments.Feb 28 2019, 10:52 PM
flist.c
531–533

Doh, I missed that c both times. Mea culpa. Yeah, +1 to importing it or otherwise implementing a shim, instead of this.

main.c
487–499

I'm confused where we cap_enter. Is it during rsync_client()?

I'm also confused why we don't allow waitpid in capability mode. Maybe historical artifact? Obviously, we expose the same information via kevent, so either it should be allowed, or kevent shouldn't be...

main.c
487–499

rsync_client() calls either rsync_sender() or rsync_receiver(). That's where we cap_enter. So technically yes, we're in capability mode after rsync_client.

This is the only reference I found as to why waitpid isn't allowed https://lists.cam.ac.uk/pipermail/cl-capsicum-discuss/2010-November/msg00001.html

main.c
487–499

There is also supposed to be a pdwait but that's not implemented.

cem added inline comments.Feb 28 2019, 11:20 PM
main.c
487–499

This email from Robert on that thread seems to suggest waitpid should just be allowed: https://lists.cam.ac.uk/pipermail/cl-capsicum-discuss/2010-November/msg00003.html

emaste added inline comments.Mar 5 2019, 1:50 AM
socket.c
287

as earlier, caph_enter

also we should leave as ERR() not ERRX(), the errno is valid and useful information

cap_fileargs change committed in: