Safer auto-download handling, for maps but not code (#130)

[smcv]

As proposed on #130, this branch separates auto-downloaded and non-auto-downloaded PK3s, and does not allow loading executable code from auto-downloaded PK3s. This lets the auto-download mechanism be used for maps and textures without needing to have absolute trust in the server admin (and everyone on the network between you and them).

Auto-downloaded PK3s get a marker in the filename (.noexec.) which keeps them non-executable even if they are dragged into a directory that is on the search path, so that users can make an auto-downloaded map permanently available without privilege escalation.

See #130 for background information, and further details of some of the choices made in designing this branch.

[ec-]

Why not check localName length before possible truncation in Com_sprintf()?
Also, you can do than once for all files in CL_InitDownloads() not waiting for a pak with long name in the tail

[smcv]

Why not check localName length before possible truncation in Com_sprintf()?

Because then I'd have to calculate how long it was going to end up (or equivalently, how long a buffer it would need to avoid truncation) and if I got that calculation wrong, we'd be back where we started. I'm aiming to make it as obvious as possible when security-sensitive things are correct.

Also, you can do than once for all files in CL_InitDownloads() not waiting for a pak with long name in the tail

Up to a point, but that requires knowing in one location in the code how large a buffer at another location is going to be. The further apart these checks are, the less obvious it will be whether they're correct.

[smcv]

Branch updated to use the return value of Com_sprintf() to check for truncation (in a similar style to strlcpy()), rather than doing the maybe-truncating printf and then checking its output.

[mickael9]

There is a small issue with your implementation: pk3s in the download folder are only added in the search path if the server explicitly names them. If the client is the server (singleplayer) then those downloaded pk3 will never be loaded. This creates a strange situation where one can, say, connect to a server having a map X but can't play it locally with /map X

This also has the side effect of restricting the (download) search path to only the pk3s referenced by the server (as opposed to those that are shared between client and server but not necessarily referenced, which would be the normal behavior). Now that could be considered a good thing (some mappers just love to override textures/sounds from other maps which can be quite annoying because a map can render differently from one server to another) , but IMHO this should be an option, not default behavior.

[smcv]

There is a small issue with your implementation: pk3s in the download folder are only added in the search path if the server explicitly names them

This is deliberate, see the comments from @sago007 and @zturtleman on #130.

It's a trade-off. If auto-downloaded pk3s are automatically made active for single-player (or a locally-hosted server) like they are now (and like you want), then a map or mod that you auto-downloaded from a server is immediately playable, which is good; but the same server can also (for instance) replace your maps/q3dm1.bsp with its own version, or replace your textures, and having those changes carry over into single player violates the principle of least astonishment. That's why @sago007 and @zturtleman advocated behaviour more like Unreal Tournament (1999), which auto-downloaded mods from servers to a Cache directory; a user (perhaps helped by a GUI launcher that wraps the actual engine) can move them from there to the actual search path (Maps or Textures or whatever in UT99, baseq3 here) if they want to keep them.

This also has the side effect of restricting the (download) search path to only the pk3s referenced by the server (as opposed to those that are shared between client and server but not necessarily referenced, which would be the normal behavior)

I believe the behaviour is:

On a "pure server", which is the default, the search path is always restricted to the pk3s that the server told you to load, in the priority order that the server told you to load them. This hasn't changed.

On an "impure" server, pk3s referenced by the server (in the order specified by the server) are highest priority. If a desired file isn't found in any of those, then the same pk3s that would be loaded in single player (and in the same order) are also searched, as a lower priority (because they were already in the search path before we prepended the auto-downloaded pk3s). In particular, if you are on an impure server and a player in the multiplayer game (you or someone else) has selected a custom model or skin that would have been available to you in single-player, you'll also see it in multiplayer.

Is there a concrete situation where this implementation would load content that differs from what the current code would load, other than it not loading content from pk3s that were auto-downloaded from a previous server but are not referenced by the current server?

[Chomenor]

There is a small issue with your implementation: pk3s in the download folder are only added in the search path if the server explicitly names them. If the client is the server (singleplayer) then those downloaded pk3 will never be loaded. This creates a strange situation where one can, say, connect to a server having a map X but can't play it locally with /map X

I think it is relevant to point out that my filesystem project does achieve the behavior you are looking for without the downsides of letting downloaded pk3s override anything. Files in the download folder are always loaded and available to use (except for qvm and cfg files), but this check prioritizes them below everything else, and effectively stops them from breaking anything that otherwise works. It also works on a per-shader level, rather than per shader file, so you don't have to worry about special shader precedence issues.

On an "impure" server, pk3s referenced by the server (in the order specified by the server) are highest priority.

This definitely has the potential to be disruptive. Referenced paks have historically only been used for downloads, so servers have a lot of flexibility in how it is used. If my understanding is correct, servers will now have problems with clients using paks downloaded from other servers if

• They don't provide a referenced pak list at all
• Any referenced paks have different names (ie map-mapname.pk3 vs mapname.pk3)
• Any referenced paks are in the wrong order
• Any referenced paks are missing

None of these things would have mattered before, so it is a lot of potential headaches for both server admins and players. Pure servers may partially mitigate some of these issues, but only around half of servers are pure anyway, so you can't depend on that.

Is there a concrete situation where this implementation would load content that differs from what the current code would load, other than it not loading content from pk3s that were auto-downloaded from a previous server but are not referenced by the current server?

Probably the shader precedence; for example there will be cases where a pk3 in the download folder will now override the ID pak shaders when it wouldn't before. This would be considerably less common as a practical problem than the other reference-list related issues, however.

[NuclearMonster]

@Chomenor: please do not jump into unrelated threads to announce your project. We know about it.

[Chomenor]

@timedoctor Only my first paragraph was about my project, and it was about an aspect of my project I had neglected to document previously, so that is why I though it was worth mentioning. It wasn't my intention to spam, but I'll try to be more careful in the future.

[smcv]

To be clear, I don't much care about any specific implementation: I just want something that the ioquake3 maintainers are willing to merge, and that has the result that a malicious server is unable to cause arbitrary code execution on its clients. If someone else's implementation meets those criteria, great.

This definitely has the potential to be disruptive. Referenced paks have historically only been used for downloads, so servers have a lot of flexibility in how it is used.

The reference (ioquake3) server sends checksums and names in most-important-first order, so the current implementation in this PR is not correct even if the server is assumed to send the same order that ioquake3 would: it iterates that list forwards, and prepends each auto-downloaded pk3 to the client's search path, resulting in the least important auto-downloaded file coming first. Of course, this only matters if there is more than one auto-downloaded pk3, or if there is an auto-downloaded pk3 that is less important than a pk3 that is on the normal search path (available to single-player games).

If auto-downloaded PK3s were inserted into the search path in sorted order, instead of always being prepended, I think that would resolve this: so for instance downloads/baseq3/pak2-and-a-half.12345678.noexec.pk3 would be inserted into the most-important-first search path between baseq3/pak3.pk3 and baseq3/pak2.pk3. Doing a linear search instead of a prepend operation in FS_AddAutoDownloaded() wouldn't be rocket science. It would be O(n**2) in number of pk3 files, but I doubt that matters.

[smcv]

If auto-downloaded PK3s were inserted into the search path in sorted order, instead of always being prepended, I think that would resolve this

That doesn't quite work, because all PK3s from a higher-precedence directory beat all PK3s from a lower-precedence directory, regardless of alphabetical order (if we're playing Team Arena, ~/.q3a/baseq3/aaa.pk3 and /usr/local/quake3/missionpack/aaa.pk3 are both more important than /usr/local/quake3/baseq3/pak0.pk3). But a cleverer comparison function could fix that.

