Open the zip archive and current directory then enter capability mode. All following syscalls have been altered to *at() variants relative to the current directory fd.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
| usr.bin/unzip/unzip.c | ||
|---|---|---|
| 283 | ||
| 284 | There's no need to set errno here. | |
| 674 | Does this approach mean that we have to open every single output file before doing any work? I'm worried that this approach will interact poorly with environments that enforce a small limit on the number of FDs, e.g., with setrlimit(2)/RLIMIT_NOFILES. poudriere does this, for example, to help catch descriptor leaks. Another drawback to this approach is that it requires us to parse the zip file before entering capability mode, which somewhat defeats the point. Looking at extract(), unzip already refuses to handle absolute paths or paths containing "..". Can we instead pre-open the root directory (either the CWD or the value of the -d option) and perform all subsequent operations using capsicum-friendly *at() syscalls? | |
| 985 | and we should check the return value. | |
Comment Actions
Open current directory, enter capability mode, then use *at() syscalls to extract archive files.
Comment Actions
This looks good! Three comments:
- I don't like that cdfd is a global variable. I'd rather see it plumbed everywhere that we pass a path, even though that's kind of onerous.
- I think readpassphrase() does not quite work in capability mode. See the implementation in lib/libc/gen/readpassphrase.c - it opens /dev/tty. It does have a fallback path, but I'm not sure how well that works. Could you please try writing a little standalone program that enters capability mode and tries to use readpassphrase()? Depending on how that goes, we may want to add a new variant of that function which takes fds from the caller.
- Have you tried testing with kern.trap_enotcap set to 1? That'll help catch any system calls that might be silently failing because we're in capability mode.
With the first and third points settled, I think we can request an exp-run for this change to see if it breaks anything in the ports tree.
| usr.bin/unzip/unzip.c | ||
|---|---|---|
| 999 | I think this can be caph_limit_stdio()? | |
Comment Actions
I am curious why cdfd would be better suited as a local variable? Namespace pollution doesn't seem to be an issue in a small userspace program and passing it would just add one extra argument to the stack for every function call. Is the idea to isolate its scope?
Comment Actions
Yeah, I guess it's not a big deal really. Unlike the path, we use the same directory descriptor everywhere so maybe it's not so bad. I'll leave it up to you.
Comment Actions
- cdfd is no longer a global variable. Instead, it is passed locally per function call.
- Open _PATH_TTY, limit its rights, and use readpassphraseat() instead of readpassphrase().
- Limit stdio instead of just stdin.
Comment Actions
The diff looks good.
I tried testing it with a few zip files I have lying around, and it seemed to work properly. Then, I tested with kern.trap_enotcap=1 and sadly hit a problem: libarchive calls iconv_open() after capability mode has been entered:
(gdb) bt #0 _open () at _open.S:4 #1 0x00000008011eda15 in __opendir2 (name=0x8011809f7 "/usr/lib/i18n", name@entry=0x80103656b <symlook_obj+331> "\204\300t\301H\213E\230\353\\1\322\367\366\213\004\221\276\003", flags=3) at /root/freebsd/lib/libc/gen/opendir.c:83 #2 opendir (name=name@entry=0x8011809f7 "/usr/lib/i18n") at /root/freebsd/lib/libc/gen/opendir.c:59 #3 0x00000008011ff0f5 in _findshlib (name=0x7fffffffd1d0 "libiconv_std", majorp=<optimized out>, minorp=<optimized out>) at /root/freebsd/lib/libc/iconv/citrus_module.c:198 #4 _citrus_load_module (rhandle=0x801c4c0a0, encname=<optimized out>) at /root/freebsd/lib/libc/iconv/citrus_module.c:297 #5 0x00000008011fd736 in open_shared (convname=0x7fffffffde40 "UTF-8/US-ASCII", src=0x7fffffffd640 "UTF-8", dst=0x7fffffffda40 "US-ASCII", rci=<optimized out>) at /root/freebsd/lib/libc/iconv/citrus_iconv.c:155 #6 get_shared (src=0x7fffffffd640 "UTF-8", dst=0x7fffffffda40 "US-ASCII", rci=<optimized out>) at /root/freebsd/lib/libc/iconv/citrus_iconv.c:240 #7 _citrus_iconv_open (rcv=0x7fffffffe280, src=<optimized out>, dst=<optimized out>) at /root/freebsd/lib/libc/iconv/citrus_iconv.c:327 #8 0x0000000801200c60 in __bsd___iconv_open (out=0x801186928 "US-ASCII", in=0x120004 <error: Cannot access memory at address 0x120004>, handle=0x0) at /root/freebsd/lib/libc/iconv/bsd_iconv.c:67 #9 __bsd_iconv_open (out=0x801186928 "US-ASCII", in=0x120004 <error: Cannot access memory at address 0x120004>) at /root/freebsd/lib/libc/iconv/bsd_iconv.c:84 #10 0x000000080110427e in create_sconv_object (fc=0x801085ffe "UTF-8", tc=0x801186928 "US-ASCII", current_codepage=<optimized out>, flag=582) at /root/freebsd/contrib/libarchive/libarchive/archive_string.c:1267 #11 get_sconv_object (a=0x801c1a000, a@entry=0x9, fc=0x801085ffe "UTF-8", fc@entry=0x801c1a000 <incomplete sequence \336>, tc=0x801186928 "US-ASCII", tc@entry=0x801c54048 "up\037", flag=6) at /root/freebsd/contrib/libarchive/libarchive/archive_string.c:1656 #12 0x0000000801104381 in archive_string_conversion_from_charset (a=0x8011809f7, charset=0x120004 <error: Cannot access memory at address 0x120004>, best_effort=best_effort@entry=1) at /root/freebsd/contrib/libarchive/libarchive/archive_string.c:1740 #13 0x0000000801100bc3 in process_extra (a=a@entry=0x801c1a000, entry=entry@entry=0x801c24000, p=p@entry=0x801c54048 "up\037", extra_length=35, extra_length@entry=26, zip_entry=0xfefefefefefefeff, zip_entry@entry=0x801c690a0) at /root/freebsd/contrib/libarchive/libarchive/archive_read_support_format_zip.c:806 #14 0x00000008010ff58a in slurp_central_directory (a=0x801c1a000, entry=0x801c24000, zip=0x801c2b800) at /root/freebsd/contrib/libarchive/libarchive/archive_read_support_format_zip.c:3894 #15 archive_read_format_zip_seekable_read_header (a=0x801c1a000, entry=0x801c24000) at /root/freebsd/contrib/libarchive/libarchive/archive_read_support_format_zip.c:4157 #16 0x00000008010bfbf7 in _archive_read_next_header2 (_a=0x801c1a000, entry=0x801c24000) at /root/freebsd/contrib/libarchive/libarchive/archive_read.c:647 #17 0x00000008010bfad0 in _archive_read_next_header (_a=0x8011809f7, entryp=0x7fffffffe528) at /root/freebsd/contrib/libarchive/libarchive/archive_read.c:685 #18 0x0000000001025599 in unzip (fn=fn@entry=0x7fffffffeb07 "../Photos-001.zip") at /usr/home/markj/src/freebsd/usr.bin/unzip/unzip.c:997 #19 0x0000000001024da2 in main (argc=2, argv=0x7fffffffe760) at /usr/home/markj/src/freebsd/usr.bin/unzip/unzip.c:1184
In particular, it wants to be able to open shared libraries under /usr/lib/i18n (or /usr/lib32/i18n for 32-bit compat executables, or the path specified by the PATH_I18NMODULE environment variable if set.
The general pattern for handling this kind of situation is to preopen the file, similar to how we handle time zone information for example. So, I tried writing a libc patch which adds __iconv_preopen(), which caches a directory descriptor for the library path. When I modify unzip to call this function before entering the sandbox, I don't see any more problems. (Really, libarchive should probably be calling it, but one step at a time.) The patches are here: https://github.com/markjdb/freebsd/commits/ff/capsicum-iconv
So I propose the following plan:
- Finish with the readpassphraseat() review.
- Put this rev, readpassphraseat(), and __iconv_preopen() patches in a branch, and request an exp-run. The idea being that an exp-run should exercise this change quite well (I count 851 ports that use unzip).
- Assuming all goes well, polish __iconv_preopen() (there are some TODOs listed in the commit message).
- Start committing the patches.