[GEN][ZH] Fix security issue in map transfer by validating file paths when creating real paths from portable paths

[slurmlord]

• Relates to Security issues in the code #272

The discussion in #272 is quite detailed, so the summary here is briefer. Please refer to the original discussion if there are sections here that are unclear, as they are probably already explained more eloquently there.

The Problem

The game has a concept of "Portable Map Paths", where the path to a map/file is stored relative to a well-known base path. The 3 base paths that can be used are "Save", "Maps" and "UserData\Maps", and are used as the start of the Portable Map Path.
The GameState::portableMapPathToRealMapPath function is used to convert a Portable Map Path to a real path on disk, by prefixing the Portable Map Path with a local directory. As an example, the corresponding real map path for the Portable Map Path Save\SomeFile.bin would be something like C:\Users\username\Documents\Command and Conquer Generals Zero Hour Data\Save\SomeFile.bin.
The issue is that a Portable Map Path could contain troublesome path components, like ..\..\..\ or similar, which when merged with the intended base path actually ends up outside the intended base path . Expanding the previous example, the Portable Map Path Save\..\..\..\..\..\windows\system32\Some.exe would actually end up as c:\windows\system32\some.exe.
Some places (loading save games, network map transfers) these Portable Map Paths are used for defining where a file should be saved to, so in practice a malicious Portable Map Path could be used to overwrite arbitrary files on the user's system.

The Solution

When combining the Portable Map Path with the base path in GameState::portableMapPathToRealMapPath, validate that the resulting absolute path is within the base path. This validation is done by normalizing the absolute path and comparing it to the normalized base path. If the normalized absolute path does not start with the normalized base path, the Portable Map Path is considered invalid and an error is returned.
In order to facilitate this, the LocalFileSystem implementations get a new normalizePath method, which is implemented via the GetFullPathName Win32 function, and std::filesystem::path::lexically_normal. This function is then exposed through FileSystem, as the underlying LocalFileSystem implementation is not public.

The major change is that now GameState::portableMapPathToRealMapPath can return an empty string in case of a troublesome Portable Map Path. As far as I can see, a malicious Portable Map Path could come from a save file or a map transfer. Both of these now recognize the condition and semi-gracefully degrades (no explicit message as to what went wrong to the user, but no overwrite of unwanted files either).

[Caball009]

I think std::filesystem::path does this already upon initialization / assignment, and if not, it has a member function for it: std::filesystem::path::make_preferred.

[slurmlord]

Yeah, I thought so, too, but then as I read https://en.cppreference.com/w/cpp/filesystem/path/make_preferred.html my understanding is now that since backslash is considered a valid character in a POSIX file name, it will not be replaced by make_preferred? Thus the "heavy-handed" approach.

[Caball009]

It looks you're right about that: https://godbolt.org/z/ebhexrE6s Notice how only MSVC converts both paths to a consistent format.

[xezon]

Nice fix. Some comments.

[xezon]

Looks very good. Just a few more minor comments.

[xezon]

Looks very good to me. Very secure.

[xezon]

Title optimized.