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

ryujinx: mark as known vulnerable #346797

Closed
wants to merge 1 commit into from

Conversation

Naxdy
Copy link
Contributor

@Naxdy Naxdy commented Oct 6, 2024

Things done

Relevant discussion: #345758

Following the merge of #346694 , I think that this is an acceptable alternative to removing the package entirely: Leave it in nixpkgs (using archive.org as a mirror for the source), but do let users who wish to install it know that "hey btw, this thing you're about to install is entirely unsupported by upsteam and might bitrot / develop unfixable (by us) security issues in the future".

This will hopefully prompt users to seek out and migrate to maintained forks (whenever they inevitably pop up) on their own, and at their own pace, until we actually do end up either removing or replacing this package sometime in the future.

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@emilazy
Copy link
Member

emilazy commented Oct 6, 2024

While we regularly set knownVulnerabilities for EOL versions of cryptography libraries and highly complex and exposed software like browser engines and language runtimes, I'm not sure it makes sense for an emulator that was maintained until a few days
days ago with no known concrete vulnerabilities.

I guess it is highly complex code with a JIT engine exposed to semi-trusted input, so I can see the argument, but I feel like loading a ROM into an emulator is more like executing a binary file than loading a web page. Not sure. I'll think about this and I welcome the feedback of others as well.

@Naxdy
Copy link
Contributor Author

Naxdy commented Oct 6, 2024

The case of Project64's RCE comes to mind: https://gist.github.com/aglab2/6bb351c4bd0ac6e13c931e03aa8f9949

Since Ryujinx, compared to Yuzu, is very much focused on emulating the Switch's actual behavior as opposed to using hacks to run games, it is naturally also capable of running (third-party-/user-developed) mods, many of which are not open source. If/when a vulnerability is discovered, it would be trivial to either piggyback off of a well-known mod's name or write your own in order to exploit it.

Then there's also the possibility of vulnerabilities being discovered in the now-permanently-frozen upstream dependencies of Ryujinx, of which there are quite a few. Youtube-dl has been marked as known vulnerable for a similar reason ( #325371 ), though in that case there have actually been issues discovered in its successor.

@emilazy
Copy link
Member

emilazy commented Oct 6, 2024

To be clear, I'm 100% behind marking it as vulnerable if a concrete issue is discovered. I'm just thinking over whether it makes sense to do prophylactically in this case. Every piece of software could turn out to have a critical vulnerability at any time and it's hard to know ahead of time which ones will actually be addressed, so we have to draw the line somewhere lest we mark every package that hasn't seen a commit in a few months as insecure. Being explicitly abandoned upstream is certainly a significant weighing factor here; it's just a question of the relative risk.

@Naxdy
Copy link
Contributor Author

Naxdy commented Oct 6, 2024

To be clear, I'm 100% behind marking it as vulnerable if a concrete issue is discovered.

I'd agree with that statement if there was still an upstream. However, the problem I see is that because upstream is gone, there is also no responsible disclosure process anymore. This means that whenever a vulnerability is actually discovered, the only way to make others aware of it is to shout into the wind, which I view as highly problematic.

Ideally, the devs are given a chance to fix the issue before it's made known to the general public, or better yet, someone submits a PR fixing the problem instead of just reporting it, but this cannot happen as it stands. And with a codebase as large as Ryujinx's, and that many moving parts, I think it's a question of "when", not "if".

Personally, I'd rather err on the side of too much caution rather than too little, and at least for myself I can say I would like to know whether I'm installing something that receives no upstream development at all. If possible, I always check upstream's commit history and open issues before I install something, just to make sure it's receiving at least maintenance-level development, but that's probably just me 😛

Either way, I'd also be interested in what others think about this, I think I've said about all I can say at this point.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Oct 6, 2024
Copy link
Member

@Atemu Atemu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, the problem I see is that because upstream is gone, there is also no responsible disclosure process anymore. This means that whenever a vulnerability is actually discovered, the only way to make others aware of it is to shout into the wind, which I view as highly problematic.

While responsible disclosure is obviously preferable, it doesn't really make a difference to us as a distro. We mark packages as vulnerable as soon as we know about the vulns; whether they were disclosed responsibly or not.

at least for myself I can say I would like to know whether I'm installing something that receives no upstream development at all

That's a valid concern but that's not what knownVulns is for. If you'd like that to be tracked, please help push the implementation of RFC 127 forward which defines the "deprecated" problem for this exact case.


I concur with @emilazy that this isn't really the same case as what we'd usually pre-emptively set knownVulnerabilities for.

Game console emulators generally do not make any security guarantees that could possibly be broken by a vuln. They do not promise isolation from the host; they're a compatibility layer, not a security barrier. You wouldn't mark WINE as vulnerable because it's possible to run Windows malware using it either.

