Page MenuHomeFreeBSD

Decompose linkat()/renameat() rights to source and target.
ClosedPublic

Authored by ed on Aug 18 2015, 2:56 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Nov 21, 12:22 PM
Unknown Object (File)
Fri, Nov 15, 5:49 AM
Unknown Object (File)
Mon, Nov 11, 2:58 AM
Unknown Object (File)
Sun, Nov 10, 8:38 PM
Unknown Object (File)
Sun, Nov 10, 4:52 PM
Unknown Object (File)
Tue, Nov 5, 9:55 AM
Unknown Object (File)
Tue, Nov 5, 9:50 AM
Unknown Object (File)
Tue, Oct 29, 1:56 AM

Details

Summary

To make it easier to understand how Capsicum interacts with linkat() and
renameat(), rename the rights to CAP_{LINK,RENAME}AT_{SOURCE,TARGET}.

This also addresses a shortcoming in Capsicum, where it isn't possible
to disable linking to files stored in a directory. Creating hardlinks
essentially makes it possible to access files with additional rights.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

ed retitled this revision from to Decompose linkat()/renameat() rights to source and target..
ed updated this object.
ed edited the test plan for this revision. (Show Details)
ed added reviewers: rwatson, jonathan, theraven.
drysdale_google.com added inline comments.
sys/sys/capsicum.h
153 ↗(On Diff #8030)

Jon, Ben and I thought we should probably add a warning comment for the LINKAT right, to indicate the potential danger of bypassing rights protections by linkat + re-openat.

Also, we failed to immediately think of [m]any applications that use link(2)/linkat(2) -- have you encountered some for CloudABI, maybe triggering this change?

214 ↗(On Diff #8030)

Hmm, this makes me think. On the Linux side, I've added a few new Linux-specific rights whose values will therefore clash with these new values. I don't think that clashing values is inherently a problem (unless FreeBSD's Linux support catches up with Linux 4.something-with-capsicum...), but clashing names could potentially be a problem.

So maybe we should make a pre-emptive stab at keeping the set of possible rights in sync. We could consider the FreeBSD capsicum.h to be the master list of rights, and add in the inert-to-FreeBSD Linux-specific names/values (and I'll add a comment to the Linux version of capsicum.h to indicate that the master list is here). Thoughts? Unnecessary?

sys/sys/capsicum.h
153 ↗(On Diff #8030)

I think that whether link(2) and linkat(2) are useful remains outside the scope of this change. These functions are simply present in POSIX. On UNIX, creating hardlinks is not something exceptional. Don't mail servers use link(2) and linkat(2) to mutate maildirs?

The same issues apply to renameat(2), so even if we were to block linkat(2) unconditionally, we would still need to fix up renameat(2).

A warning has already been added to the manual page.

214 ↗(On Diff #8030)

I'd personally say that would not be necessary. It is important to remain API compatible, but so far we don't really care about the ABI being cross-platform. The Linux and BSD ABIs already differ so wildly, it wouldn't be worth having the Capsicum ABI in sync.

The CloudABI Capsicum rights are already using a different numbering, for example.

sys/sys/capsicum.h
214 ↗(On Diff #8030)

Sorry for not being clear, I was thinking of API compatibility not ABI compatibility.

A theoretical example might be clearer: let's say a future FreeBSD developer decides to add epoll support, in a way that's vaguely compatible with Linux. How do they know that there's already a CAP_EPOLL_CTL right they can re-use, rather than inventing a new (say) CAP_EPOLL_MODIFY right -- and thus dooming epoll-using application developers to a world of ifdefs? Re-use of the name might be a little bit more likely if it's already there in the capsicum.h file. [This is all outside the scope of this change, of course...]

On the subject of ABI compatibility, though, is this a change that's just intended for 11.x? I'm guessing it should be, because it's effectively an ABI change -- a program that runs on (say) 10.1 could stop working on 10.x with this change. (This goes back to the point that was discussed on the Capsicum call a few weeks ago, that the rights-needed for various operations now forms part of the ABI contract.)

More generally, I should point out that when/if the Linux version is upstreamed then we probably won't be able to make changes like this -- Linux maintains ABI compatibility between all versions (unlike FreeBSD, which AIUI can change ABI between major versions). So if a Capsicum-aware userspace application works for Linux kernel n, it needs to continue working for all x>=n.

rwatson edited edge metadata.

This patch seems reasonable to me.

This revision is now accepted and ready to land.Aug 19 2015, 8:39 AM
sys/sys/capsicum.h
214 ↗(On Diff #8030)

That's a good question. I'm not the person in charge, but I do have some thoughts on this. On one hand, we could just add a comment to <sys/capsicum.h> saying: "if you're adding more bits, please check the implementations of $list as well". Or we could set up a registry that contains a union of all of the bits we'd want across operating systems.

My plan is to not MFC this change, by the way. I'll just leave it in 11.

sys/sys/capsicum.h
214 ↗(On Diff #8030)

Yeah, I think the general point needs more discussion -- probably elsewhere, as it's beyond the scope of this perfectly sensible change.

However, on the 10.x/11.x point, is it worth back-creating placeholder names for the new rights in 10.x, with a zero/ineffective value associated with them? That way, a userspace application could have (say) CAP_LINKAT_SOURCE in its code, and be compilable on 10.x/11.x/Linux without ifdefs.

E.g. something like:

/* Placeholder right value for forward compatibility */
#define CAP_NOOP CAPRIGHT(0, 0x00ULL)
...
/* Present in 11.x, has no effect on 10.x systems */
#define CAP_LINKAT_SOURCE CAP_NOOP
#define CAP_RENAMEAT_TARGET CAP_NOOP

wblock added a reviewer: wblock.
wblock added a subscriber: wblock.

The man page changes look okay to me. Please test with igor -R and mandoc -Tlint before commit. Thanks!

sys/sys/capsicum.h
214 ↗(On Diff #8030)

That sounds like a good idea. I'll push in this change into 11.0 right now and we'll discuss how we're going to do the forward compatibility in next week's meeting. :-)

This revision was automatically updated to reflect the committed changes.