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

Adds PCK encryption support (using script encryption key for export). #38308

Merged
merged 1 commit into from
Sep 7, 2020

Conversation

bruvzg
Copy link
Member

@bruvzg bruvzg commented Apr 28, 2020

  • Adds optional PCK encryption (per file encryption + directory encryption) to export PCK and PCKPacker.

  • Changes FileAccessEncrypted's encryption mode from ECB to CFB (encryption/decryption is done in the one sequential pass anyway, there's no reason to use ECB).

  • Adds support for whitebox decryption code instead of hardcoded key. Actual decryption function implementation should be supplied by user. Example generated by https://github.com/balena/aes-whitebox is provided for testing purpose.
    Removed, will add it as separate PR if I'll find usable open-source implementation.

menu

Related issue #24716.

@bruvzg bruvzg force-pushed the sad_security_circus branch from 174e47e to 7c87f9f Compare April 30, 2020 15:23
@bruvzg bruvzg requested a review from vnen as a code owner April 30, 2020 15:23
@bruvzg
Copy link
Member Author

bruvzg commented Apr 30, 2020

Changed it to the per-file encryption (file name filter in the export settings + optional directory encryption to hide file names), previous approach was decrypting whole PCK to the memory, every time file in it is opened.

@bruvzg bruvzg force-pushed the sad_security_circus branch 3 times, most recently from 8c99b13 to cd178e8 Compare May 1, 2020 11:54
@bruvzg bruvzg requested a review from bojidar-bg as a code owner May 1, 2020 11:54
@bruvzg bruvzg force-pushed the sad_security_circus branch 7 times, most recently from af372b4 to 8b956fd Compare May 3, 2020 11:13
@vnen
Copy link
Member

vnen commented May 4, 2020

I was wondering how this behaves with large PCK files, but with per-file encryption it shouldn't be so bad.

@bruvzg
Copy link
Member Author

bruvzg commented May 4, 2020

I was wondering how this behaves with large PCK files, but with per-file encryption it shouldn't be so bad.

With mindless "encrypt everything" approach it's quite slow. Excluding textures (*.stex) from the encryption makes it fast enough even with the white-box decryption (which is significantly slower than release build with default mbedtls AES). With the reasonable filters for hiding sensitive information only, overhead should be negligible.

@TisFoolish
Copy link

Can the key that is stored in the binary be used to decrypt the pck file (a given), change the content (eg make walls translucent), then re-encrypt modified game data? Encryption is not something I'm that knowledgeable in so sorry if this is a question with what should be an obvious answer.

@bruvzg
Copy link
Member Author

bruvzg commented May 5, 2020

Can the key that is stored in the binary be used to decrypt the pck file (a given), change the content (eg make walls translucent), then re-encrypt modified game data? Encryption is not something I'm that knowledgeable in so sorry if this is a question with what should be an obvious answer.

Yes, if get the key out of the binary you can do this. With white-box encryption there is no plain key stored in the binary, it's mixed with random data and code and attacker will need to find and reverse-engineer decryption code, it's still possible but harder to do.

@TisFoolish
Copy link

TisFoolish commented May 5, 2020

Someone in the future is probably going to request something like an option to use public key encryption as a preventative measure against cheaters in multiplayer games. That or do it themselves.

Personally I don't have a use for it as I'm just doing either single player games or small, co-op games.

@bruvzg
Copy link
Member Author

bruvzg commented May 5, 2020

Someone in the future is probably going to request something like an option to use public key encryption as a preventative measure against cheaters in multiplayer games. That or do it themselves.

It's always possible to modify binary, and bypass encryption altogether, and it's possible to manipulate decrypted resources in the process or/and GPU memory. Adding asymmetric cryptography probably won't make it any harder.

And relying on encryption as the only multiplayer anti-cheat measure is a bad idea anyway. Proper solution to prevent wall hacks is restricting the information sent by the server - calculate visibility on the server and give only visible opponent data to the clients and so on.

@mhilbrunner
Copy link
Member

mhilbrunner commented May 5, 2020

Someone in the future is probably going to request something like an option to use public key encryption as a preventative measure against cheaters in multiplayer games. That or do it themselves.

Personally I don't have a use for it as I'm just doing either single player games or small, co-op games.

That is not how public key encryption works. I guess you could use it to sign your binary or assets, but that doesn't really help against someone simply taking out the check for that.

But I guess that's what bruvzg already outlined in the previous comment, which I somehow missed :P