I don't think it's warranted to set a known vuln due to deprecation.

@AndersonTorres
Copy link
Member

Another possibility is to use the yet-to-be-merged Problem Infrastructure:

@emilazy
Copy link
Member

emilazy commented Oct 6, 2024

Honestly my experience is that users generally do expect anything that looks like an emulator or VM to be a security boundary. I think the WINE case is slightly different because providing unfettered access to the system is part of its goal, but even there people are surprised that it’s not safe to run untrusted code in WINE. A console emulator is closer to an isolated, hermetic experience in theory, so I expect the user expectations to be stronger there. We can’t do this solely based on vibes though, but since knownVulnerabilities is intended as a warning to users we should consider their expectations when making decisions like this.

Using the problems infrastructure to designate packages that are abandoned upstream would be a great solution for things like this, were it possible. The problem is that knownVulnerabilities is a big hammer and so we need to carefully consider if the trade‐offs are worth it.

@artemist
Copy link
Member

artemist commented Oct 6, 2024

Ryujinx is substantially stronger of a security boundary than wine or even other console emulators like dolphin. It's written in a memory-safe language, memory accesses in the JIT-generated native code are checked by default, and file accesses are limited to known paths.

I don't think it makes sense to mark this as vulnerable just because it's unmaintained. I would care much more about if software is designed securely in the first place and meets user security expectations, and ryujinx meets those.

@emilazy
Copy link
Member

emilazy commented Oct 6, 2024

I knew that the code was in C#, but not that it went to lengths to try and ensure the memory safety of the generated code. That reassures me and inclines me further towards continuing to ship Ryujinx without a warning for now. The reduced incentive to find and report bugs in an abandoned project is a fair point; since I expect people will continue using Ryujinx unless a replacement arises, there will still be an incentive to exploit its users, but the incentive to do altruistic security research may be reduced. I am not sure what concrete information about that threat landscape would change my bottom‐line conclusion, but my vibes point toward leaving off a warning for now, in the absence of the problems infrastructure.

I am still concerned about the long‐term maintenance state given its extensive pinned dependencies. It would be great if a competent fork appeared, but it seems like it’s best not to hold one’s breath for that with these emulators (though perhaps the lack of an explicit DMCA will reduce the chilling effect this time around).

@Faeranne
Copy link

Faeranne commented Oct 6, 2024

Not an immediate solution, but this and Duckstation seem like perfect reasons to have an unmaintained flag of some kind. something that says "we don't know of any vulnerabilities, but it very well could go bust at any moment". Would also help in the future when a package starts failing builds, to know that said package has no maintainer, and should be marked deprecated, or in the event of a security issue, marked vulnerable. Maybe a system like this already exists internally, but if not, this could be a good indicator for others, throwing a warning for users when used.

@emilazy
Copy link
Member

emilazy commented Oct 6, 2024

I think that DuckStation is a different case since the upstream is actively hostile on legal grounds to packagers of even the last FOSS version and it’s unlikely we are in strict licence compliance. I still think we should probably remove it or at least set hydraPlatforms = [ ]; to reduce our liability there; a disappearing upstream and a hostile one call for different responses. But yes, offering that kind of warning is why the problems infrastructure RFC was passed and is currently pending implementation.

@AndersonTorres
Copy link
Member

You wouldn't mark WINE as vulnerable because it's possible to run Windows malware using it either.

I think this is substantially different.
Complementing what Emilazy said, Wine is (saying in very crude terms) a thin (?) layer written as a library file that translates Windows API to POSIX.

An emulator like Bochs is completely different: it emulates the whole hardware.
In order to exploit a bug on Bochs, we should create a boot loader that executes a x86 instruction block that triggers that particular bug in Bochs.

Indeed, supposing a hardware bug in X86 like Meltdown was emulated by Bochs, I can't imagine such a bug affecting the host machine - how can Bochs cause Meltdown in RISC-V?

@Atemu
Copy link
Member

Atemu commented Oct 6, 2024

Bochs has an explicit security barrier; it emulates a whole system in a sandbox. Every piece of hardware is emulated.

Game console emulators OTOH are closer to that "thin layer" approach of WINE; translating i.e. console-specific graphics to native Vulkan or maps filesystem syscalls to directly accessing some directory on disk (no filesystem image with emulated storage devices or anything).
CPU emulation is also frequently made to be fast, not correct or complete because its purpose is to function well enough to run existing games, not any possible program.

@Naxdy
Copy link
Contributor Author

Naxdy commented Nov 13, 2024

Closing because of 1) general consensus, and 2) there appears to be a maintained fork of Ryujinx now, so switching to that would make a lot more sense than this PR ( #345758 (comment) )

@Naxdy Naxdy closed this Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants