Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Seccomp isn't applied if wine is already confined #99

Open
gasinvein opened this issue Oct 29, 2020 · 7 comments
Open

Seccomp isn't applied if wine is already confined #99

gasinvein opened this issue Oct 29, 2020 · 7 comments

Comments

@gasinvein
Copy link

gasinvein commented Oct 29, 2020

If wine process is already confined externally (e.g. with some container sandbox like flatpak, LXC, docker, etc), it's own seccomp filters are never applied.
It comes from this check:

if (!(ret = prctl(PR_GET_SECCOMP, 0, NULL, 0, 0)))

if (ret == 2)
TRACE("Seccomp filters already installed.\n");

Since seccomp filters are stackable and are optimized with BPF-JIT, I believe they should be applied unconditionally, and only skipped if there was an actual error (prctl manpage say that PR_GET_SECCOMP returns negative value on error).
See flathub/com.valvesoftware.Steam#608 for more details.

@gofman
Copy link
Contributor

gofman commented Nov 2, 2020

If to just remove the check as suggested, the (redundant) filters will be stacked further on each subprocess Wine starts (and it is quite common for games to launch a few processes in a chain before it gets to the actual game executable). I think this is something to be avoided for performance reasons at least, each native syscall (that is, actually 100.0% of syscalls), will go through the whole seccomp chain. That can be avoided of course by introducing some other way of checking that these filters were already set up by parent process (passing the information, as it is probably no straightforward way to query which seccomp filters are set without root permissions). But it is not immediately obvious to me if this complication is needed in Proton now.

Frankly I did not expect that such way of syscall emulation can work under flatpak or similar sandbox aiming for security boundary w/seccomp filtering before due to various reasons. Now when I looked up what the actual filters are (https://github.com/flatpak/flatpak/blob/master/common/flatpak-run.c), I think it probably can. Yet I would be hesitant to blindly adding some optimism into trying to set up such filters outside of explicitly supported configurations and without testing the thing. Please note also that this way of Windows syscall emulation in Proton is rather a workaround than a good long term solution.

FWIW, the other problems which I would expect from those filters I see in flatpak-run (not related to syscall emulation) are:

  • blocking ptrace (which is blocked for non-'devel' mode only, not sure how 'devel' mode is set) may break various things. ptrace is currently used in Wine for reading / writing other process memory under certain patterns, getting and setting debug registers. Some DRMs (and not necesarily only them) use that;
  • blocking modify_ldt will likely break 16 bit applications and some DRMs too.

@nanonyme
Copy link

nanonyme commented Nov 3, 2020

Aha, so the reason why it seemed not to have security impact is there was no deep subprocess chain? I think Flathub Steam package already allows --devel for above reasons. But right, let's try to have a more complete solution for upstreaming purpose.

@Erick555
Copy link

Erick555 commented Nov 3, 2020

There was some discussion about seccomp perf in lkml not so long time ago you may want to look at.

@gofman
Copy link
Contributor

gofman commented Nov 3, 2020

I have made an updated patch which doesn't check for seccomp filter presence but checks if the relevant filters are installed the other way, so the filters do not get stacked for spawned processes. The patch is in Wine-Staging: https://github.com/wine-staging/wine-staging/blob/master/patches/ntdll-Syscall_Emulation/0001-ntdll-Support-x86_64-syscall-emulation.patch

I am still not sure if Proton needs that, probably not at least until this gets some testing.

@nanonyme
Copy link

nanonyme commented Nov 3, 2020

So the beef is that we're building Wine through https://github.com/flathub/com.valvesoftware.Steam.CompatibilityTool.Proton/blob/beta/com.valvesoftware.Steam.CompatibilityTool.Proton.yml#L156 which gets used inside flatpak and our current patch https://github.com/flathub/com.valvesoftware.Steam.CompatibilityTool.Proton/blob/beta/patches/wine/seccomp-external-filter.patch is what was suggested through this PR. Seems we should probably take that new patch into use instead to promote testing it but it might take a bit of rebase work.

@gasinvein
Copy link
Author

@gofman Thanks for the patch. I've adopted it for our proton flatpak community build (see PR linked above). As of now, no regressions spotted (the previous fix was to just always apply seccomp filters), let's see how it goes.

@gasinvein
Copy link
Author

This issue was mostly fixed in 6.3, thanks to whoever did this. RDR2-specific filter still won't work, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants