Skip to content

Conversation

@zbx1425
Copy link
Contributor

@zbx1425 zbx1425 commented Jul 14, 2021

This PR contains two main changes.

  1. The FileFormats.IsNautilusFile function is removed since the request at Control over the sequence of the plugins #459 is no longer needed.
  2. Plugin interface is modified so that the Load and SetVehicleSpecs + Initialize stages of train plugin loading is divided.
  3. The plugin of the player train is loaded before the loading of the route. After loading the train, the SetVehicleSpecs and Initialize functions are then called.

This change is to enable train plugins to affect the loading progress, by placing hooks on FileSystem Apis.
In the current implementation, if a train plugin wants to achieve this, it'll have to reset the game status and restart the loading by performing a lot of internal reflection calls, for example:

// Reset game status
ReflectionManager.Log("Resetting game status.");
try {
    // Reset current route
    if (loadType == 0) {
        FCurrentRoute.SetValue(null, Activator.CreateInstance(
            FCurrentRoute.FieldType,
            new object[] { FRenderer.GetValue(null) }
        ));
        FPlayerTrain.SetValue(null, null);
        FTFOs.SetValue(null, Array.CreateInstance(typeof(AbstractTrain), 0));
    } else if (loadType == 1) {
        FCurrentRoute.SetValue(null, Activator.CreateInstance(
            FCurrentRoute.FieldType,
            new object[] { FCurrentHost.GetValue(null), FRenderer.GetValue(null) }
        ));
        FTrainManager.SetValue(
            null,
            Activator.CreateInstance(TTrainManager, new object[] {
                FCurrentHost.GetValue(null),
                FRenderer.GetValue(null),
                FCurrentOptions.GetValue(null),
                FFileSystem.GetValue(null)
            })
        );
    }
    MReset.Invoke(null, new object[] { true });

    // Reset object library
    var renderer = FRenderer.GetValue(null) as dynamic;
    renderer.StaticObjectStates = new List<ObjectState>();
    renderer.DynamicObjectStates = new List<ObjectState>();
    renderer.InfoTotalTriangles = 0;
    renderer.InfoTotalTriangleStrip = 0;
    renderer.InfoTotalQuadStrip = 0;
    renderer.InfoTotalQuads = 0;
    renderer.InfoTotalPolygon = 0;
    FAnimatedWorldObjects.SetValue(null, Array.CreateInstance(typeof(WorldObject), 4));
    FAnimatedWorldObjectsUsed.SetValue(null, 0);

    // Reset loading status
    if (loadType == 0) {
        FTrainProg.SetValue(null, 0.0);
        FTrainProgSum.SetValue(null, 0.0);
        FTrainProgWeight.SetValue(null, 1.0);
    }
} catch (Exception ex) {
    ReflectionManager.Log("ERROR Reset: " + ex.ToString());
    return false;
}

// Restart load progress
ReflectionManager.Log("Restarting load progress.");
try {
    MLoadThreaded.Invoke(null, new object[] { });
} catch (Exception ex) {
    ReflectionManager.Log("ERROR Restart: " + ex.ToString());
    return false;
}

This method extends the loading time, and internal changes in OpenBVE can easily break its compatibility.

By loading the plugin before the route loads, the plugin can reliably alter the loading process of the route. This also eliminates the risk of future incompatibility due to internal changes.

@AmberIsFrozen
Copy link
Contributor

If the train plugin is loaded before anything else, this meant that we can make an object that load a specific texture
(e.g. a.png)

Then fetch a texture file from online sources that can provide the latest news, and replace the old a.png with the new one from the internet.

This will allow us to display the latest news inside the train just like in real life to make the game experience more realistic, all without having to manually restart the loading progress via the plugin.

@leezer3
Copy link
Owner

leezer3 commented Jul 15, 2021

I'm somewhat underwhelmed by the idea of moving the train plugin initialization like this.

The essential trouble is that (especially with legacy Win32 stuff), we have no idea on what's going on in the plugin internally.
Assume for example, a plugin which kicks off a timer immediately on load; You're introducing a different delay between the load and initialize calls with no clue what that could do.
I also know that some versions of OS_ATS are very finicky with regards to timing etc.


Online textures:
I don't especially like the idea, but something like this would be a far more sensible idea IMHO:

[MeshBuilder]
Vertex -0.5, -0.1, 0
Vertex -0.5, 0.1, 0
Vertex 0.5, 0.1, 0
Vertex 0.5, -0.1, 0
Face 0, 1, 2, 3
[Texture]
URL http://example.com/myTexture.png
Coordinates 0, 0, 1
Coordinates 1, 0, 0
Coordinates 2, 0.15625, 0
Coordinates 3, 0.15625, 1

The CSV / B3D plugin would then fetch the file and use as per a texture.

