Page MenuHomeFreeBSD

Add O_RESOLVE_BENEATH and AT_RESOLVE_BENEATH to mimic Linux' RESOLVE_BENEATH
ClosedPublic

Authored by kib on Jul 29 2020, 11:31 PM.

Details

Summary

It is like O_BENEATH, but disables to walk out of the subtree rooted in the starting directory. O_BENEATH does not care if path walks out if it returned.

Change O_BENEATH to handle relative paths same as absolute: do not care if path walks out if it returns.

Only clear latch for O_BENEATH when we walk out of the startdir, not unconditionally.

Check for latch even for the last component if it is a directory [*].

Stop abusing LI_NCF_STRICTRELATIVE outside vfs_lookup,c, LI_NCF should be only used during namei(). Add ni_resflags NIRES_STRICTREL to indicate the need to install file rights for kern_openat().

Update comment explaining why the tracker is needed at all.

Add at2cnpflags() to centralize convertion from AT_* flags to NAMEI flags.

PR: 248335
Reported by: pho [*]
Tested by: pho

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
kib requested review of this revision.Jul 29 2020, 11:31 PM
kib edited the summary of this revision. (Show Details)

Fix definition of AT_RBENEATH.

This patch survived simple testing.

I like the at2cnpf() function, I guess it can be committed separately.

So we permit the combination of O_BENEATH and O_RBENEATH, and it is the same as only O_RBENEATH?

