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

Allow applications to make use of modify_ldt syscall #4297

Closed
alaviss opened this issue May 31, 2021 · 17 comments · Fixed by #5082
Closed

Allow applications to make use of modify_ldt syscall #4297

alaviss opened this issue May 31, 2021 · 17 comments · Fixed by #5082

Comments

@alaviss
Copy link
Contributor

alaviss commented May 31, 2021

Linux distribution and version

Gentoo

Flatpak version

Flatpak 1.10.0

Description of the problem

Some variants of Wine requires this syscall, see: flathub/net.lutris.Lutris#85

@HarryMichal
Copy link

This prevents Wine from running inside of a flatpak.

@mwleeds
Copy link
Collaborator

mwleeds commented Oct 29, 2021

The comment in the source says "modify_ldt is a historic source of interesting information leaks" so it doesn't make sense to just stop blocking it to fix Wine, without fully understanding the security implications of that.

@HarryMichal
Copy link

The comment in the source says "modify_ldt is a historic source of interesting information leaks" so it doesn't make sense to just stop blocking it to fix Wine, without fully understanding the security implications of that.

Oh, I completely missed that part... Apologies for the needless bump, then. 🙏

@xM8WVqaG
Copy link

As documented in source, this seccomp rule and it's associated comment is copied verbatim from an upstream project.

For some more context on a source of this rule:

CVE-2015-3290, 5157: Bugs in the kernel's non-maskable interrupt handling allowed privilege escalation. Can't be exploited on Sandstorm because the modify_ldt() system call is blocked using seccomp.

-- sandstorm-io/sandstorm/docs/using/security-non-events.md

Although the linked CVEs have been patched, the comment suggests LDTs have posed issues before these CVEs.

Unfortunately modify_ldt() isn't just used in unusual patched versions of wine (wine-lol from the OP). This rule also blocks running other older 16-bit software under Flatpak via unpatched wine. It would be convenient if it were possible to opt-in to enabling this system call for this usecase.

@mortenfc
Copy link

mortenfc commented Jul 3, 2022

I guess the security argument comes from making it a bit harder to pack viruses?
In my humble opinion that's not worth the cost. People shouldn't download random flatpaks.

@M-Reimer
Copy link

M-Reimer commented Aug 4, 2022

Actually it's kind of sad to see this issue hanging around for so long. Flatpak is really cool stuff which could really help simplifying software deployment for Linux. And now we are on a point where other tools with similar goals, like Lutris, are trying to get their way into flathub. Maybe that League of Legends case is just an "edge case" but there are classics on Lutris that may require 16 bit support in the Wine version behind it.

Currently modify_ldt is blocked by flatpak because of being "a historic source of interesting information leaks". Are the issues behind that still in existence? Also the quoted CVE above is fixed since years, right? I totally agree that allowing only a minimum of permissions is a good idea. Most people won't need 16 bit anymore. Almost all software developed in the last few decades is at least 32 bit. But for the people that want to ship an application that needs this support, there should be a (default turned off) checkbox to enable "16 bit support". If you really think this is necessary you could add a warning next to this checkbox and maybe place it to some "advanced settings" place.

@andrewathalye
Copy link

Seconding M-Reimer, I believe it would be very useful to see a flatpak configuration option that allows modify_ldt (disabled by default, of course).

@GloriousEggroll
Copy link

GloriousEggroll commented Sep 4, 2022

As documented in source, this seccomp rule and it's associated comment is copied verbatim from an upstream project.

For some more context on a source of this rule:

CVE-2015-3290, 5157: Bugs in the kernel's non-maskable interrupt handling allowed privilege escalation. Can't be exploited on Sandstorm because the modify_ldt() system call is blocked using seccomp.

-- sandstorm-io/sandstorm/docs/using/security-non-events.md

Although the linked CVEs have been patched, the comment suggests LDTs have posed issues before these CVEs.

Unfortunately modify_ldt() isn't just used in unusual patched versions of wine (wine-lol from the OP). This rule also blocks running other older 16-bit software under Flatpak via unpatched wine. It would be convenient if it were possible to opt-in to enabling this system call for this usecase.

I feel the CVEs involved should be handled(patched) by the software that the CVE reports exposes them, and if as you say these have already been patched, it's not the place of flatpak to block LDTs.