@zbx1425
Copy link
Contributor Author

zbx1425 commented Jul 15, 2021

The timing is indeed a potential problem for plugins that rely too much on system timers...

I hope at least there's an optional way to load a plugin before loading everything else.
For example a field in the proposed ats.xml format.
Or a Preload function in .Net Plugins whose presence is check by reflection and will be called if it's present.

Loading a plugin earlier gives it more power to affect things.
If a plugin can't be loaded before the route loads, then I'll have to use these reflections in order to load dynamic contents, which breaks on new releases and forces me to rewrite the plugin for every OpenBVE version. This would be quite a labor.

leezer3 added a commit that referenced this pull request Jul 15, 2021
@leezer3
Copy link
Owner

leezer3 commented Jul 15, 2021

Opened the following PR with texture URLs:
#671

(I keep on intending to push a release soon, will aim to merge this after that, along with the GL menu and animated GIF code)

Can you clarify exactly what you're trying to do with affecting the loading process?
A train should ideally be entirely route independant.....

@zbx1425
Copy link
Contributor Author

zbx1425 commented Jul 15, 2021

This is again about some anti-piracy issues. I have been still working on these issues due to their severity.
You can see the what the plugin does at https://translate.google.com/translate?sl=zh-CN&tl=en&u=https://www.zbx1425.cn/nautilus/assetbundleloader.html .
A route with this plugin can be downloaded at https://svc.bvecs.tk:8953/bcs-src/2110036678.qq.com/ECMSC-Guangzhou%20Metro%20Line%208%20Ver.3.0%20for%20ob.zip .

Basically, a dummy train and a route will be generated with the sole purpose of loading this plugin, while the actually route and train contents being packaged in a single large ASSET package.
When it loads, it parses the ASSET package, then uses Harmony.Lib to intercept FileSystem calls (including File.Exists, File.ReadAllText, File.ReadAllLines, FileStream constructor and functions, FileInfo functions, Bitmap constructor and Image.FromFile), in order to simulate the effect of the files of the route presenting on the disk, despite them being non-existant while their contents being packed into an ASSET package, and redirecting the attempts to read them to return bytes from the ASSET package.

Because the plugin is loaded after the train is loaded, to affect the loading of the route and train, it has to use reflection to reset the CurrentRoute and Renderer, then restart the loading progress. Because there currently isn't a function to perform this (Game.Reset doesn't seem to reset everything), it has to perform many internal calls, which often breaks when a new version comes out.
I originally tried to do this with route/object/texture loading plugins, but since installing them requires admin privilege, it is even more unstable than this approach. Also this way a permanent impact on the player's computer, which is not very good.

An alternative would be a way for a route or a train to specify their custom route/object/texture loading plugins required, and loading them before the route/train is actually loaded.
Or calling a PreLoad function on a .Net Plugin if it exists, by reflection.
Or ensuring Game.Reset to reliably reset all game and renderer statuses.

@zbx1425
Copy link
Contributor Author

zbx1425 commented Jul 15, 2021

This is quite a hacky implementation, but there doesn't seem to be any elegant way to do this. The rest of it is already finished, and the only thing it needs is to load the DLL before the route loading starts.
The requirement's there, so not doing it is not really an option.
I think such a edge case application is too uncommon and doesn't worth a complex "proper" solution, and I think the "call PreLoad function by reflection before the route loads if it exists" should be a simple and effective solution. It also opens a door for developers to implement their own mechanisms, such as the online image stuff.

@leezer3
Copy link
Owner

leezer3 commented Jul 16, 2021

Your 'copyright protection technique' is nothing more than an incredibly dangerous variant of the ZipSlip attack technique:
https://snyk.io/research/zip-slip-vulnerability

An attacker could quite easily put anything in the encrypted archive, which your 'fake' train loading plugin will then quite happily chuck out.

There's no way I'm deliberately merging something designed to do this- It's irresponsible and dangerous.
Allowing arbritrary plugins is bad enough (an attack surface like this is specifically why a lot of apps which support plugins are moving to require them being signed)

@zbx1425
Copy link
Contributor Author

zbx1425 commented Jul 16, 2021

I think you misunderstood the behavior of my plugin. It doesn't "extract" anything. It intercepts OpenBVE's attempts to read files, and return data in the package instead of data on the disk. Nothing is written to the user's disk in the whole process.

What we're requesting is just a way to call a function in the train plugin before the route and train loading starts.
OpenBVE doesn't have sandbox for train plugins, which means it's already unsafe. Calling another plugin function before the route loads won't really make it safer/unsafer.
BVE is a game about third-party developers making things, and the whole train plugin system is to enable third-party developers to do what they want. The earlier a plugin loads, the more ability it has. I think not allowing a plugin to run before the route and train loads is an unnecessary limitation. The security matter is always not a major concern of the community, and I fail to see the necessity to bring it up now, especially when the plugin don't actually write anything to the user's disk, registry or anything else.

