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

Improve mesh accessor validation #234

Merged
merged 2 commits into from
Nov 27, 2023
Merged

Improve mesh accessor validation #234

merged 2 commits into from
Nov 27, 2023

Conversation

zeux
Copy link
Contributor

@zeux zeux commented Oct 26, 2023

This change attempts to flag more invalid glTF files as invalid when this likely interferes with the mesh processing by causing crashes.

  • Fix parsing of cgltf_size by using cgltf_json_to_size in a couple places where int was used erroneously and replacing negative numbers with 0. We would previously return -1 when the JSON token was invalid and an arbitrary negative value if a negative number was present, but this runs a high risk of an integer overflow during any of the size computation, which causes a security risk. In the future it would be nicer to use a limit for positive values as well (for example, glTF spec says that all sizes should be limited to 2^53), as with this change there is still a risk of overflow during multiplication.

  • Fix validation of accessors - an invalid component type or type may lead to computed accessor stride to be 0 which may invalidate all sorts of logic. Both component type and type are mandatory per glTF spec.

  • Make sure that indices in mesh primitives have no custom stride. This is enforced by the glTF spec, and cgltf_calc_index_bound ignores the stride, which makes it possible to craft an index buffer that passes index validity checks but results in bogus indices later. This observation may also allow us to simplify cgltf_accessor_unpack_indices in the future.

  • Make sure that there's at least one primitive. This is guaranteed by the spec, and without this it's possible to specify indices without vertices.

  • Make sure that there's at least one vertex. This is guaranteed by the spec as all accessors must have count >= 1 (for now this is checked just for attribute accessors, in the future it might make sense to move this check to accessor validation). Without this, it's possible to have an index buffer filled with zeroes (without a buffer view, accessors are zero-initialized...), so we'd never check the index bound.

This change attempts to flag more invalid glTF files as invalid when
this likely interferes with the mesh processing by causing crashes.

- Fix parsing of cgltf_size by using cgltf_json_to_size in a couple
  places where int was used erroneously and replacing negative numbers
  with 0. We would previously return -1 when the JSON token was invalid
  and an arbitrary negative value if a negative number was present, but
  this runs a high risk of an integer overflow during any of the size
  computation, which causes a security risk. In the future it would be
  nicer to use a limit for positive values as well (for example, glTF
  spec says that all sizes should be limited to 2^53), as with this
  change there is still a risk of overflow during multiplication.

- Fix validation of accessors - an invalid component type or type may
  lead to computed accessor stride to be 0 which may invalidate all
  sorts of logic. Both component type and type are mandatory per glTF
  spec.

- Make sure that indices in mesh primitives have no custom stride. This
  is enforced by the glTF spec, and cgltf_calc_index_bound ignores the
  stride, which makes it possible to craft an index buffer that passes
  index validity checks but results in bogus indices later. This
  observation may also allow us to simplify cgltf_accessor_unpack_indices
  in the future.

- Make sure that there's at least one primitive with at least one
  vertex. Both are guaranteed by the spec, as accessor count has to be
  at least 1, and without this it's possible to specify indices without
  vertices, and we never check the indices.
@zeux
Copy link
Contributor Author

zeux commented Oct 26, 2023

This is easier to review with ignored whitespace in the diff. Also note that I am discovering these as I go; we previously never asked the question, can you meaningfully use various cgltf accessor functions on the result of cgltf_parse, and these changes are attempting to answer that :) I expect there will be a few more PRs in the coming days that fix corner cases like this.

zeux added a commit to zeux/meshoptimizer that referenced this pull request Oct 26, 2023
This removes a few extra corner cases around mesh processing.

Upstream PR: jkuhlmann/cgltf#234
zeux added a commit to zeux/meshoptimizer that referenced this pull request Nov 2, 2023
This includes the up to date master plus unmerged
jkuhlmann/cgltf#234; there's other open PRs but
this is the most important one since it fixes various under/overflow
conditions.
@jkuhlmann jkuhlmann merged commit 5a9f50f into jkuhlmann:master Nov 27, 2023
3 checks passed
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

Successfully merging this pull request may close these issues.

2 participants