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.

Sign up for GitHub

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

Jump to bottom

Rust soundness fixes #7518

Merged
merged 15 commits into from
Sep 29, 2022
Merged

Rust soundness fixes #7518

merged 15 commits into from
Sep 29, 2022

Conversation

tustvold
Copy link
Contributor

@tustvold tustvold commented Sep 9, 2022 •
edited
Loading

There are various internal APIs exposed by the flatbuffer crate, and in the code it generates that are unsound, in that they permit undefined behaviour through safe APIs. It is extremely unlikely any downstreams are actually making use of these APIs and even less likely that they are doing so in ways that result in UB, however, the medium of RUSTSEC is not very effective to convey this subtlety. I think it is a shame, not to mention unfair, that this crate gets labelled "unsafe" or "dangerous" as a result of this, and so I'd like to do something to help fix this. Part of this is a selfish desire to stop having the same discussions repeatedly.

As originally proposed by @CasperN (#6627 (comment)) this makes the methods taking unvalidated &[u8] unsafe. The result is that if users are only using safe APIs, which force validation to be performed, they should be protected from UB => the crate is sound

I will endeavour to highlight the non-mechanical changes in the diff, but the major changes are:

  • It makes the two trait methods Follow::follow and Push::push unsafe
  • Push::push is provided with the already written length, but not the already written data (to help with optimisation of loops)
  • It makes the various constructors Array::new, Table::new, Vector::new, VectorIter::from_slice, VTable::init unsafe
  • buffer_has_identifier and the auto-generated *_buffer_has_identifier and *_size_prefixed will panic if given too small a buffer
  • Removes SafeSliceAccess for types with alignment or value constraints (e.g. bool, u32, f32, etc...)
  • Table accessors now always return Vector<'a, T> instead of sometimes returning slices
  • EndianScalar uses an associated type Scalar to allow endianness conversions for non-trivially transmutable types such as enumerations

@google-cla
Copy link

google-cla bot commented Sep 9, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@github-actions github-actions bot added c++ codegen Involving generating code from schema rust labels Sep 9, 2022
@tustvold tustvold mentioned this pull request Sep 9, 2022
Open
@alamb
Copy link

alamb commented Sep 9, 2022

For context, the relevant rustsec issue is https://rustsec.org/advisories/RUSTSEC-2021-0122.html

@dbaileychess
Copy link
Collaborator

@CasperN I'll let you handle this one.

@dbaileychess
Copy link
Collaborator

@tustvold You'll need to sign the CLA as well

CasperN
CasperN reviewed Sep 12, 2022
View reviewed changes
Copy link
Collaborator

@CasperN CasperN left a comment

Choose a reason for hiding this comment

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

Thanks for this change! This is really impactful

rust/flatbuffers/src/endian_scalar.rs Show resolved Hide resolved
rust/flatbuffers/src/follow.rs Outdated Show resolved Hide resolved
rust/flatbuffers/src/verifier.rs Show resolved Hide resolved
rust/flatbuffers/src/vector.rs Show resolved Hide resolved
rust/flatbuffers/src/push.rs Show resolved Hide resolved
src/idl_gen_rust.cpp Show resolved Hide resolved
@CasperN CasperN mentioned this pull request Sep 12, 2022
Closed
4 tasks
@CasperN
Copy link
Collaborator

CasperN commented Sep 28, 2022

Are we all ready to merge?

@tustvold
Copy link
Contributor Author

I'm happy if you're happy 😄 I don't think I've missed anything...

@CasperN
Copy link
Collaborator

CasperN commented Sep 28, 2022

I think this is great. Thank you for this PR!

@TethysSvensson @alamb any further comments? If not, I'll submit once the branch is up to date and CI is green

@TethysSvensson
Copy link
Contributor

I don't have any more comments right now. I might have time to take another look for unsoundness at a later stage.

@CasperN CasperN enabled auto-merge (squash) September 29, 2022 13:56
@CasperN CasperN merged commit 374f8fb into google:master Sep 29, 2022
@tustvold
Copy link
Contributor Author

image

Much approval 😆

I'm not sure what the release schedule for this repo is, but I at least would be very interested in testing out a pre-release build of this if such a thing were possible. This would help iron out any kinks that result from these changes

@CasperN
Copy link
Collaborator

CasperN commented Sep 29, 2022

We need both a release of flatc (@dbaileychess) and the flatbuffers crate to get this change out there

@tustvold
Copy link
Contributor Author

tustvold commented Sep 29, 2022 •
edited
Loading

For my purposes we vendor the generated code, and so I would be happy with just a pre-release of the crate, we already have instructions for compiling flatc from source (as the versions in most distributions are incredibly old)

@alamb alamb mentioned this pull request Sep 29, 2022
Merged
@dbaileychess dbaileychess added the release-notes The PR should be highlighted in the release notes of the next release it is in. label Sep 29, 2022
@dbaileychess
Copy link
Collaborator

Release in: https://github.com/google/flatbuffers/releases/tag/v22.9.29 Haven't pushed to crates.io since I don't have permissions yet.

@tustvold
Copy link
Contributor Author

This has a number of substantial breaking changes, it probably needs to be a major release. Sorry i should have made that clear

@CasperN CasperN mentioned this pull request Oct 3, 2022
Merged
@y0nil
Copy link

y0nil commented Oct 3, 2022

Release in: https://github.com/google/flatbuffers/releases/tag/v22.9.29 Haven't pushed to crates.io since I don't have permissions yet.

Hi @dbaileychess / @tustvold,

Version v22.9.29 of flatc and version <= 2.1.2 of the flatbuffers crate are incompatible.
Is there an estimation for when a matching version will be available on crates.io?

@CasperN
Copy link
Collaborator

CasperN commented Oct 3, 2022

I want #7553 to go in before pushing, but otherwise it can go in EoWeek

@tustvold
Copy link
Contributor Author

No rush, but just wondering where we have got to with this?

@CasperN
Copy link
Collaborator

CasperN commented Oct 18, 2022 •
edited
Loading

Ok, #7553 did not make it, I'll push the publish button (once I get #7574 merged)

@CasperN CasperN mentioned this pull request Oct 18, 2022
Merged
@CasperN
Copy link
Collaborator

CasperN commented Oct 18, 2022

released!

@tustvold tustvold mentioned this pull request Oct 18, 2022
Merged
@enum-class enum-class mentioned this pull request Dec 7, 2022
Merged
jochenparm pushed a commit to jochenparm/flatbuffers that referenced this pull request Oct 29, 2024
@tustvold
Rust soundness fixes (google#7518)
357b0dd 
* Rust soundness fixes

* Second pass

* Make init_from_table unsafe

* Remove SafeSliceAccess

* Clippy

* Remove create_vector_of_strings

* More clippy

* Remove deprecated root type accessors

* More soundness fixes

* Fix EndianScalar for bool

* Add TriviallyTransmutable

* Add debug assertions

* Review comments

* Review feedback
jochenparm pushed a commit to jochenparm/flatbuffers that referenced this pull request Oct 29, 2024
@tustvold
Rust soundness fixes (google#7518)
219d727 
* Rust soundness fixes

* Second pass

* Make init_from_table unsafe

* Remove SafeSliceAccess

* Clippy

* Remove create_vector_of_strings

* More clippy

* Remove deprecated root type accessors

* More soundness fixes

* Fix EndianScalar for bool

* Add TriviallyTransmutable

* Add debug assertions

* Review comments

* Review feedback
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TethysSvensson TethysSvensson TethysSvensson left review comments

@alamb alamb alamb left review comments

@CasperN CasperN CasperN approved these changes

Assignees
No one assigned
Labels
c++ codegen Involving generating code from schema release-notes The PR should be highlighted in the release notes of the next release it is in. rust
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@tustvold @alamb @dbaileychess @CasperN @TethysSvensson @y0nil