Let's look at the classic log4j as an example. If the CVE is exposed in the log4j binary, is it flatpak's responsibility to block it? No, I don't think so. It should be patched within log4j like it was.

If flatpak's goal is to act as a cross distro library agnostic translation layer I don't think things like this should be blocked, it's not flatpak's responsibility to mitigate and/or patch other party's software.

Furthermore this quote:
"modify_ldt is a historic source of interesting information leaks" is fear mongering. If there -is- some kind of information leak it should be properly exposed and patched within the software exposing it, not hidden away, especially not hidden away by another software that is unrelated to it.

I feel this should just be enabled. I don't think a toggle is a viable option as it's just going to be yet another thing new users have no clue about and have to spend x amount of time digging up or googling or reading docs on, then entering X Y Z commands blindly in the terminal and possibly screwing up their flatpak install.

GloriousEggroll added a commit to GloriousEggroll/flatpak that referenced this issue Sep 4, 2022
resolves flatpak#4297

Reasoning explained in detail here:
flatpak#4297 (comment)

Signed-off-by: Thomas Crider <gloriouseggroll@gmail.com>
@smcv
Copy link
Collaborator

smcv commented Sep 5, 2022

I'm sympathetic to the need to have a way to make Wine work in Flatpak apps, but I think we cannot do this without more context on what this syscall even does, and why it's blocked. Please don't assume that all Flatpak maintainers understand the finer points of 16-bit x86 code!

I've done some archaeology in the git history, and this has been here since b1aa93a "Use seccomp to limit allowed syscalls" where it was originally added by @alexlarsson, back in the days of xdg-app (before the rename to Flatpak).

If flatpak's goal is to act as a cross distro library agnostic translation layer I don't think things like this should be blocked, it's not flatpak's responsibility to mitigate and/or patch other party's software.

At a high level, Flatpak has two goals:

  • cross-distro runtime library stacks so apps can be portable between distros
  • sandboxing apps and limiting them to accessing only their own data, analogous to what happens on Android and iOS, so that it's at least somewhat safe to run apps that have not been exhaustively audited by a distro or sysadmin

Those goals sometimes support each other, but are sometimes in conflict. Some people (approximately those who consider Flatpak to be an alternative to something like 0install, Lutris, GNU Stow or BSD ports) only want the cross-distro runtime library stack and are frustrated by the sandboxing, while others (approximately those who consider Flatpak to be an alternative to something like Firejail) only want the sandboxing and are frustrated by the need to use a separate library stack. (Equally, some people think Flatpak's sandboxing doesn't go far enough, and consider each new way to relax the sandboxing to be a bug.)

I feel the CVEs involved should be handled(patched) by the software that the CVE reports exposes them, and if as you say these have already been patched, it's not the place of flatpak to block LDTs.

Yes and no, it's a bit more complicated than that.

Part of what Flatpak does is to put up a security boundary, which forbids things that would break the security model that it was designed for. For instance, its security model is that non-sandboxed apps on the host system must be able to identify a sandboxed app as being sandboxed, which is why we can't allow non-sandboxed apps to call clone3() (if they could, then they'd be able to change their own root directory and hide /.flatpak-info). In cases like this, even if all system components are working perfectly, we need to forbid the action because it would break our security model. This is limiting, but nicely straightforward.

