nwnxee / unified

Binaries available under the Releases tab on Github
https://nwnxee.github.io/unified
GNU General Public License v3.0
129 stars 92 forks source link

Add basic file writing capacities #1723

Closed julien-lecomte closed 8 months ago

julien-lecomte commented 8 months ago

I currently export textual data when starting the server. My current method consists of using the log functions, and using the script name as a prefix to then grep the server logs and build a rst (sphinx documentation file).

During discussions, it was proposed I use sqlite instead. But that requires creating the schema, exporting to sqlite, and then extracting and formatting to rst. Much more complicated.

Being able to write arbitrary files also will allow me to keep a distinct warnings.log and errors.log, something like:

void LogWarning(string sLogEntry, string sConsoleColor = CONSOLE_COLOR_YELLOW);
void LogWarning(string sLogEntry, string sConsoleColor = CONSOLE_COLOR_YELLOW)
{
  WriteTimestampedLogEntryEx("warning:", sLogEntry, CONSOLE_COLOR_YELLOW);
  NWNX_File_Append("/../../logs/warnings.log", sLogEntry);
}

There's many other possibilities that open up when allowing file operations via NWNX.

I've added the strict minimum for now, but will augment the available functions depending on demand.

mtijanic commented 8 months ago

I just want to add an explanation for why this was closed, since the discussions were on Discord and not searchable, and someone else will have the same idea eventually.

The reason this PR is rejected is that the functions are deceptively tempting, but dangerous in ways that may not be obvious to our users. This means that anyone with the power to run nwscript can trivially take over the server in question. There are probably several existing RCEs in NWNX that can be triggered from nwscript (and so everyone should be very careful with those privileges), but the skill level required is orders of magnitude more complex than a oneliner to execute arbitrary shellcode. This is just us applying the Defense In Depth principle.

Everyone is, of course, free to make something like this themselves and use from a private fork/plugin, but hopefully that dissuades folks from using it. Instead, what we suggest is that we isolate the actual usecases people have and implement a more locked down version of the API that handles that, that is less likely to lead to accidental pwnage. This has happened before, when folks requested DeleteFile() (which is much less dangerous than writing files), but what they actually wanted was just a way to delete old BIC files, so we instead added NWNX_Administration_DeletePlayerCharacter() which handles that case only and is much harder to abuse.

Finally, note that this is not just the case of nannying someone from not shooting their foot off. I feel that reducing the security of internet-connected devices is an irresponsible thing to do, towards the general public. These machines then become part of networks used for spam, DDOS and other abuse that hurt everyone.