Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

forcefully add .txt to all safe IO files #675

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ASpoonPlaysGames
Copy link
Contributor

This is a breaking change, all existing safe IO files wont be able to be read anymore since they wont have the .txt extension. I guess just get people to add it manually or something idk.

There is a absolutely a better solution than this, but yeah

@Alystrasz
Copy link
Contributor

What's the reason for imposing the txt file extension?

@ASpoonPlaysGames
Copy link
Contributor Author

What's the reason for imposing the txt file extension?

To stop people from writing executable scripts like .bat files

@Alystrasz Alystrasz added needs testing Changes from the PR still need to be tested needs code review Changes from PR still need to be reviewed in code labels Mar 12, 2024
@Alystrasz
Copy link
Contributor

@ASpoonPlaysGames build fails

@EladNLG
Copy link
Member

EladNLG commented Mar 13, 2024

Add .json as an exception.

Safe I/O supports JSON and there is prractically no reason for .json to not be an available file extension.

Additionally, people who already use .txt as a file extension will have their files saved as file_name.txt.txt, which is insanely dumb.

@EladNLG
Copy link
Member

EladNLG commented Mar 13, 2024

And, if we're talking making executable scripts, isn't much easier to just... open a link which downloads an .exe and tell the user to open it? The user has to MANUALLY open their titanfall 2 dir and go to R2Northstar/save_files/MOD.NAME/...

Shouldnt we look into that as well?

@Jan200101
Copy link
Member

Add .json as an exception.

I don't think it matters what extension the file has at the end, as long as it can't result in code execution by PEBKAC.

I do take issue with .txt being the default, the data being stored doesn't necessarily have to be plaintext.
.dat would probably be way better (though perhaps a whitelist of extensions with a default appended .dat would be even more better for developers)

@GeckoEidechse
Copy link
Member

The user has to MANUALLY open their titanfall 2 dir and go to R2Northstar/save_files/MOD.NAME/...

Not really. Before #674 LaunchExternalWebBrowser() had a bad check that (combined with Windows allowing to cd into and out of non-existing dirs), meant that LaunchExternalWebBrowser("http\\..\\R2Northstar\\save_data\\Gecko.AwesomeMod\\my.bat", WEBBROWSER_FLAG_FORCEEXTERNAL) would attempt to run http\\..\\R2Northstar\\save_data\\Gecko.AwesomeMod\\my.bat which given that .bat files are treated as executable on Windows, it would then run that batch script.

The same cannot happen with .txt as it's not treated as executable on Windows.

We patched LaunchExternalWebBrowser() ofc, so the exploit chain is already broken but we still want to consider preventing writing files that are treated as "executable" by default in Windows ^^

@ASpoonPlaysGames
Copy link
Contributor Author

I do take issue with .txt being the default, the data being stored doesn't necessarily have to be plaintext.

Literally how, safe IO works in strings from squirrel, so writing a binary format is just not gonna go well

@EladNLG
Copy link
Member

EladNLG commented Mar 13, 2024

The user has to MANUALLY open their titanfall 2 dir and go to R2Northstar/save_files/MOD.NAME/...

Not really. Before #674 LaunchExternalWebBrowser() had a bad check that (combined with Windows allowing to cd into and out of non-existing dirs), meant that LaunchExternalWebBrowser("http\\..\\R2Northstar\\save_data\\Gecko.AwesomeMod\\my.bat", WEBBROWSER_FLAG_FORCEEXTERNAL) would attempt to run http\\..\\R2Northstar\\save_data\\Gecko.AwesomeMod\\my.bat which given that .bat files are treated as executable on Windows, it would then run that batch script.

The same cannot happen with .txt as it's not treated as executable on Windows.

We patched LaunchExternalWebBrowser() ofc, so the exploit chain is already broken but we still want to consider preventing writing files that are treated as "executable" by default in Windows ^^

I think its still a problem that a mod can easily can do LaunchExternalWebBrowser( "https://shadywebsite.com/NotAVirus.exe" ) and start a download whilst prompting the user to open the file.

@GeckoEidechse
Copy link
Member

GeckoEidechse commented Mar 13, 2024

I think its still a problem that a mod can easily can do LaunchExternalWebBrowser( "https://shadywebsite.com/NotAVirus.exe" ) and start a download whilst prompting the user to open the file.

Probably but at least that requires an interaction from the user. The other one was zero-click.

Nevertheless best to make an issue for this IMO so that we can keep track of it as it's unrelated to this PR at this point ^^

@EladNLG
Copy link
Member

EladNLG commented Mar 13, 2024

Add .json as an exception.

I don't think it matters what extension the file has at the end, as long as it can't result in code execution by PEBKAC.

I do take issue with .txt being the default, the data being stored doesn't necessarily have to be plaintext.
.dat would probably be way better (though perhaps a whitelist of extensions with a default appended .dat would be even more better for developers)

The data, necessarily, has to be plaintext.

@Jan200101
Copy link
Member

Literally how, safe IO works in strings from squirrel, so writing a binary format is just not gonna go well

despite that its not uncommon to use a string as a container for binary data.
I'm not sure exactly how Squrirel, let alone the squirrel in the game, handles this, but this is a common pattern in similar languages most notably Python and Lua.

@ASpoonPlaysGames
Copy link
Contributor Author

despite that its not uncommon to use a string as a container for binary data. I'm not sure exactly how Squrirel, let alone the squirrel in the game, handles this, but this is a common pattern in similar languages most notably Python and Lua.

strings in squirrel are null terminated, so good luck making a decent binary format with that. There is also not really any need for binary formats, just encode things as json and parse once on load, you arent gonna be doing things like texture streaming through this come on now

@EladNLG
Copy link
Member

EladNLG commented Mar 13, 2024

despite that its not uncommon to use a string as a container for binary data. I'm not sure exactly how Squrirel, let alone the squirrel in the game, handles this, but this is a common pattern in similar languages most notably Python and Lua.

strings in squirrel are null terminated, so good luck making a decent binary format with that. There is also not really any need for binary formats, just encode things as json and parse once on load, you arent gonna be doing things like texture streaming through this come on now

Fun fact:

They are not null terminated.

Despite that, you cannot store binary data through safe i/o since we restrict data to only consist of ASCII characters.

@RoyalBlue1
Copy link
Contributor

despite that its not uncommon to use a string as a container for binary data. I'm not sure exactly how Squrirel, let alone the squirrel in the game, handles this, but this is a common pattern in similar languages most notably Python and Lua.

strings in squirrel are null terminated, so good luck making a decent binary format with that. There is also not really any need for binary formats, just encode things as json and parse once on load, you arent gonna be doing things like texture streaming through this come on now

Fun fact:

They are not null terminated.

yes but almost every function interacting with them expects them to be and the last byte of the string is set to 0 in the contstructor

@Jan200101
Copy link
Member

Despite that, you cannot store binary data through safe i/o since we restrict data to only consist of ASCII characters.

Do we?
The only code I can find that restricts a string to ascii is related to the path, which I think makes sense
But file content only has a null byte check (which may be wrong here since the function includes a comment that says its suppose to check the path?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs code review Changes from PR still need to be reviewed in code needs testing Changes from the PR still need to be tested
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

None yet

6 participants