Restricting path name lookup with openat2()

Posted Aug 23, 2019 4:55 UTC (Fri) by epa (subscriber, #39769)
Parent article: Restricting path name lookup with openat2()

It might be useful to have a flag which suppresses directory traversal altogether, that is, only lets you open something directly inside the current working directory.

Restricting path name lookup with openat2()

Posted Aug 23, 2019 12:00 UTC (Fri) by cyphar (subscriber, #110703) [Link] (6 responses)

That might be useful, though it could be done (without a hypothetical RESOLVE_NO_SUBDIRS) by checking whether the path contains a '/' in userspace and using O_NOFOLLOW (you wouldn't even need RESOLVE_NO_SYMLINKS).

Restricting path name lookup with openat2()

Posted Aug 23, 2019 12:38 UTC (Fri) by Paf (subscriber, #91811) [Link] (5 responses)

I think the idea of attacker controlled path is supposed to extend to full control over that blob of memory, so any user space checks are subject to races.

Restricting path name lookup with openat2()

Posted Aug 23, 2019 16:50 UTC (Fri) by NYKevin (subscriber, #129325) [Link] (4 responses)

Copy the shared buffer into an area of private memory, and thereafter only use the private buffer.

The only way an attacker can go after that is to 1) ptrace you or 2) already be running code in your process. If either of those is the case, then you've already lost.

Restricting path name lookup with openat2()

Posted Aug 23, 2019 20:05 UTC (Fri) by epa (subscriber, #39769) [Link] (3 responses)

Right, there are safe ways to do it, but a flag would be convenient, obvious, and relatively foolproof.

Restricting path name lookup with openat2()

Posted Aug 24, 2019 5:40 UTC (Sat) by cyphar (subscriber, #110703) [Link] (2 responses)

With the downside that using the syscall would be more frustrating -- users that don't care about resolution would need to pass an extra 0 all the time (and the upgrade_mask union would be quite ugly to use too). Not to mention that there is a limit on the number of syscall arguments (6), so in the future we would probably need to switch to a struct-based argument anyway. Linus also mentioned in an earlier review that he wasn't a fan of the variadic open(2) semantics -- so I would like to avoid carrying that syscall design forward as well.

This isn't all a hypothetical -- my first draft of the syscall did just add a new argument, and I discovered pretty quickly (while writing the selftests) that it was abysmal to actually use that interface. The fact that C zeroes out structs when you do designated initialisation makes using structs so much more straightforward here. All of that being said, I'm not married to the current interface at all. If the only concern people have with the patches is what the syscall looks like, I'm more than happy to change it.

Restricting path name lookup with openat2()

Posted Aug 24, 2019 8:00 UTC (Sat) by epa (subscriber, #39769) [Link] (1 responses)

When I said a ‘flag’ I didn’t mean it had to be an extra argument. A Boolean flag can be passed in lots of ways, including a bitmask of flags, and could be part of a struct. What I meant was, in addition to all the funky options mentioned in the article to stop following ‘magic links’ and so on, there could be one more to stop following any directory traversal whatever.

Restricting path name lookup with openat2()

Posted Aug 24, 2019 8:26 UTC (Sat) by cyphar (subscriber, #110703) [Link]

Oh, I'm really sorry -- I completely misread the rest of the comment thread I was responding to. I thought you were arguing for not having a struct *at all* (as someone else has suggested in a separate thread) and that's what I was talking about. Yes, a RESOLVE_NO_SUBDIRS (or whatever) could be useful -- though I'd prefer to land openat2() first and then we can work on extensions like that (I'm already worried enough that the patch touches too many things).