sys/kern/vfs_lookup.c
1378 ↗(On Diff #75213)

I believe style prefers uint64_t to u_int64_t.

sys/kern/vfs_syscalls.c
117 ↗(On Diff #75213)

uint64_t

118 ↗(On Diff #75213)

at2cnpflags() might be more clear to a casual reader.

sys/sys/namei.h
137 ↗(On Diff #75213)

Why does the mask have to be split?

kib marked 4 inline comments as done.Aug 4 2020, 4:27 PM

So we permit the combination of O_BENEATH and O_RBENEATH, and it is the same as only O_RBENEATH?

No, look at the chunk in vfs_lookup.c.

sys/kern/vfs_lookup.c
420 ↗(On Diff #75213)

This check disallows BENEATH | RBENEATH, both for O_ and AT cases.

sys/kern/vfs_syscalls.c
117 ↗(On Diff #75213)

I kept the type of ck_flags. I think this should be same even syntaxically.

OTOH, I can change all of them.

sys/sys/namei.h
137 ↗(On Diff #75213)

There is no more bits in the MODMASK. Layout it: 2 bits for op, then 7 bits for modmask, then 22 bits parameters descriptors (see below).

I do not want to break this KBI (again).

kib marked 2 inline comments as done.

at2cnpflags, use uint64_t.

The patch is not against HEAD, I will rebase later.

Would O_BENEATH_STRICT be a better name?

Would O_BENEATH_STRICT be a better name?

The best name would be O_RESOLVE_BENEATH. I hesitated to do that because I am not 100% sure we are 100% compatible with Linux flag.
https://man7.org/linux/man-pages/man2/openat2.2.html

To close the loop, the man page referenced above states 'The semantics of RESOLVE_BENEATH were modeled after FreeBSD's O_BENEATH' sigh.

In D25886#576619, @kib wrote:

Would O_BENEATH_STRICT be a better name?

The best name would be O_RESOLVE_BENEATH. I hesitated to do that because I am not 100% sure we are 100% compatible with Linux flag.
https://man7.org/linux/man-pages/man2/openat2.2.html

I am a bit confused by O_BENEATH now. The man page says, "O_BENEATH returns ENOTCAPABLE if the specified relative path, after resolving all symlinks and ".." references, does not reside in the directory hierarchy of children beneath the topping directory." To me this implies that the following should succeed, but it fails with ENOTCAPABLE:

chdir("/tmp");
fd = open("../tmp/foo", O_RDWR | O_CREAT | O_BENEATH, 0666);
if (fd < 0)
    err(1, "open");

However, if I change the open() path to "/tmp/foo", it succeeds.

So it looks to me like the absolute and relative path handling is not really consistent. With relative paths namei() behaves like it would in a Capsicum sandbox (i.e., at all stages of lookup we are not allowed to traverse above the topping dir), and with absolute paths it is more flexible (only the result must be beneath the topping dir).

Rather than adding a flag to behave more like Linux, can we change the semantics for O_BENEATH+absolute paths, and require an extra flag to get the original behaviour? I do not remember the original motivation for r340343.

Alternately, I would relax O_BENEATH+relative paths to behave the same as O_BENEATH+absolute paths, and add O_RESOLVE_BENEATH. I am not yet certain about Linux compatibility either.

To close the loop, the man page referenced above states 'The semantics of RESOLVE_BENEATH were modeled after FreeBSD's O_BENEATH' sigh.

Uh, sorry, obviously my previous comment does not make much sense - of course O_BENEATH+absolute path has to do some resolution above the topping dir.

Rebase after recent namei.h changes.

kib edited the summary of this revision. (Show Details)

See updated summary.

kib retitled this revision from Add O_RBENEATH and AT_RBENEATH to mimic Linux' RESOLVE_BENEATH to Add O_RELATIVE_BENEATH and AT_RLEATIVE_BENEATH to mimic Linux' RESOLVE_BENEATH.Sep 12 2020, 6:57 PM
kib retitled this revision from Add O_RELATIVE_BENEATH and AT_RLEATIVE_BENEATH to mimic Linux' RESOLVE_BENEATH to Add O_RESOLVE_BENEATH and AT_RESOLVE_BENEATH to mimic Linux' RESOLVE_BENEATH.
markj added inline comments.
sys/kern/vfs_lookup.c
221 ↗(On Diff #76952)

s/walks/walk/

225 ↗(On Diff #76952)

This is not true by default (lookup_cap_dotdot_nonlocal defaults to 1).

420 ↗(On Diff #75213)

Hmm, doesn't it mean that O_BENEATH cannot be used with capability mode enabled, even if the lookup does not violate Capsicum's stricter rules? Maybe that's acceptable, though I would expect to see ENOTCAPABLE in that case.

sys/kern/vfs_vnops.c
301 ↗(On Diff #76952)

Perhaps factor the common bits for LOOKUP and CREATE into a subroutine.

This revision is now accepted and ready to land.Sep 14 2020, 7:28 PM
kib marked 4 inline comments as done.Sep 14 2020, 9:45 PM
kib added inline comments.
sys/kern/vfs_lookup.c
420 ↗(On Diff #75213)

IMO it would add to much clutter to the implementation. We can reconsider it later if asked for.

kib marked an inline comment as done.

Add man page description of O_RELATIVE_BENEATH.
Update description of O_BENEATH for relative paths.
Add comments in fcntl.h for flags definitions.
Add open2nameif().
Fix grammar in comments.

This revision now requires review to proceed.Sep 14 2020, 9:46 PM

Forgot to commit AT_RELATIVE_BENEATH man page additions.

lib/libc/sys/access.2
131 ↗(On Diff #77028)

"Only walks paths". Same below.

lib/libc/sys/open.2
340 ↗(On Diff #77028)

I think you could combine the two sentences by writing, "if any intermediate component of the specified relative path does not reside in the directory hierarchy beneath the topping directory." IMO it is a bit clearer. It may be worth noting that absolute paths are not permitted.

kib marked 2 inline comments as done.Sep 15 2020, 1:54 PM
kib added inline comments.
lib/libc/sys/open.2
340 ↗(On Diff #77028)

I still think that the second sentence is useful, I added note about abs paths there.

kib marked an inline comment as done.

Man pages improvements.

markj added inline comments.
sys/kern/vfs_vnops.c
302 ↗(On Diff #77050)

It looks like there is extra ws following |=.

This revision is now accepted and ready to land.Sep 15 2020, 1:58 PM
kib marked an inline comment as done.Sep 15 2020, 2:18 PM
kib added inline comments.
sys/kern/vfs_vnops.c
302 ↗(On Diff #77050)

It was a tab.

kib marked an inline comment as done.

Space nit.
Add comment for NIRES_STRICTREL.

This revision now requires review to proceed.Sep 15 2020, 2:18 PM
kib edited the summary of this revision. (Show Details)

Fix conversion from AT flags to NAMEI, for NOFOLLOW. It is a pseudo-flag, not FOLLOW.
Check last component for latch.

lib/libc/sys/open.2
561 ↗(On Diff #77343)

Isn't it O_RESOLVE_BENEATH? Ditto in other man page changes.

Looks ok to me aside from the man pages.

This revision is now accepted and ready to land.Sep 22 2020, 5:58 PM

Name the flags RESOLVE_BENEATH in the man pages.

This revision now requires review to proceed.Sep 22 2020, 6:00 PM
This revision was not accepted when it landed; it landed in state Needs Review.Sep 22 2020, 9:54 PM
This revision was automatically updated to reflect the committed changes.