@bruvzg bruvzg changed the title [WIP] Adds PCK encryption support (using script encryption key for export). Adds PCK encryption support (using script encryption key for export). May 13, 2020
@bruvzg bruvzg force-pushed the sad_security_circus branch from 6550f3d to 1006779 Compare June 23, 2020 07:22
@bruvzg bruvzg mentioned this pull request Jul 27, 2020
@bruvzg bruvzg force-pushed the sad_security_circus branch 3 times, most recently from 42b4d3e to ede0e6c Compare September 3, 2020 06:22
Comment on lines +257 to +260
const int file_num = files.size();
if (p_verbose && (file_num > 0)) {
if (count % 100 == 0) {
printf("%i/%i (%.2f)\r", count, files.size(), float(count) / files.size() * 100);
printf("%i/%i (%.2f)\r", count, file_num, float(count) / file_num * 100);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change was added to suppress MSVC: warning C4723: potential divide by 0 error.

@bruvzg bruvzg force-pushed the sad_security_circus branch 2 times, most recently from e150f3f to 392dfda Compare September 4, 2020 05:28
Copy link
Collaborator

@Faless Faless left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really good work! 🚀 🏅

One thing I noticed, is that the Script export tab is a bit misleading now.
Setting Script Export Mode to Encrypted do not actually encrypt scripts unless *.gd files are included in the filter (which is not the default).
This is partly unrelated to this PR (since it was implemented in modules/gdscript/register_types.cpp and removed in recent GDScript update).

My suggestion is to have the Encryption tab always shown (maybe with an on/off switch) remove the Script tab, and move the Script Export Mode option to Resources with just the option: Text and Compiled (since Encryption will be handled by the dedicated section). This should also potentially allow to encrypt "compiled" gdscript.

What do you think?

Change default encryption mode from ECB to CFB.
@bruvzg bruvzg force-pushed the sad_security_circus branch from 392dfda to f043eab Compare September 5, 2020 11:54
@bruvzg
Copy link
Member Author

bruvzg commented Sep 5, 2020

My suggestion is to have the Encryption tab always shown (maybe with an on/off switch) remove the Script tab, and move the Script Export Mode option to Resources with just the option: Text and Compiled (since Encryption will be handled by the dedicated section). This should also potentially allow to encrypt "compiled" gdscript.

Done.

Screenshot 2020-09-05 at 14 52 25

(When "Encrypt exported PCK" is unchecked, the rest of the encryption option are grayed-out).

@NHodgesVFX
Copy link
Contributor

I think this will be great to add. Although anything can be broken some licences for 3d models and other assets require that you try to protect the assets.

Copy link
Collaborator

@Faless Faless left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank is amazing work 🚀 ! Thank you!

@akien-mga akien-mga merged commit 3c42d57 into godotengine:master Sep 7, 2020
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

BTW, one thing I thought about and mentioned to @Faless today is that it could be good UX-wise to validate the encryption key in the configured export template when exporting with encryption enabled.

This could be done e.g. by exporting a temporary project with a simple magic string and see if it can be decoded properly (using OS.execute). Or possibly running a script directly with godot -s, though I think the -s feature might not be exposed to non-tools builds currently.

@Xrayez
Copy link
Contributor

Xrayez commented Sep 13, 2020

Or possibly running a script directly with godot -s, though I think the -s feature might not be exposed to non-tools builds currently.

Should be possible with #41190.

@rieniter
Copy link

Does this feature still available on godot 3.2.4 ? As I compile this version but still not found it
2021-02-24_043248

@bruvzg bruvzg deleted the sad_security_circus branch February 23, 2021 21:34
@NHodgesVFX
Copy link
Contributor

Does this feature still available on godot 3.2.4 ? As I compile this version but still not found it
2021-02-24_043248

Pretty sure this feature is in godot 4 not 3

@bruvzg
Copy link
Member Author

bruvzg commented Feb 23, 2021

Does this feature still available on godot 3.2.4 ? As I compile this version but still not found it

No, it's for 4.0 only.

@Calinou
Copy link
Member

Calinou commented Jun 24, 2021

With mindless "encrypt everything" approach it's quite slow. Excluding textures (*.stex) from the encryption makes it fast enough even with the white-box decryption (which is significantly slower than release build with default mbedtls AES). With the reasonable filters for hiding sensitive information only, overhead should be negligible.

Should we add *.stex to the encryption exclude filter by default? It's not very useful to encrypt those textures as they can be gathered using tools like RenderDoc or apitrace anyway.

@bruvzg
Copy link
Member Author

bruvzg commented Jun 24, 2021

Should we add *.stex to the encryption exclude filter by default? It's not very useful to encrypt those textures as they can be gathered using tools like RenderDoc or apitrace anyway.

Maybe even default it to the .godot/imported/*.

@kienvn
Copy link

kienvn commented Jun 25, 2021

With mindless "encrypt everything" approach it's quite slow. Excluding textures (*.stex) from the encryption makes it fast enough even with the white-box decryption (which is significantly slower than release build with default mbedtls AES). With the reasonable filters for hiding sensitive information only, overhead should be negligible.

Should we add *.stex to the encryption exclude filter by default? It's not very useful to encrypt those textures as they can be gathered using tools like RenderDoc or apitrace anyway.

We should have option select encrypt whole file data or encrypt 1000 first bytes of file data (textures).

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

Successfully merging this pull request may close these issues.