[Safe I/O] Only allow creating files with whitelisted filetypes #682
Conversation
|
I wrote this on a system that is unable to compile Northstar and without having written C++ for like more than half a year, would be surprised if it even compiles xD |
|
@ASpoonPlaysGames is also working on that (#675), are you aware of that? 😄 |
ngl that PR is kinda dead, i've just not had the time to do much lately. Honestly this PR might be preferable to mine. |
primedev/mods/modsavefiles.cpp
Outdated
| } | ||
|
|
||
| // TODO: move into list of global consts? | ||
| std::set<std::string> whitelist = {".txt", ".json"}; |
There was a problem hiding this comment.
Is there a reason for this being a set instead of like an vector or array or is it just an arbitrary choice?
There was a problem hiding this comment.
Mostly arbitrary choice
There was a problem hiding this comment.
Then it should probably be a vector.
There was a problem hiding this comment.
can probably also be made static const
compiler might already optimise it to static idrk but just in case it isn't doing that, this'll prevent unnecessary allocs
There was a problem hiding this comment.
@barnabwhy done via 314b86a
@catornot going back to this, wouldn't a set make more sense as soon as the list of extensions gets quite long? Like rn it's only 2 cause that's the two most important plaintext filetypes I could think of but the idea is that the list would get extended quite a bit later down the line.
There was a problem hiding this comment.
how many filetypes will you need?
|
@GeckoEidechse you created the branch on the main repo so we cannot update it 😢 |
| std::string extension = file.extension().string(); | ||
| if (whitelist.find(extension) == whitelist.end()) | ||
| { | ||
| spdlog::error("SAVE FAILED!"); |
There was a problem hiding this comment.
This should maybe make it more clear that it's an issue with a mod so users don't freak out.
| // Check if has extension and return early if not | ||
| if (!file.has_extension()) | ||
| { | ||
| spdlog::error("SAVE FAILED!"); |
|
This isn't directly related to the PR but I noticed while reviewing that the code immediately below directly locks and unlocks the mutex, but ideally it should be using |
Only allow creating files with whitelisted filetypes to prevent writing
.bator.vbfiles with Safe I/O that could then be called using some exploit (e.g. #674), we should prevent writing file types that by default are interpreted as executable.Whitelisting was chosen over blacklisting as we can always extend the list and don't have to worry about potentially forgetting to blacklist a certain filetype.
Of course this doesn't prevent writing a bash script to a
.txtfile and then somehow getting Windows to interpret it as a batch file.Reading files is not restricted to filetypes as we are primarily concerned with creating files here.
Supersedes #675
Completely untested atm.
Testing instructions:
(to be extended)
Basically write a save I/O file with and without a disallowed file extension and check that it handles it respectively.