FunkinCrew
[ENHANCEMENT] FileUtil additions + sandboxing
Briefly describe the issue(s) fixed.
im currently working on a mod launcher/manager/updater mod, and require certain file manipulation functions that are blacklisted and also not currently present in FileUtil, so this pr aims to expand FileUtil functionality by adding the following:
moveFile(path:String, destination:String):Void
deleteFile(path:String):Void
getFileSize(path:String):Int
isDirectory(path:String):Bool
readDir(path:String):Array<String>
moveDir(path:String, destination:String, ?ignore:Array<String>):Void
deleteDir(path:String, recursive:Bool = false, ?ignore:Array<String>):Void
getDirSize(path:String):Int
rename(path:String, newName:String, keepExtension:Bool = true):Void
this pr also sandboxes all the functions!! the core functionality is now housed in FileUtilBase (used only internally, blacklisted in scripts), which FileUtil uses, sanitizing the paths first and preventing them from leaving the game folder
functions that have the capability to modify/delete are prevented from messing with assets, manifest and everything in it, plugins and everything in it, and the dlls and game executable
all functions above have been tested; if anyone finds any faults, please make them known
Wouldn't this theoretically allow mods to do malicious things? Unless somehow directories that are not Funkin's are restricted.
Wouldn't this theoretically allow mods to do malicious things? Unless somehow directories that are not Funkin's are restricted.
mods can already do that with the functions already in FileUtil like going ../ repeatedly upwards out of the game root directory and then back down to some important file and overwriting it or something
var root:String = "./";
while (FileUtil.doesFileExist(root)) {root += "../";}
FileUtil.writeStringToPath(root.substring(0, root.length - 6) + "some important path here", "", FileWriteMode.Force);
though sandboxing to only the game root directory is definitely needed i think thats an issue for another pr
Fair enough! Sandboxing would be very nice.
itd be an easy addition like using this in all the functions that have a path input
/**
* Prevent paths from exiting the root.
*
* @param path The path to sanitize.
* @return The sanitized path.
*/
public static function sanitizePath(path:String):String
{
path = path.trim().replace('\\', '/');
if (path.contains(':'))
{
path = path.substring(path.lastIndexOf(':') + 1);
}
while (path.charAt(0) == '/')
{
path = path.substring(1);
}
var parts:Array<String> = path.split('/');
var sanitized:Array<String> = [];
for (part in parts)
{
switch (part)
{
case '.':
case '':
continue;
case '..':
if (sanitized.length > 0) sanitized.pop();
default:
sanitized.push(part);
}
}
return sanitized.join('/');
}
however the problem is that theres a few places internally where they use the functions to do stuff outside of the root folder (unit tests/polymod asset redirect/etc)
Shouldn't it be better to extend FileUtilBase and override all functions including those that cause trouble or am i dumb ?
Shouldn't it be better to extend FileUtilBase and override all functions including those that cause trouble or am i dumb ?
will do that rn lol
Pretty interesting!
Of note, you could change the names so that the one that gets used by the core game is still FileUtil, and the one that the scripts uses is called something like FileUtilSandboxed, and then you can add a Polymod import alias which replaces imports of FileUtil with FileUtilSandboxed.
why isn't this merged yet..... grrrrr.....
this is a sizeable pr and also related to security stuff so im not surprised if it takes a while to review and merge
made a bunchh of changes; still need to test them all... but its open for review if anyone else wants to as well
I plan to review and merge this as-is if it's functional, but one change to make for the future (to help with #4737) is to disallow URLs from FileUtilSanitized.openFolder, because this function can be used to access any URL (openFolder("https://www.google.com/") worked for me on Desktop)
I plan to review and merge this as-is if it's functional, but one change to make for the future (to help with #4737) is to disallow URLs from
FileUtilSanitized.openFolder, because this function can be used to access any URL (openFolder("https://www.google.com/")worked for me on Desktop)
noted adding to pr asap
alsoo we need to figure out how to reliably resolve symlinks because currently the root can be exited via a relative symlink...
sys.FileSystem.fullPath despite being documented with "symlinks will be followed and the path will be normalized" it wasnt able to resolve their true paths on my machine (windows 11)
alsoo we need to figure out how to reliably resolve symlinks because currently the root can be exited via a relative symlink...
I think this is fine as long as the game doesn't provide a way to create a symlink. If it doesn't, exploiting this would require an existing symlink inside the Funkin' game directory.
This was partially merged in 0.6.3, it's just missing the fixes + stronger sandboxing commit
idk why html5 was even trying to create a directory in the first place but this commit shouldd fix the issue
This puts the PR into sort of a weird place since it's already in the game, but I'll make sure this commit gets reviewed!
This puts the PR into sort of a weird place since it's already in the game, but I'll make sure this commit gets reviewed!
re-reviewed
The commit was made after the update, so it will require reviewing and merging for 0.6.4
the new commits look fine, just gotta fix the conflicts and we should be good on this one.
