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

[SECURITY FIX] Profile import RCE #1288

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Conversation

sadkris
Copy link

@sadkris sadkris commented Apr 6, 2024

Hi! Whenever importing a profile from a code or a file, r2modman would arbitrarily extract all files without checking the type. This allows the user to add DLL files to the modpack that aren't included on the mod list, allowing a malicious party to add malware that is automatically loaded by BepInEx and run in the game without appearing on the modlist of the pack. This change ignores DLL files when extracting, not allowing them to be extracted to the user's installation directory.

Suggested remediation in the future would be to add more security onto the profile code APIs, as any user can upload any data permanently and receive a profile code that can be used to download their arbitrary content from your servers. This can be used in many malicious ways, for example allowing a hacker to shift blame to you for holding exploit code on your local servers.

@CLAassistant
Copy link

CLAassistant commented Apr 6, 2024

CLA assistant check
All committers have signed the CLA.

@sadkris
Copy link
Author

sadkris commented Apr 11, 2024

@ebkr Bumping this since the longer this is left in the public eye, the bigger the security risk. This is already a really critical vulnerability

@MythicManiac
Copy link
Collaborator

Hey, just commenting to acknowledge we've seen this. Right now this isn't considered urgent due to the fact every & any mod install in itself already counts as RCE, so the main difference here is that this isn't visible in the UI the same way normal mod installs are. In other words, this vector isn't as major of a problem as it might appear on the surface if you consider it in the context of the entire ecosystem.

We will most likely address this but it'll require narrowing down the profile import/export file format & feature capabilities somewhat. The way it currently works is by design and there's some chance we'd break something by simply merging this change as it is, although at least based on some preliminary investigation it looks like it shouldn't break anything important.

@sadkris
Copy link
Author

sadkris commented Apr 28, 2024

Hey, just commenting to acknowledge we've seen this. Right now this isn't considered urgent due to the fact every & any mod install in itself already counts as RCE, so the main difference here is that this isn't visible in the UI the same way normal mod installs are. In other words, this vector isn't as major of a problem as it might appear on the surface if you consider it in the context of the entire ecosystem.

We will most likely address this but it'll require narrowing down the profile import/export file format & feature capabilities somewhat. The way it currently works is by design and there's some chance we'd break something by simply merging this change as it is, although at least based on some preliminary investigation it looks like it shouldn't break anything important.

I disagree with this. By nature all mods have a risk of RCE, but the point of this issue is that it allows people to hide mods inside the export code without a user's knowledge or ever being listed in the UI or store

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

Successfully merging this pull request may close these issues.

3 participants