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

USDZ and Zip Compliance #3424

Open
dgovil opened this issue Nov 14, 2024 · 1 comment
Open

USDZ and Zip Compliance #3424

dgovil opened this issue Nov 14, 2024 · 1 comment

Comments

@dgovil
Copy link
Collaborator

dgovil commented Nov 14, 2024

Description of Issue

There is an unfortunate confluence of effects around USDZ's zip file.cpp that I would like to raise and suggest some solutions:

  1. The USDZ spec doesn't say that it is explicitly Zip32 , but the implementation is Zip32 only, and doesn't support Zip64.
  2. The USDZ spec says that it all USDZ files are compliant zip files.
  3. Zipfile.cpp doesn't read/validate the the central directory record and instead writes/reads the files linearly.

This means that you have the following issues:

  1. If you try and add a single file that is larger than 32bits, it will fail somewhat silently, as the values are invalid. See Creating large usdz files produces invalid files without error #3098
  2. You can add many files that are each under the 32bit limit but total over 32bits. This means that the central directory record (which is also 32bit) is out of spec as the offset values that point to the individual file offsets are invalid.

Therefore it's quite easy to make USDZ files that are technically corrupt zip files but USD will happily read again because it ignores the corrupt central directory listing..
However, it also means that you can easily corrupt the output file as well in silent ways, that USD will not read again.

I propose the following:

  1. We clarify in the documentation that it must be a Zip32. This will be part of the AOUSD specification language too.
  2. Zipfile.cpp should validate both the local file headers and the central directory record so we don't get invalid values
  3. We revisit zipfile.cpp to support more of the zip spec, starting with Zip64 support.

This is unlikely to be a security issue, but it is a data loss issue.

Steps to Reproduce

  1. Add a ton of files to a USDZ until you're over the 32bit limit.
  2. Check the central directory listing to see if the offsets point to the individual files.
dgovil added a commit to dgovil/USD that referenced this issue Nov 14, 2024
It is a common point of confusion that USDZ files are Zip64 compliant.

In the AOUSD specification, I'll be mentioning that we only support Zip32 and I'd like to do the same for the USDZ spec doc as well.

See PixarAnimationStudios#3424 for more information
@jesschimein
Copy link
Contributor

Filed as internal issue #USD-10442

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

No branches or pull requests

2 participants