[ec-]

Until you provide some reliable and automatic update system - all this stuff looks like shooting to the one remaining leg

[mickael9]

It's a trade-off. If auto-downloaded pk3s are automatically made active for single-player (or a locally-hosted server) like they are now (and like you want), then a map or mod that you auto-downloaded from a server is immediately playable, which is good; but the same server can also (for instance) replace your maps/q3dm1.bsp with its own version, or replace your textures, and having those changes carry over into single player violates the principle of least astonishment.

I agree with @Chomenor here, this is easily solved by putting the downloaded pk3s at a lower priority in the search list by default. Then the map is playable but won't override anything. Connecting to pure servers works as before. And unpure just as you described except that it would also look for other locally downloaded content (not on the server) instead of just the one referenced by the server.

[smcv]

@timedoctor, @zturtleman, other ioquake3 maintainers: What search-path order would you prefer for auto-downloaded mods on impure servers?

And would you prefer auto-downloads to be active after disconnecting from the server that used them (like vanilla Quake 3, so players can receive mods while joining a server and easily keep them afterwards), or not (like Unreal Tournament '99, so players can rely on joining a server not permanently changing textures or behaviour after disconnecting)?

When I have answers to those two questions I'll update this PR to follow them.

Non-maintainers can try to persuade the maintainers that a particular answer to one of those questions is best, but I'm not going to try to keep everyone happy by producing multiple variations of this branch, because there are multiple contradictory views; so when the maintainers make a decision, I'm going to treat it as final. I would be inclined to give more weight to the opinions of maintainers of widely used ioquake3 forks, like @sago007 (OpenArena) and @ensiform (OpenJK), who both commented on #130, because if widely used forks don't pick this up then I won't have achieved my goal of making the idtech3-based ecosystem more secure.

For reference, the current search-path order for a mod (let's say Team Arena, missionpack) modifying a base game (let's say vanilla Quake 3, baseq3) is to take the first file found by iterating through this list:

• loose files in ~/.q3a/missionpack
• most important PK3 in ~/.q3a/missionpack (latest in ASCII order)
• ...
• least important PK3 in ~/.q3a/missionpack (earliest in ASCII order)
• loose files in read-only missionpack
• most important PK3 in read-only missionpack
• ...
• least important PK3 in read-only missionpack
• loose files in ~/.q3a/baseq3
• most important PK3 in ~/.q3a/baseq3 (latest in ASCII order)
• ...
• least important PK3 in ~/.q3a/baseq3 (earliest in ASCII order)
• loose files in read-only baseq3
• most important PK3 in read-only baseq3
• ...
• least important PK3 in read-only baseq3

where PK3 files in ~/.q3a include both mods manually installed by the user and mods auto-downloaded from the current or a previous server. For the base game only (no mod), skip the lines that mentioned missionpack.

I think the reasonable options for sort order are:

• behave as though auto-downloads (or at least those referenced by the server) were interleaved with non-auto-downloads in ~/.q3a, sorting alphabetically as usual (later in the alphabet = more important)
• promote auto-downloads to be more important than non-auto-downloads, and one of:
  • sort them in the order given by the remote server (first = most important reference from remote server is treated as most important on client)
  • sort alphabetically among auto-downloads (filenames later in the alphabet are more important as usual)
• demote auto-downloads to be less important than non-auto-downloads in ~/.q3a but still more important than the read-only path, and one of:
  • sort them in the order given by the remote server
  • sort alphabetically among auto-downloads
• demote auto-downloads to be less important than anything, even the read-only path, and one of:
  • sort them in the order given by the remote server
  • sort alphabetically among auto-downloads

At the moment this branch promotes auto-downloads to be more important than non-auto-downloads, in the reverse of the order given by the remote server (least important remote file is most important on the client), but that's clearly wrong and needs fixing. I'll correct it when I know what I'm aiming for.