Fix for non-deterministic behavior in PCKPacker #81280
Conversation
akien-mga
changed the title
There was a problem hiding this comment.
I guess why not, but the same changes should be done it editor/export/editor_export_platform.cpp as well, PCKPacker is not used in export, it is for creating PCK files for the user code only.
padding being random does not have any cryptographic significance.
There's no "cryptographic significance" in a PCK encryption in general. The key is stored in the executable as the plain text, and trivially extracted, so it's just a mild deterrent for script kiddies who are too lazy to google for a few tools.
If there's a desire to indroduce something that functions as a nonce it should probably be added explicitly and only if encryption is enabled.
Underlying encryption API is already using randomize IV.
|
Thanks for the quick review! I'll make the requested changes in editor/export/editor_export_platform.cpp as well.
I guess I was assuming folks would be distributing their decryption keys with some server API call or something. There's value in shipping a PCK with your build (with arbitrary app store or console review delays) and then having the server be able to "reveal" the keys at some future point being (like for an event or something) and unlock the content. We used a similar process on Fortnite for shipping skins we didn't want to leak ahead of a store feature. |
|
Changes seem good, but I'll let bruvzg give the final approval. Could you squash the commits? See PR workflow for instructions. |
PCK files (like other build products) should be deterministic based on their inputs. Removed calls to Math::rand() that are being used to generate padding. Looks like these were introduced as part of adding encryption support, but the padding being random does not have any cryptographic significance. This can be trivially inferred since file blocks that happen to be aligned don't get padding anyway. If there's a desire to indroduce something that functions as a nonce it should probably be added explicitly and only if encryption is enabled. remove Math::rand() calls in editor_export_platform.cpp follow up to make consistent with pck_packer
ogapo
force-pushed
the
deterministic_pcks
branch
from
7a5ff37 to
067807c
Compare
akien-mga
added
the
cherrypick:4.1
akien-mga
added
the
cherrypick:3.x
|
Thanks! And congrats for your first merged Godot contribution 🎉 |
YuriSizov
removed
the
cherrypick:4.1
|
Cherry-picked for 4.1.3. |
PCK files (like other build products) should be deterministic based on their inputs. This fix removes calls to Math::rand() that are being used to fill in padding (0-padding is preferred).
It looks like these lines were introduced as part of adding encryption support, but the padding being random does not have any cryptographic significance. This can be trivially inferred since file blocks that happen to be aligned don't get padding at all and would receive no contribution to their entropy.
If there's a desire to indroduce something that functions as a nonce it should probably be added explicitly and only if encryption is enabled. That said, such a feature would probably be more simply implemented (with no additional code) by just changing the encrypted assets or cycling the keys used for the pck file, so I've made no attempt to address it with this fix.