However, another part of what Flatpak does is to protect the kernel from being attacked by a malicious process ("security hardening" rather than "being a security boundary"). The goal of this is to limit the damage if a Flatpak app is either malicious (perhaps an attacker gains control over a Flatpak app maintainer's credentials and pushes malicious code to Flathub or equivalent, or perhaps the Flatpak app maintainer is the attacker), or compromised (if the Flatpak app contains a library that has an arbitrary-code-execution vulnerability - perhaps an image or video library - then it can end up running attacker-controlled code).

And, yes, in this scenario it's easy to say "you shouldn't have trusted that app maintainer" or "you shouldn't have used that image library" or "you shouldn't have run a kernel where an obscure syscall can be exploited to do bad things" or whatever - but code is not perfect, neither are people, and both mistakes and malice do happen (it's a matter of when, not if). If we have some security hardening in place that makes a vulnerability harder to exploit, then that hopefully buys distros, sysadmins and users enough time to upgrade to a fixed version before damage is done.

As far as I can tell, modify_ldt() is one of the syscalls that are blocked as a security hardening measure, rather than as a security boundary. perf_event_open() is another. Allowing or blocking these syscalls is a risk/benefit tradeoff, rather than an absolute thing. Each syscall that we expose gives the kernel a larger attack surface than if we had not exposed the syscall, which means there's a higher risk of an app being able to exploit a kernel security vulnerability that involves that syscall. For most syscalls, the benefit of allowing the syscall is high (because "most" apps need it) and the risk is relatively low, so we allow them; but for syscalls that are either particularly rare or particularly scary, we can end up with the risk outweighing the benefit.

You are right to say that the attacks we're preventing with this hardening are "really" kernel security vulnerabilities rather than being Flatpak's fault, which means Flatpak is not really responsible for blocking them; but at the same time, the kernel is large enough and complicated enough that at any given time, it probably has some known vulnerabilities that your distro has not been able to patch yet (and maybe upstream hasn't been able to patch them either). We want Flatpak to be something that people can feel justifiably confident about installing and using, and that's a property of the overall system (the OS and Flatpak together).

In this issue, people have pointed out that modify_ldt() is needed for ... something, in Wine. That increases the benefit of this syscall to a point where it could well be outweighing the risk. Please could someone who is good at x86 describe in a bit more detail what it is needed for? Is it to run 16-bit Windows executables, or is there something more?

To enable that use, one possible route is to allow it globally, for all apps, as proposed in #5079.

The other possible route is what people talked about earlier in this issue: gating it behind some sort of flag. I think the most appropriate would probably be a new --allow flag, perhaps --allow=16bit or --allow=wine or --allow=emulation or something like that (which maintainers of apps like Steam and Lutris would add to their apps, but most apps would continue to not have). Mechanically, it would end up very similar to --allow=devel, which opens up ptrace(), personality() and perf_event_open(), all of which are normally blocked as a hardening measure (because, similarly, "most" apps don't need them, and they're particularly "powerful" syscalls to be giving attackers access to, so the benefit of allowing them is lower than usual and the risk is higher).

I don't know enough about the finer points of x86 backwards-compatibility to be the only one looking at the risk/benefit trade-off here, so I cannot say which of those routes is the better one. (But perhaps other maintainers can?)

@smcv
Copy link
Collaborator

smcv commented Sep 5, 2022

If flatpak's goal is to act as a cross distro library agnostic translation layer I don't think things like this should be blocked

One way to look at this is:

Some software authors just want to be able to use Flatpak as a cross-distro runtime environment, and are uninterested in being sandboxed: "I trust myself, I trust my codebase, and everyone else should too".

However, even for those authors, it is to their advantage for Flatpak to be something that users, sysadmins and distros consider to be safe and trustworthy, and having a useful level of sandboxing and security hardening is something that contributes to that. If users/sysadmins/distros don't trust Flatpak to be a safe thing to use, then they won't install or use it, which immediately makes it a less useful cross-distro runtime environment, because it can no longer reach those users/sysadmins/distros.

I'm not saying that this specific piece of security hardening is necessarily pulling its weight - maybe it is, maybe it isn't, I don't have enough context to say one way or another. However, the way we should make this decision is more like "which is bigger, the risk or the benefit?" rather than trying to set up a false dichotomy where each piece of security hardening is either essential or useless.

@smcv
Copy link
Collaborator

smcv commented Sep 5, 2022

"modify_ldt is a historic source of interesting information leaks" is fear mongering. If there -is- some kind of information leak it should be properly exposed and patched within the software exposing it, not hidden away, especially not hidden away by another software that is unrelated to it

I don't have enough historical context to know what specific information leak this is talking about. Can someone help?

(My best guess would be that it lets one process get information about the memory layout of another process, which is helpful to attackers who want to turn vulnerabilities in things like image decoders from a crash into an arbitrary code execution by crafting the contents of memory to include valid pointer values.)

There seem to have been attempts in the past to patch modify_ldt()-related attacks "properly", in the kernel, by disabling the syscall entirely, either on 64-bit kernels or everywhere (example, example). While you're right to say that this would be perhaps a "more correct" place to put the hardening, that's not going to help the affected versions of Wine.

@Erick555
Copy link
Contributor

Erick555 commented Sep 5, 2022

Please could someone who is good at x86 describe in a bit more detail what it is needed for? Is it to run 16-bit Windows executables, or is there something more?

I think examples were already mentioned in the thread before:

  • running true 16 bit windows apps through normal wine
  • using wine fork which uses this syscall to bypass anti-cheat mechanism in league of legends

I think this syscall was considered so obscure that nobody would ever use it except actual malware thus blocking it had no cost. But this assumption is challenged here. IMO blocking this syscall has pretty marginal security impact.

@Erick555
Copy link
Contributor

Erick555 commented Sep 5, 2022

I think having separate --allow option is too much of slice and dice for such obscure feature and including it in --allow=multiarch may make more sense. This way multiarch won't only mean 32bit support but both 32bit &16bit support which is still consistent with its name.

multiarch is in ~99% for wine & friends anyway so for apps this would be transparent change (it won't change anything for apps which don;t use it and apps who do don't need to adapt themselves except need for latest flatpak).

@mortenfc
Copy link

mortenfc commented Sep 5, 2022

As a general rule, if blocking something for security would prevent any kind of super obscure software from being able to run, then that rule should be made configurable.
Perhaps you can introduce multiple security levels that the user is prompted with before installing a flatpak.
Like, is the app very secure, medium, or no restrictions. And is the user "sure it wants to install an app of this security level? Blabla there is a low risk of it messing up your Linux machine. Blablabla Flatpak is not responsible". I don't think that people that install obscure software on their Linux machine really care that much about messing it up, since they are most likely tinkering around for fun.
Reviewed, popular apps are much more likely to be secure.

I dislike the idea of generally banning things because of a fear of the unknown. It is, in my opinion, a lazy, narrow-minded, and limiting approach.

@xM8WVqaG
Copy link

xM8WVqaG commented Sep 5, 2022

FWIW, one thing I failed to make clear in my previous comment is the modify_ldt restriction was (apparently) copied verbatim from a web framework which genuinely had no reason to be running any code using this interface. So the comment 16-bit code is unnecessary in the sandbox made complete sense in it's original context of a web sandbox but is more arbitrary in Flatpak.

For a primer on local descriptor table (LDT)'s functionality, there is this Wikipedia article and the modify_ldt man page both of which are pretty approachable for a 101 but neither discuss malicious usage.

Of the currently proposed --allow flags to hide this behind: --allow=16bit and --allow=multiarch are more accurate than --allow=wine and --allow=emulation. If the concern is unnecessarily exposing this syscall - it's more likely somebody would innocently toggle on wine or emulation (because that's the software they're running) when they don't need modify_ldt.

@smcv
Copy link
Collaborator

smcv commented Sep 5, 2022

Perhaps you can introduce multiple security levels that the user is prompted with before installing a flatpak.

We already have those, they're the various permissions flags (including --allow, which I think is probably the most appropriate one here).

including it in --allow=multiarch may make more sense

That hadn't occurred to me as an option, because the multiarch flag is really about something completely different (it's really about the ability to call "compat" syscalls, like when you run native Linux i386 binaries on an x86_64 kernel)... but it's a surprisingly good fit, because the most likely reason you'd want to be able to call "compat" syscalls is that you're running legacy proprietary software, and the sorts of situations where you want to do that have a lot of overlap with the sorts of situations where you want to be able to run Windows binaries under Wine (in practice mostly games).

Please could you put a merge request together for that if it's something you want to pursue?

jmonteiro64 added a commit to jmonteiro64/flatpak that referenced this issue Sep 6, 2022
Some patches for Wine, as well as old 16-bit programs,
require this syscall to work.

As the only programs that need it are using --allow=multiarch,
this commit keeps it disabled when it isn't used,
as a security hardening measure.

For more information, see issue flatpak#4297.
@alexlarsson
Copy link
Member

alexlarsson commented Sep 6, 2022

I think going the multiarch route makes a lot of sense. They really are quite similar, both in what they are used for and in the "security risk" level, and we don't want a ton of static permissions.

alexlarsson pushed a commit that referenced this issue Sep 6, 2022
Some patches for Wine, as well as old 16-bit programs,
require this syscall to work.

As the only programs that need it are using --allow=multiarch,
this commit keeps it disabled when it isn't used,
as a security hardening measure.

For more information, see issue #4297.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet