Add O_RESOLVE_BENEATH and AT_RESOLVE_BENEATH to mimic Linux' RESOLVE_BENEATH

[kib]

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

[kib]

Fix definition of AT_RBENEATH.

This patch survived simple testing.

[markj]

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]

In D25886#575018, @markj wrote:

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]

at2cnpflags, use uint64_t.

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

[kib]

Rebase, fix conflicts.

[markj]

Would O_BENEATH_STRICT be a better name?

[kib]

In D25886#576437, @markj 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

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

[markj]

In D25886#576619, @kib wrote:

In D25886#576437, @markj 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.

[markj]

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.

[kib]

Rebase after recent namei.h changes.

[kib]

See updated summary.

[markj]

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.

[kib]

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]

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.

[kib]

Forgot to commit AT_RELATIVE_BENEATH man page additions.

[markj]

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]

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]

Man pages improvements.

[markj]

sys/kern/vfs_vnops.c

302 ↗

(On Diff #77050)

It looks like there is extra ws following |=.

[kib]

sys/kern/vfs_vnops.c

302 ↗

(On Diff #77050)

It was a tab.

[kib]

Space nit.
Add comment for NIRES_STRICTREL.

[kib]

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

[markj]

lib/libc/sys/open.2

561 ↗

(On Diff #77343)

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

[markj]

Looks ok to me aside from the man pages.

[kib]

Name the flags RESOLVE_BENEATH in the man pages.