Sys_UnpackDLL() is a security risk

[smcv]

In the Quake III Arena engine on which JK2/JA are based, executable game modules (cgame, game, ui) in the form of bytecode can be loaded from inside a .pk3 file. Because of the auto-downloading feature, it is possible to join a server and have the server send you a PK3 containing cgame and ui code to be run while on that server. That's quite scary from a security point of view (you're running arbitrary code sent to you by an unauthenticated server on the internet) but the bytecode runs in a somewhat sandboxed interpreter, so it isn't too bad - but it's almost certainly possible for a determined attacker to escape from the sandbox, so I still recommend disabling auto-downloading in all Q3-engine games, and only installing mods whose authors you trust completely.

However, JK2/JA don't use the Q3 bytecode interpreter, and their game modules are native-code (DLLs). To get round the fact that the operating system can't load DLLs from inside a container file like PK3, the Windows versions of JK2/JA unpack the game modules from the PK3 into separate files while the game is running, and run them from there. This upgrades auto-downloading from "quite scary" to "really bad idea" from a security point of view - if enabled, the game will download PK3s from servers you join, and unpack and execute arbitrary native code from them. This means that the server admin, or anyone on the network path between you and the server, can make the game do anything, with your privileges.

Mitigation 1: in OpenJK this only happens on Windows, because the relevant function (Sys_UnpackDLL()) has not been implemented for Linux/Mac. (I don't know whether the Mac port of vanilla JA/JK2 is vulnerable, but the Mac port of OpenJK seems to be OK.)

Mitigation 2: according to #643, most JK2/JA players turn off auto-downloading anyway, because there are other unfixed vulnerabilities in vanilla JK2/JA.

Mitigation 3: in OpenJK, auto-downloading is disabled by default, both client-side (your client will not auto-download from servers unless you configure it to do so) and server-side (servers will not let you auto-download from them unless configured to allow it; so if "most" servers do not allow it, there is little reason for players to have enabled it).

[smcv]

From a security point of view, the ideal solution would be to remove or stub Sys_UnpackDLL(), so the Windows build behaves the same as the Linux build in this respect. However, existing JA/JK2 mods are often distributed as a PK3 file containing modded DLLs (relying on those DLLs being unpacked and run by the game engine), so that would make these mods not work (even on the Windows versions of OpenJK where in principle they should work fine), which probably makes this solution unacceptable.

Another possible solution would be to remove the auto-downloading functionality. That would be a shame, because PK3s that do not contain DLLs are believed to be safe, and it would be convenient to be able to allow auto-downloading for small addons with no code in them (models, skins, maps).

Another possibility might be to "mark" auto-downloaded files by giving them a different extension (maybe .pk3dl?), and have the rule be: load resource files like textures, maps and models from .pk3dl files, but do not extract DLLs from them?

If there is a relatively small number of JA/JK2 mods in common use, another idea might be to have a list of cryptographic hashes (SHA256) of "known-good" mod DLLs, and allow extracting those, and only those; subsequent releases of those mods wouldn't be on the list, so they would have to put the DLLs alongside the PK3, instead of in the PK3.

[ensiform]

None of the solutions are really ideal sadly.

[Razish]

It's also worth noting OpenJK has hardened security against this - in the retail copy of the game, a server could force a client to enable the automatic download option through the CVAR_SYSTEMINFO flag.
OpenJK client only allows the server to set certain white-listed cvars.

I've demonstrated this exploit to run arbitrary code with administrative privileges on a retail copy of JA with the JA+ mod.

Once someone had connected to my server, I would force their cvars to allow downloading, and to use the "fast-http"/cURL downloads from a URI I specified.
Because retail JA did not support homepath correctly, players using Windows Vista or higher always ran as administrator.
It is quite scary running arbitrary code downloaded from an arbitrary URI with administrative privileges.

You could extend this further to exploit the proxy support in tech 3 - run a malicious server and send a heartbeat to the master server(s)
When players refresh the server list, they will send a getinfo/getstatus request to your server. Collect this IP.
For 30 seconds after this request, spam them with connectResponse packets.
If they try to connect to any server, they will receive your connectResponse and assume the connection was handled by a load-balancing proxy.
Once their connection is hijacked and established, proceed with the above exploit of forcing automatic downloads.
You can run some code from the cgame module to connect to the server they initially attempted to connect to, and a careless user would have no idea what has just happened (i.e. they don't suspect you)
The next time they run their game, the code from the UI module will load, and you're free to wreak havok on their system with administrative privileges.
Botnet incoming.

[xycaleth]

A 4th option would be to prevent the server from serving PK3 files which include DLLs. This shouldn't be an issue for existing mods because auto-downloading isn't very common so players will generally already have the mod downloaded. I don't think people would mind continuing to do this, and it still gives the benefit of being able to download new maps/models/etc from the server directly.

[smcv]

Any solution to this has to be on the client side. Modifying the server to disallow serving PK3 files which include DLLs would not be particularly useful, because a malicious server would just undo that modification and we'd be back where we started.

If the idea is that the client checks whether the auto-downloaded PK3 contains DLLs or not before deciding whether to use it, then it would have to download it to a temporary filename not ending with .pk3 to indicate that it had not been checked yet (otherwise, cancelling a connect attempt could leave you with a malicious PK3 in place, ready for next startup), at which point it's very similar to my .pk3dl idea.

[smcv]

None of the solutions are really ideal sadly.

I understand that, but I think they're all better than exposing players to attack from malicious servers; it's just a matter of choosing the least-bad option.

If, in practice, everyone leaves client-side auto-downloading switched off anyway (it's off by default), then I think the best short-term answer might be to stub out that feature. If this is just done by making cl_allowDownload read-only (and perhaps disabling cURL integration since this would make cURL pointless), then it'd be easy to revert later, after implementing some other solution (perhaps the .pk3dl thing I suggested).

[smcv]

I think the best short-term answer might be to stub out that feature

For instance https://github.com/JACoders/OpenJK/compare/JACoders:master...smcv:no-auto-download?expand=1 (untested)

and perhaps disabling cURL integration

I had misremembered, OpenJK does not seem to have any cURL integration (yet?) so there's nothing to disable.

[ensiform]

Unfortunately I don't think we will take action on this because mods that exist now which aren't maintained don't distribute the DLL files extracted already. And then there's the case where the DLLs and mod assets can be in the same pk3 such as the jka vanilla pk3s which obviously cannot be downloaded but still.

[ensiform]

I should also note that RTCW and ET also use DLLs that are extracted from a pk3.

[ensiform]

And also, disabling the extraction opens the possibility to make it easier for more people to cheat especially on pure servers.

Or it may break pure server connection all together. Because it won't be able to mark the cgame pak and ui pak.

[smcv]

mods that exist now which aren't maintained don't distribute the DLL files extracted already

As I mentioned already, if that's one of your requirements, then the first of the possible solutions I described above is not acceptable. The rest (e.g. disabling auto-downloading) don't necessarily contradict this requirement though.

opens the possibility to make it easier for more people to cheat

I understand what you're trying to achieve here, but preventing user-controlled code from executing on clients is just not possible; anyone who can compile OpenJK can easily make a client that pretends to be pure even when it isn't. The CRC-32 used for the pure check is easy to brute-force in any case (I had to do that for Debian's OpenArena packaging, for tedious reasons involving a non-Free compiler, and it takes a matter of minutes on a modern laptop).

I should also note that RTCW and ET also use DLLs that are extracted from a pk3.

Yeah, checking iortcw for that is next on my to-do list (I'm looking into packaging both OpenJK and iortcw for Debian). It's still a security vulnerability, however many projects have it. I know iortcw has switched from native-code (like JA) to QVM bytecode (like Q3A), which mitigates this a bit, because the bytecode runs in a sandbox; but I need to check whether the logic for unpacking DLLs is still present, because if it is, a malicious server can still take advantage of it.

[xycaleth]

To be honest, I think if we disabled autodownloading completely on the client side, nobody would notice or complain. No servers enable auto-downloading...

We could only do it for the first release if we wanted to try and figure out an alternative solution.

[ensiform]

We already know how easy it is to bypass but we still need to support the existing pure server support on both sides because of old clients connecting to openjk pure server and old servers serving all clients with pure. ( Not in regards to disabling autodownload, but to disabling the extraction on Windows at all)

[ensiform]

I'm still not entirely sure on why this matters for Linux specifically since the Linux mods aren't even supported from the pk3 anyway.

[smcv]

Another possibility might be to "mark" auto-downloaded files by giving them a different extension (maybe .pk3dl?), and have the rule be: load resource files like textures, maps and models from .pk3dl files, but do not extract DLLs from them?

I've opened ioquake/ioq3#130 to discuss this in the context of ioquake3. As @ensiform pointed out, OpenJK (GPL-2) and iortcw (GPL-3) both have essentially the same thing, so it's better if any long-term solution is implemented in a GPL-2+ project like ioquake3 so they can both merge it without licensing problems.

To be honest, I think if we disabled autodownloading completely on the client side, nobody would notice or complain.

smcv@9ada560 implements this, I think. Would you like me to turn it into a PR? (Or you're welcome to cherry-pick it.)

Obviously there's a lot of code that becomes unreachable and hence useless with that change; but if we think there'll be a better long-term solution, it might be better to leave it in and just have this minimal commit disabling it, which will be easier to revert later.

I'm still not entirely sure on why this matters for Linux specifically

It doesn't, but I think Windows users deserve to be safe too.