Re: [PATCH v3 seccomp 2/5] seccomp/cache: Add "emulator" to check if filter is constant allow

YiFei Zhu
2020-10-02T11:08:27Z

On Thu, Oct 1, 2020 at 4:05 PM Kees Cook <keescook@chromium.org> wrote:
> Right, but we depend on that test always doing the correct thing (and
> continuing to do so into the future). I'm looking at this from the
> perspective of future changes, maintenance, etc. I want the actions to
> match the design principles as closely as possible so that future
> evolutions of the code have lower risk to bugs causing security
> failures. Right now, the code is simple. I want to design this so that
> when it is complex, it will still fail toward safety in the face of
> bugs.
>
> I'd prefer this way because for the loop, the tests, and the results only
> make the bitmap more restrictive. The worst thing a bug in here can do is
> leave the bitmap unchanged (which is certainly bad), but it can't _undo_
> an earlier restriction.
>
> The proposed loop's leading test_bit() becomes only an optimization,
> rather than being required for policy enforcement.
>
> In other words, I prefer:
>
> inherit all prior prior bitmap restrictions
> for all syscalls
> if this filter not restricted
> continue
> set bitmap restricted
>
> within this loop (where the bulk of future logic may get added),
> the worse-case future bug-induced failure mode for the syscall
> bitmap is "skip *this* filter".
>
>
> Instead of:
>
> set bitmap all restricted
> for all syscalls
> if previous bitmap not restricted and
> filter not restricted
> set bitmap unrestricted
>
> within this loop the worst-case future bug-induced failure mode
> for the syscall bitmap is "skip *all* filters".
>
>
>
>
> Or, to reword again, this:
>
> retain restrictions from previous caching decisions
> for all syscalls
> [evaluate this filter, maybe continue]
> set restricted
>
> instead of:
>
> set new cache to all restricted
> for all syscalls
> [evaluate prior cache and this filter, maybe continue]
> set unrestricted
>
> I expect the future code changes for caching to be in the "evaluate"
> step, so I'd like the code designed to make things MORE restrictive not
> less from the start, and remove any prior cache state tests from the
> loop.
>
> At the end of the day I believe changing the design like this now lays
> the groundwork to the caching mechanism being more robust against having
> future bugs introduce security flaws.
>

I see. Makes sense. Thanks. Will do that in the v4

YiFei Zhu