And I'm not asking for merging the plugin, which is refused years ago.
I understand that these copyright protection thingies counteract with the open-source philosophy, which might make one feel annoyed. But the problem is there, and it's not going away if nothing is done.
The request is just for calling a function before the route and train loads.

@leezer3
Copy link
Owner

leezer3 commented Jul 16, 2021

I'm not misunderstanding it at all. I'm afraid you're showing a complete disregard for best developmental and security practices in the name of copyright protection.

You're passing out an arbritary encrypted archive, the contents of which then get loaded completely unsanitized, and you seem to think this is a good idea as we can already do that. Whilst you've got a (minor) point there, I can upload any given train plugin / content plugin to somewhere such as VirusTotal, or run it through my own antivirus, You can't do that with an encrypted archive......

You also say that the community doesn't care about this sort of issue. I agree with you there, but as a responsible developer / maintainer, it's my job to protect people from what they don't care about or understand.

@zbx1425
Copy link
Contributor Author

zbx1425 commented Jul 16, 2021

My point is that it's not comparable to the "ZipSlip attack technique".

Especially when the plugin is loaded before the route and train loads. Even though the plugin patches the FileSystem Apis at the time it's loaded, OpenBVE will not actively load any DLL files after that, and given that my plugin also doesn't load any DLLs, that means there's no arbitary code execution from that archive file.

And the only plugin that's getting loaded is this plugin that patches the FileSystem Apis, which is stored outside (since the apis are original when it's loaded), you can easily use an antivirus software to confirm its behavior.
I think this is only loading data, and there's no room for arbitary code execution in this process. So I don't see security problems in this process.

The worries are

  • File extraction and overwriting
  • Code execution from encrypted content
  • Not able to use antivirus to check behavior
    And in this case, they are not present.

Again, this is just requesting calling another function at an earlier time, and we are talking about whether it's a good idea to load a plugin 30s earlier, rather than whether it's a good idea to load encrypted contents. I personally think disapproving an application is not a strong reason to not supporting an interface.
BVE supported plugins despite the potential security problems, and I think it's not a strong statement to disapprove loading plugins before the route just because "you can load encrypted contents with that", especially when there's no actual risk of ACE, and loading encrypted contents is already possible before. So I don't think approving this PR will make it any more unsafe, or rejecting this PR will make it safer.
And if this PR doesn't get merged, I'll just keep on using that "use reflection to call LoadEverythingThreaded again" method to achieve essentially the same. The copyright problem won't disappear if the PR isn't merged, and developers has to find a way around it. Why force us to use reflection to maneuver it around while a small addition can make it more straightforward?

@leezer3
Copy link
Owner

leezer3 commented Jul 16, 2021

It's 100% directly comparable.

Your asset bundle encoder creates an encrypted ZIP file, nothing more complicated than that. By the looks of things, you've got an encryption key and a checksum in the routefile. With a little decompiling / reversing (I note you're using some sort of code obsfucation, but this doesn't stop someone determined), it'll be trivial to supply an arbritrary archive. Your encoder doesn't even seem to block the archiving of DLLs etc. from the route folder.....

The contents of this archive have to be decrypted / extracted somewhere, before being loaded into the existing plugins, which absolutely expose us to arbritray code execution and ZipSlip / similar vunerabilities.

You then use Harmony ( https://harmony.pardeike.net/articles/intro.html ) to modify in memory the filesystem APIs (bluntly, this is virus-like behavious in itself).

This will not be merged; it's messing with timings, and is highly likely to have unexpected effects.
In addition, your entire approach is dangerous in the extreme and should be reconsidered.


If you want a 'correct' way for a plugin to actually achieve a game reset, a far more appropriate place for it would be a delegate method in the Runtime API, as per:
https://github.com/leezer3/OpenBVE/blob/master/source/OpenBveApi/Runtime/Runtime.cs#L40

This wouldn't affect the timing of existing method calls or break backwards compatability.

@zbx1425
Copy link
Contributor Author

zbx1425 commented Jul 16, 2021

Sorry for the disagreement.


Then I'll take the "'correct' way for a plugin to actually achieve a game reset" solution. I hope there's a function that when called will reset the game status and then reload the current route, train and train plugins, without restarting the process. This is roughly equivalent to loading a plugin before the route loads, with the difference of wasting some time (since the first loading got discarded).
By the way, I think with the OpenGLMenu addition, a way to reset the game / return to the main menu without restarting the process might be nice after all. (Like BVE5)


Please also remove the FileFormats since it's no longer needed, and might have a minor impact on loading performance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants