On 11/08/2020 01:03, Jann Horn wrote: > On Tue, Aug 11, 2020 at 12:43 AM Mickaël Salaün <mic@xxxxxxxxxxx> wrote: >> On 10/08/2020 22:21, Al Viro wrote: >>> On Mon, Aug 10, 2020 at 10:11:53PM +0200, Mickaël Salaün wrote: >>>> It seems that there is no more complains nor questions. Do you want me >>>> to send another series to fix the order of the S-o-b in patch 7? >>> >>> There is a major question regarding the API design and the choice of >>> hooking that stuff on open(). And I have not heard anything resembling >>> a coherent answer. >> >> Hooking on open is a simple design that enables processes to check files >> they intend to open, before they open them. From an API point of view, >> this series extends openat2(2) with one simple flag: O_MAYEXEC. The >> enforcement is then subject to the system policy (e.g. mount points, >> file access rights, IMA, etc.). >> >> Checking on open enables to not open a file if it does not meet some >> requirements, the same way as if the path doesn't exist or (for whatever >> reasons, including execution permission) if access is denied. > > You can do exactly the same thing if you do the check in a separate > syscall though. > > And it provides a greater degree of flexibility; for example, you can > use it in combination with fopen() without having to modify the > internals of fopen() or having to use fdopen(). > >> It is a >> good practice to check as soon as possible such properties, and it may >> enables to avoid (user space) time-of-check to time-of-use (TOCTOU) >> attacks (i.e. misuse of already open resources). > > The assumption that security checks should happen as early as possible > can actually cause security problems. For example, because seccomp was > designed to do its checks as early as possible, including before > ptrace, we had an issue for a long time where the ptrace API could be > abused to bypass seccomp filters. > > Please don't decide that a check must be ordered first _just_ because > it is a security check. While that can be good for limiting attack > surface, it can also create issues when the idea is applied too > broadly. I'd be interested with such security issue examples. I hope that delaying checks will not be an issue for mechanisms such as IMA or IPE: https://lore.kernel.org/lkml/1544699060.6703.11.camel@xxxxxxxxxxxxx/ Any though Mimi, Deven, Chrome OS folks? > > I don't see how TOCTOU issues are relevant in any way here. If someone > can turn a script that is considered a trusted file into an untrusted > file and then maliciously change its contents, you're going to have > issues either way because the modifications could still happen after > openat(); if this was possible, the whole thing would kind of fall > apart. And if that isn't possible, I don't see any TOCTOU. Sure, and if the scripts are not protected in some way there is no point to check anything. > >> It is important to keep >> in mind that the use cases we are addressing consider that the (user >> space) script interpreters (or linkers) are trusted and unaltered (i.e. >> integrity/authenticity checked). These are similar sought defensive >> properties as for SUID/SGID binaries: attackers can still launch them >> with malicious inputs (e.g. file paths, file descriptors, environment >> variables, etc.), but the binaries can then have a way to check if they >> can extend their trust to some file paths. >> >> Checking file descriptors may help in some use cases, but not the ones >> motivating this series. > > It actually provides a superset of the functionality that your > existing patches provide. It also brings new issues with multiple file descriptor origins (e.g. memfd_create). > >> Checking (already) opened resources could be a >> *complementary* way to check execute permission, but it is not in the >> scope of this series.
