Bug 222258 - renameat(2) capability error with absolute path names outside of a sandbox
Summary: renameat(2) capability error with absolute path names outside of a sandbox
Status: In Progress
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: 11.0-RELEASE
Hardware: Any Any
: --- Affects Many People
Assignee: Konstantin Belousov
URL:
Keywords: patch
Depends on:
Blocks: 231027
  Show dependency treegraph
 
Reported: 2017-09-12 14:04 UTC by Mathieu Arnold
Modified: 2019-02-12 03:33 UTC (History)
11 users (show)

See Also:


Attachments
kdump output (40.95 KB, text/plain)
2017-09-12 14:04 UTC, Mathieu Arnold
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Mathieu Arnold freebsd_committer 2017-09-12 14:04:21 UTC
Created attachment 186296 [details]
kdump output

This week's lang/perl5-devel update brought in a change in the way perl -i -ne 'foo' (equivalent of sed -i -e 'foo') works, it now uses renameat(2).

When using lang/perl5-devel from ports r449631, (and before ports r449684 where I told Perl not to use the *at functions,) this happens:

# perl5.27.4 -i.bak -ne 's/foo/bar/' /tmp/foo
Can't rename in-place work file '/tmp/f8BoL2A7' to '/tmp/foo': Capabilities insufficient

It works all right when given a relative path, but not at all if given an absolute one.

Attached is the output of ktrace that command.

(This is outside of any sandbox.)
Comment 1 Conrad Meyer freebsd_committer 2017-09-12 19:00:34 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:
Comment 2 tony 2017-09-14 00:02:58 UTC
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.
Comment 3 Robert Watson freebsd_committer 2017-09-15 14:15:50 UTC
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.
Comment 4 Ed Maste freebsd_committer 2017-09-16 01:41:59 UTC
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
Comment 5 Ed Maste freebsd_committer 2017-09-16 02:40:56 UTC
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
Comment 6 Jilles Tjoelker freebsd_committer 2017-09-16 13:18:33 UTC
(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.
Comment 7 Ed Maste freebsd_committer 2017-09-16 14:15:34 UTC
(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.
Comment 8 Ed Maste freebsd_committer 2017-09-16 16:02:00 UTC
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?
Comment 9 Mateusz Guzik freebsd_committer 2017-09-16 16:24:45 UTC
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.
Comment 10 commit-hook freebsd_committer 2017-09-17 14:04:32 UTC
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
Comment 11 commit-hook freebsd_committer 2017-09-18 10:41:33 UTC
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
Comment 12 Mark Linimon freebsd_committer freebsd_triage 2017-10-03 21:53:54 UTC
Over to committer.
Comment 13 Ed Maste freebsd_committer 2017-10-04 18:03:05 UTC
(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
Comment 14 commit-hook freebsd_committer 2017-10-07 20:20:26 UTC
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
Comment 15 Ed Maste freebsd_committer 2017-10-13 13:09:02 UTC
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)
Comment 16 commit-hook freebsd_committer 2019-02-08 04:19:12 UTC
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
Comment 17 commit-hook freebsd_committer 2019-02-12 03:33:08 UTC
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