Port openrsync to FreeBSD using capsicum

[borako.ozarslan_gmail.com]

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 [e-mail]

[emaste]

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]

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]

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]

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]

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]

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]

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]

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]

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?

[borako.ozarslan_gmail.com]

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]

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

[borako.ozarslan_gmail.com]

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

[borako.ozarslan_gmail.com]

main.c

487–499

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

[cem]

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]

socket.c

287

as earlier, caph_enter

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

[emaste]

cap_fileargs change committed in:

• rS346313
• rS346314
• rS346315 (main commit)
• rS346316 (update consumers)
• rS346318 (fix for GCC 4.2.1 archs)
• rS346321 (fix for fix for GCC 4.2.1 archs)

[asomers]

FYI I've proposed a GSoC project to capsicumize the fts(3) family of functions. That would enable finishing the OpenRsync port.
https://wiki.freebsd.org/SummerOfCodeIdeas?highlight=%28fts_open%29#Capsciumize_fts.283.29

[emaste]

blocks.c

21

For reference: @markj has been looking at endian.h

[emaste]

blocks.c

21

That was a draft comment from years ago that I accidentally submitted just now. @imp added include/endian.h in 30e0d2a51026830e5141ce3dd43854819bba910c

[markj]

blocks.c

21

I think @imp ended up doing that work, see commit 30e0d2a510268.