Bug 222258
| Summary: | renameat(2) capability error with absolute path names outside of a sandbox | ||||||
|---|---|---|---|---|---|---|---|
| Product: | Base System | Reporter: | Mathieu Arnold <mat> | ||||
| Component: | kern | Assignee: | Konstantin Belousov <kib> | ||||
| Status: | Closed FIXED | ||||||
| Severity: | Affects Many People | CC: | allanjude, bapt, cem, emaste, jilles, jonathan, mjg, ngie, oshogbo, rwatson, tony | ||||
| Priority: | --- | Keywords: | patch | ||||
| Version: | 11.0-RELEASE | ||||||
| Hardware: | Any | ||||||
| OS: | Any | ||||||
| See Also: |
https://reviews.freebsd.org/D19154 https://reviews.freebsd.org/D19096 |
||||||
| Bug Depends on: | |||||||
| Bug Blocks: | 231027 | ||||||
| Attachments: |
|
||||||
|
Description
Mathieu Arnold
2017-09-12 14:04:21 UTC
This is a little odd. The kernel implementation of rename(2) just invokes kern_renameat() with AT_FDCWD, like renameat() with AT_FDCWD. But perhaps Perl in *at() mode uses fds other than AT_FDCWD.
Looking at namei() and sys_capability.c, I don't see where we ever check if a thread is sandboxed or not. It seems like something like this might be needed, although I'm not sure if it is sufficient:
--- a/sys/kern/kern_descrip.c
+++ b/sys/kern/kern_descrip.c
@@ -2492,9 +2492,11 @@ fget_cap_locked(struct filedesc *fdp, int fd, cap_rights_t *needrightsp,
}
#ifdef CAPABILITIES
- error = cap_check(cap_rights_fde(fde), needrightsp);
- if (error != 0)
- goto out;
+ if (IN_CAPABILITY_MODE(curthread)) {
+ error = cap_check(cap_rights_fde(fde), needrightsp);
+ if (error != 0)
+ goto out;
+ }
#endif
if (havecapsp != NULL)
@@ -2593,9 +2595,11 @@ fget_unlocked(struct filedesc *fdp, int fd, cap_rights_t *needrightsp,
if (fp == NULL)
return (EBADF);
#ifdef CAPABILITIES
- error = cap_check(&haverights, needrightsp);
- if (error != 0)
- return (error);
+ if (IN_CAPABILITY_MODE(curthread)) {
+ error = cap_check(&haverights, needrightsp);
+ if (error != 0)
+ return (error);
+ }
#endif
count = fp->f_count;
retry:
Yes, the code grabs a handle to the cwd when opening the file for -i and uses that handle when cleaning up when the file is closed, in this specific case when renaming the original file to a backup file, or renaming the work file over the original file. If we were passing AT_FDCWD we could just call rename(). I have a workaround patch (for perl) that calls rename() for absolute paths on FreeBSD if Mathieu wants to try it on the tonyc/127663-freebsd-renameat branch in the perl5 git at git://perl5.git.perl.org/perl.git I do wonder if renameat() will behave correctly in a container on FreeBSD. It would be good to get some Capsicum reviewers for this change; something appears to be wrong here, but I'm not convinced the current patch is entirely correct. I will add some Capsicum folk to the PR. In my testing the failure only occurs when - the "to" file already exists - the "to" file is specified by an absolute path Running under ktrace I observed a bogus cap_rights_t triggering an assertion failure when running kdump, with my fix in https://reviews.freebsd.org/D12391 FWIW the failure comes from the cap_check() in kern_renameat(), sys/kern/vfs_syscalls.c:3515:
3509 #ifdef CAPABILITIES
3510 if (newfd != AT_FDCWD) {
3511 /*
3512 * If the target already exists we require CAP_UNLINKAT
3513 * from 'newfd'.
3514 */
3515 error = cap_check(&tond.ni_filecaps.fc_rights,
3516 cap_rights_init(&rights, CAP_UNLINKAT));
3517 if (error != 0)
3518 goto out;
3519 }
3520 #endif
(In reply to Ed Maste from comment #5) The block of code checking for CAP_UNLINKAT should not apply when an absolute path was originally passed to the system call. This is needed to maintain POSIX's requirement that renameat() be equivalent to rename() unless either old or new specifies a relative path. I don't immediately know how to code this best. The patch from Conrad Meyer seems wrong since capabilities are supposed to be enforced for all processes, not only ones in capability mode. This feature may be useful when passing file descriptors to a process with a lower privilege level. In any case, this seems a valid bug. (In reply to Jilles Tjoelker from comment #6) Agreed on both points: this is indeed a FreeBSD bug, and Conrad's patch isn't the right fix. namei() doesn't populate ndp->ni_filecaps when it performs an absolute path lookup (which is how we ended up with an invalid capability set, prompting D12391). This works for most cases of rights checks because the check is done in namei against the passed-in set of required rights (e.g., when unlinkat calls namei with CAP_UNLINKAT required), but not in the case here where an explicit check is performed afterwards in namei's caller. Of course this is irrelevant for capability mode because the absolute path is disallowed anyway. For non-capability mode I wonder if we need to fgetvp_rights also in the absolute path case? Now this is sketchy. So when you openat/whatever with an absolute path caps are not getting populated and the dir fd is not logged by audit. On the other hand the fd is not used in the lookup, so making its caps affect the outcome anyway may not be the right thing to do here. There is also potential crappery with startdir (used by nfs). I guess restructuring is the way to go here. Note there is a much-needed cleanup to do here anyway: audit code *duplicates* the logic used to determine starting vnodes. A commit references this bug: Author: emaste Date: Sun Sep 17 14:03:55 UTC 2017 New revision: 323675 URL: https://svnweb.freebsd.org/changeset/base/323675 Log: libsysdecode: report invalid cap_rights_t Previously we'd have an assertion failure in cap_rights_is_set if sysdecode_cap_rights is called with an invalid cap_rights_t, so test for validity first. PR: 222258 Reviewed by: cem MFC after: 1 month Sponsored by: The FreeBSD Foundation Differential Revision: https://reviews.freebsd.org/D12391 Changes: head/lib/libsysdecode/flags.c A commit references this bug: Author: mat Date: Mon Sep 18 10:41:26 UTC 2017 New revision: 450037 URL: https://svnweb.freebsd.org/changeset/ports/450037 Log: Update to v5.27.3-107-g4369267. A workaround for the renameat(2) bug[1] was added in https://github.com/Perl/perl5/commit/84dbe61c9, so use *at functions again. PR: 222258 [1] Changes: https://github.com/Perl/perl5/compare/v5.27.3-84-g7bd9fb1...v5.27.3-107-g4369267 Sponsored by: Absolight Changes: head/lang/perl5-devel/Makefile head/lang/perl5-devel/distinfo Over to committer. (In reply to Ed Maste from comment #4) To clarify, this issue requires all of the following to be true for it to manifest: - the "tofd" is an actual directory file descriptor, not AT_FDCWD - the "to" file already exists - the "to" file is specified by an absolute path A commit references this bug: Author: emaste Date: Sat Oct 7 20:20:02 UTC 2017 New revision: 324398 URL: https://svnweb.freebsd.org/changeset/base/324398 Log: MFC r323675: libsysdecode: report invalid cap_rights_t Previously we'd have an assertion failure in cap_rights_is_set if sysdecode_cap_rights is called with an invalid cap_rights_t, so test for validity first. PR: 222258 Sponsored by: The FreeBSD Foundation Changes: _U stable/11/ stable/11/lib/libsysdecode/flags.c A test for this case is now in the capsicum-test suite: https://github.com/google/capsicum-test/commit/7a79785948d50df4cd1db5a76b1619167af7856c [----------] 1 test from RenameAt [ RUN ] RenameAt.AbsDesignationSame rename.cc:42: Failure Expected: (0) <= (renameat(-100, src_path, dfd, src_path)), actual: 0 vs -1 errno 93 Capabilities insufficient rename.cc:44: Failure Expected: (0) <= (renameat(dfd, src_path, dfd, src_path)), actual: 0 vs -1 errno 93 Capabilities insufficient [ FAILED ] RenameAt.AbsDesignationSame (0 ms) [----------] 1 test from RenameAt (0 ms total) A commit references this bug: Author: kib Date: Fri Feb 8 04:18:18 UTC 2019 New revision: 343891 URL: https://svnweb.freebsd.org/changeset/base/343891 Log: Fix renameat(2) for CAPABILITIES kernels. When renameat(2) is used with: - absolute path for to; - tofd not set to AT_FDCWD; - the target exists kern_renameat() requires CAP_UNLINK capability on tofd, but corresponding namei ni_filecap is not initialized at all because the lookup is absolute. As result, the check was done against empty filecap and syscall fails erronously. Fix it by creating a return flags namei member and reporting if the lookup was absolute, then do not touch to.ni_filecaps at all. PR: 222258 Reviewed by: jilles, ngie Sponsored by: The FreeBSD Foundation MFC after: 1 week X-MFC-note: KBI breakage Differential revision: https://reviews.freebsd.org/D19096 Changes: head/sys/kern/vfs_lookup.c head/sys/kern/vfs_syscalls.c head/sys/sys/namei.h A commit references this bug: Author: ngie Date: Tue Feb 12 03:32:41 UTC 2019 New revision: 344041 URL: https://svnweb.freebsd.org/changeset/base/344041 Log: Bump `__FreeBSD_version__` for r343891 This will allow upstream consumers, e.g., capsicum-test and third-party packages (via ports(7)), to test for a specific `__FreeBSD_version__` and expect `renameat(2)` to be functional. PR: 222258 Approved by: emaste (mentor) Reviewed by: emaste MFC with: r343891 Differential Revision: https://reviews.freebsd.org/D19154 Changes: head/sys/sys/param.h Was merged to stable/12 in: commit 3fc4df5693d0a86142f2d0234f3c550893c3d11f Author: Konstantin Belousov <kib@FreeBSD.org> Date: Fri Feb 15 11:13:39 2019 +0000 MFC r343891: Fix renameat(2) for CAPABILITIES kernelsi. MFC Note: Layout of the struct nameidata is changed. I specifically decided to not move the new field to the end of the new structure since it would mostly make the corruption silent. __FreeBSD_version is bumped. Notes: svn path=/stable/12/; revision=344152 |