Tracking issue: Rust buffer verification

[rw]

This is a tracking issue to document design and implementation of a verifier system for Rust Flatbuffers. I'm thinking that we can clone the logic from the C++ codebase.

The benefit is twofold:

1. Verifiers let users check if data is valid, thereby providing a security check for unknown data.
2. If a buffer is verified, we can justify using more unsafe pointer access in Rust, thereby removing bounds checking.

Anyone have thoughts on this? @aardappel

[rw]

cc @ry

[aardappel]

It be nice to run a test to see what speedup Rust gets from not using bounds checks.

Besides that, another advantage of a Verifier (as opposed to catching an out of bound error) is that it is all in one place (catching bounds checks is hard to make modular). Also may be easier to debug.

[pwrdwnsys]

As an interim mitigation, are there any examples of creating and using a Rust FFI wrapper for the C++ buffer verifier?

[rw]

@pwrdwnsys Not that I know of, but @aardappel would be able to chime in. I suspect it would be straightforward to do that.

[rw]

@jean-airoldie and @tymcauley: You both have been making excellent contributions to the Rust port. Would either or both of you be interested in working on the Rust Verifier functionality? I would've emailed you personally, but I don't know how to get in touch with either of you :-) If you want to email me, link is in my profile.

[tymcauley]

@rw I might be interested in taking a look at that at some point (work's busy right now, so it might not be for a week or two). I'll let you know if I've got any questions about the implementation you had in mind.

[jean-airoldie]

@rw I'm afraid I'm quite busy at the moment and I can't really commit to anything too time consuming.

[jean-airoldie]

@rw Alright I got some time now. Email me at maxence.caron@protonmail.com.

I was thinking of maybe creating a flatbuffers-src crate which would expose the flatbuffers source code, similarly to openssl-src. Then we could use the c++ buffer verifier via cffi. This source lib could also be used to build flatc from source, which would solve #5216.

[purtato]

Just in-case no one is aware of this, someone has written their own verifier:
https://github.com/nervosnetwork/cfb
https://lib.rs/crates/flatbuffers-verifier

[aardappel]

@goodsauce @doitian "Canonical FlatBuffers".. sounds interesting, is this a new encoding? Some more details would be welcome.

[doitian]

@goodsauce @doitian "Canonical FlatBuffers".. sounds interesting, is this a new encoding? Some more details would be welcome.

No, it's just a strict FlatBuffers builder. And our verifier implementation has nothing to do with CFB, its for the reader implemented here. We use FlatBuffers to parse message received from the P2P network. Without a verifier, any accessor may panic. So we build a generator via Python and Jinja as a temporary solution. The code has been moved to branch legacy, and here is an example of the generated verifier code: https://github.com/nervosnetwork/cfb/blob/legacy/tests/common/scalars_with_different_size_generated_verifier.rs

[krojew]

Any info if it's still planned?

[jean-airoldie]

@krojew Still planned, but I don't have to time to work on it for a while.

[krojew]

2020 - any news? I've been using CFB and it seems to work fine, so maybe it's possible to their implementation?

[aardappel]

For reference: recent discussion on Rust verifier vs using the C++ one: #5732

An older related issue: #5000

[krojew]

I have to agree with the previous discussion - if there's no way to verify the validity of a buffer, the idiomatic way to handle such situation in Rust is to return a Result. At the moment, the generated code panics on error which results in termination of current thread. In other words - without a verification like in CFB, it's very dangerous to use flatbuffers in Rust, unless someone places absolute trust in incoming data.

[krojew]

Any news?

[rw]

In other words - without a verification like in CFB, it's very dangerous to use flatbuffers in Rust, unless someone places absolute trust in incoming data.

I'd like to clarify that panicking is very inconvenient but it is safe. I believe that there is no risk of a vulnerability due to unsafe, for example.

EDIT: contributors have found problems with our usage of unsafe; I was incorrect.

[krojew]

@rw in this case, it's not simply inconvenient - it makes FB in Rust effectively unusable when dealing with untrusted message sources. Not all programs can afford terminating an entire thread with panic, when somebody sends invalid data. This effectively means a malicious sender can kill a process simply by sending garbage bytes. That's not something one would want in production environment.

The idiomatic thing to do is to use a Result instead of a full thread panic. But for it to work, we need verification. I don't see why we wouldn't want to integrate cfb into Rust FB directly, hence solving the problem.

[TheButlah]

I would like to second @krojew 's sentiment. I'm writing a high performance streaming library using flatbuffers in Rust and I won't have the luxury of deserializing in a separate thread. The ability for a malicious actor to crash the thread (and by extension the whole process, since I only have one thread) is "safe" in terms of memory safety, but still a vulnerability.

A Result return type instead of a panic should be considered essential to using flatbuffers in a production/untrusted context.

(P.S. thank you so much for the rust implementation - verifier or no, I'm really glad I have this tool to use in Rust :) )

[aardappel]

Again, the solution to not having panics is to verify a buffer beforehand, so we know access will not panic.

Besides the ergonomic issues of each field access being a Result, there is the more important issue of weaving the bound checks through all FlatBuffers accessor code, which would make it terribly slow. It is simply against everything FlatBuffers was designed for.

We don't have any implementation for any language that verifies the buffer during an access, they all verify ahead of time or rely on some form of out of bounds exception if they don't. Rust would be best served by the former, since panics apparently are not always meant to be caught?

[krojew]

@aardappel we can keep current behavior for access. The issue is with having a mechanism to verify the buffer beforehand, as you know. Currently, this is only provided by cfb, which has been having problems with generated FB code lately. We should have an official one.

[TheButlah]

@aardappel your justification makes sense to me now, thanks for the clarification. A verifier + panic on invalid access would be the most performant while still offering the protections necessary for production use

[TheButlah]

@aardappel So two things with this - it wouldn't be a slowdown, as the version right now already does bounds checks. Secondly, you would not need to constantly unwrap() - its just the ? operator, which is very very easy to use (doing whatever()?.field isn't even any different in number of characters than whatever()->field is in C++).

The main issue here is that a verifier doesn't exist (and won't for quite a while), and even if it did, people shouldn't be forced to use one just to avoid panics. There are two reasons someone might want safe, non-panicking code, that doesn't use a verifier:

1. My understanding is that the verifier is O(n), as opposed to a bound checked accessor which is O(1). So there is a performance hit when using a verifier that doesn't exist in bounds checked accessors, one that people should have agency over.
2. You often don't care if the data you are reading is really the type of flatbuffer you think it is, you just care that your program is safe and not going to crash on some input.

For this reason, there is a need for the option to use a Verifier-free api that does not panic. I can't see an argument for making people choose between a verifier and code that can panic - the most common use case is wanting code that both does not panic, and is agnostic to or even opposed to a verifier, probably for performance reasons. This is in addition to the fact that Result is more idiomatic. And if a breaking change is unacceptable, these result based accessors can be made an addition to the API - whether the old way is removed or not is really a secondary issue.

For the case where someone does want a verifier, or wants non-bounds checked code, that is where the separate concept of a VerifiedTable becomes useful. That is for exactly what you described @aardappel, where the responsibility of the verification is handled at the buffer level instead of the individual accessor level. They are two different, compatible approaches, both of which I believe ought to be supported

[rw]

If we do this, I'd like to add an integration test that uses something like https://github.com/dtolnay/no-panic to prove that accessor code won't panic.

[aardappel]

@rw I think we should just focus on a verifier. As I've tried to explain, Result is entirely not appropriate here from the pov of FlatBuffers. Remember, there is no precedent here, trying to compare FlatBuffers with other deserializers falls flat.

@TheButlah
I don't see how you could require such a huge breaking changes upon your users. You may find adding ? everywhere lightweight, but it is a very heavyweight feature, that effectively inserts if result != Ok { return result } (or whatever the syntax would be) all over the place.

And yes, it will be slower. Besides doing manual offset checks (which will likely not get as well optimized as the compiler inserted ones, and can't be elides similarly) you need to propagate these errors, stick em in a Result (which is likely twice as big as a naked return value), and have user code repeat that check. Run a benchmark to see.

Yes a verifier is more expensive than a single access, but only needs to happen once per buffer.. and skips any string data or scalars, so is rather fast for most kinds of buffers. And is entirely optional if data comes from a trusted source, or panic would be acceptable.

You often don't care if the data you are reading is really the type of flatbuffer you think it is, you just care that your program is safe and not going to crash on some input.

I find that a weird thing to say. Of course you do. You really don't want to be reading random binary as a FlatBuffer, all sorts of interesting data could come out.

there is a need for the option to use a Verifier-free api that does not panic

I do not see a strong argument for that.

This is in addition to the fact that Result is more idiomatic

No, it isn't. What is idiomatic entirely depends on what you're doing. Current Rust APIs are full of potential panics, and for good reason: they encode unexpected failures. Result encodes expected ones. FlatBuffer field access is very much unlike a JSON parsing function, and very similar to a data structure accessor (which in Rust will also panic if you pass a bad index value or whatever). Think of FlatBuffers not as a serialization system, but as a custom data structure allocator.

[TheButlah]

In my use case I have only three things that I care about:

1. Memory safe code (i.e. should be verified or bounds checked)
2. Fast as possible (which is what makes me push back on the Verifier approach since I only access a small piece of the full data)
3. Should be protected against crashing the thread (either a verifier or a Result will give this).

I might be wrong about point 2, but I really do think there are a number of common use cases where you want those three things, and a verifier is too slow. For example, when you read only a small part of the overall data, you would rather eat the performance cost of the bounds check + Result than verify the whole buffer. But maybe that is too niche of a situation and not something worth the effort. I certainly won't ask someone else to put in effort adding a feature that I myself don't have the know-how to implement, if it won't be widely useful. If its simply not a common enough use case, then that's that.

[rw]

Idea: we could let the user choose which type of overhead they want to incur.

I'm looking at the impl for SliceIndex<[T]> for usize, which contains the logic to use a machine word (usize) to fetch a value from a vector: https://doc.rust-lang.org/src/core/slice/mod.rs.html#2846-2880

The read-only methods they have there are get, get_unchecked, and index. get returns a Result, but the other two don't.

We could let users configure their flatbuffer reader in three ways:

1. Checked array accesses that panic on out-of-bounds access. Uses the function index. The return type for this would be Result<T, ()>. The error variant is () because the error is impossible, it will panic. So, the compiler will generate a small type for the result. Also, according to the impl I linked to above, the existing implementation of index uses intrinsic indexing, so it may be just as fast as usual.

2. Unchecked array accesses that cause undefined behavior on out-of-bounds access. Uses the unsafe function get_unchecked. This would also have the return type Result<T, ()>, but errors would lead to undefined behavior instead of panicking.

3. Checked array accesses that return an error on out-of-bounds access. Uses the function get. This would return a Result<T, OutOfBoundsError>, which can be caught by the caller.

This choice can be orthogonal to verification. Maybe careful users want verification AND checked array accesses. Maybe risk-embracing users want no verification combined with unsafe access.

Thoughts?

Edit: maybe we should use the Never type, instead of ()?

[TheButlah]

@rw I'm all for letting the user choose what type of overhead they want to incur, although I do think that once verified the buffer should no longer have bounds checked access since there is simply no point to continuing to do so once verified. I can't provide much feedback on the rest of your comment, as I'm new to rust.

Why do 1 and 2 return results if the error types are impossible? It seems like they should just be T, unless the goal here is for the return type to be consistent in all three cases and make it easy for users to have a standard pattern of usage via ? regardless of whether they are doing unsafe vs bounds checked vs verified.

If that is the intent, the Never (aka !) type makes more sense than a (), although it has not been stabilized for use outside of the std library. I also expect that a Result<T, !> and the following ? operation would be optimized by the compiler to just a T, since the Result would always be Ok.

[krojew]

I agree with @aardappel - a verifier is a better approach than a result. I would very much like to have a verifier returning a Result + current accessors + unsafe unchecked ones. This covers backwards compatibility, performance and verification at once.

Also, for a Result with an impossible error, use Infallible or ! (when stabilized).

[rw]

@TheButlah

1. Result

It seems like they should just be T, unless the goal here is for the return type to be consistent in all three cases and make it easy for users to have a standard pattern of usage via ? regardless of whether they are doing unsafe vs bounds checked vs verified.

Yes, just to have a unified external API.

2. Overhead of verification

Could you say more about this use case? I'm imagining the worst-case scenario, where a user wants to read one integer out of a buffer that is 2GB in size.

For example, when you read only a small part of the overall data, you would rather eat the performance cost of the bounds check + Result than verify the whole buffer.

[TheButlah]

@rw my buffers are usually a lot smaller than that, but let's take streaming video with some metadata, like timestamps, from some camera to some client as an example.

First concern is robustness. I don't want the client to crash because all of this is supposed to be robust enough to run on a remote system, but can't afford the resources/don't want the complexity of managing a thread for deserialization - so code that can panic doesn't work here. Therefore, either a verification step or bounds checking + a Result would be needed.

Second concern is performance. In this case, I'm just logging the timestamp from the sensor without reading any of the video data, and then I will pass off the flatbuffer to some callback the user provided. Does it make sense for the client to have to scan the whole flatbuffer, almost all of which is RGB data, just to get out a timestamp? The user will typically not care about verification either - if they cannot access the RGB frame due to an invalid buffer, thats fine - they will just wait for the next valid frame.

Under the current system, panics and the associated crash would occur if the user connected the client to a port that generated flatbuffers for a different sensor type. In this case, the client crashes, when I want a guarantee that the client won't crash. I don't actually mind if they connect to a sensor of the wrong type and they get back bogus data due to a misinterpretation of the flatbuffer - that's on them to verify the buffer if they feel a need to (and they generally won't, because they would rather see the bogus video frames and realize they connected to a wrong sensor and reconnect at that point, than incur the cost of verification)

This of course is all much less important if the verifier can skip a vector's data (such as if it's able to just verify the start and size of a vector but skip the vector contents), but so far I haven't heard that to be the case - hoping I'm wrong though because if so I would definitely just verify the buffer

[TheButlah]

Perhaps an additional question is in order - under what circumstances can the verifier skip the contents of vectors in the flatbuffer by checking just the start index and the number that describes the size of the vector?

I would expect a vector of ints could be checked by verifying the initial offset and total size without scanning through the actual contents.

What about a vector of tables? I'm assuming in that case, nothing could really be skipped since each table has to be checked for it's structure.

What about a vector of fixed size arrays, or a vector of structs? I'd assume this is equivalent to the vector of ints case

If the verifier can skip vectors of primitives, it pretty much solves my use case above - I can verify the vector knowing it's basically O(1) rather than O(num pixels)

[rw]

@TheButlah

1. Quick thought: even if the verifier skips primitives (I believe that the C++ implementation does skip them), if there are many vectors, that could still have a performance overhead.

2. Talking about panic and threads... I wonder what we would need to do to make Rust FlatBuffers work in no_std environments?

[TheButlah]

@rw Yes, it would still have a performance overhead that would scale with the size of the input some cases, like the one you have mentioned and in the case of vectors of non-primitives, I assume. I was just trying to find a middle ground that would justify a verifier for my use case since this proved to be a lot more controversial than expected :P

no_std support would also be very nice - I'm hoping to target some embedded applications and use flatbuffers over USB serial and not just TCP in the future

[krojew]

The verifier should skip primitives - how would you verify e.g. an int? If it's there, it's valid and that's it. A vector of ints follows the same rule - if the size is valid, the vector is valid. I don't see why a verifier would be linear in terms of buffer size in this case. Of course we cannot forget enums, which should be verified.

[aardappel]

@TheButlah
We have 7+ years of people using the C++ verifier in very high performance production use cases, and not once has the "the verifier is a bottleneck for us" come up. I'd refrain from making conclusions about a Rust verifier that doesn't exist yet, and a use case it hasn't been tested against.

Remember, you only run a verifier once, as data arrives. You are then free to access it efficiently afterwards. If you constantly are receiving brand new buffers, and then only touch a fraction of it, then I don't understand your use case, or at least I don't think we need to cater for it.

@rw
I'd vote against even optionally generating Result. It is against the semantics of FlatBuffers and a bad idea for an otherwise efficient language like Rust. Again, see my reasoning in posts above.

Also, I know using catch_unwind is probably frowned upon in Rust, but there may be cases where its sufficient for when you have to process untrusted buffer and a verifier is (not yet) an option?

[jean-airoldie]

So from what I'm gathering from this thread so far, a flatbuffer type could have two ways of construction:

• The current unchecked way where the buffer might be invalid which could lead to panics when accessing (and eventually be UB if we remove the checks for better perf)
• By first running the verifier which ensures that accesses won't panic.

Assuming that we had a working rust verifier the obvious approach would be:

impl Foo {
  pub fn new(bytes: &[u8]) -> Result<Self, Error> {
    // This is the hypothetical verifier fn.
    verify::<Self>(bytes)?;
    Ok(get_root::<Self>(bytes))
  }

  pub unsafe fn new_unchecked(bytes: &[u8]) -> Self {
    // Currently this is not actually unsafe because we do bounds checking in the accessors,
    // but this would allow us to make accessing unchecked (performance) without it being a breaking change.
    unsafe {
      get_root::<Self>(bytes)
    }
  }
}

So in that sense the Foo flatbuffer type is a contract that assumes a valid buffer, just like you assume that the underlying representation of a pure rust struct is valid on the type system level. Think of it like some funky, more complex, alternative to #[repr(C)].

[jean-airoldie]

Moreover I would argue that if you want some data in a flatbuffer that should not be validated by the verifier for performance reasons, this data should be expressed a an array of bytes. This way you can do the type checking only when you access said data.

[TheButlah]

@jean-airoldie exactly, thats what I was talking about earlier in the thread with a VerifiedTable - in order to work with a buffer, you should have the verified-ness as part of the contract of the type, and you can get one either by an unsafe function, or by running a verifier. Once you have a VerifiedTable, all future accesses are not bounds checked and do not need Result, because the table is already verified. I do want to say though, that if the panicking version stays and isn't removed from the API, it is not ok to remove its bounds checks unless it will require the user to wrap the accessor with an unsafe block - its considered a very large no no to expose potentially unsafe behavior in a safe API. Instead having it as an unsafe fn is the move there.

@aardappel OK, if the performance concerns I'm listing have not been an actual issue in reality, then thats that - I'll defer to your experience here. Especially now that I know that the verifier can skip the primitives.

Based on this it sounds like Result is not the move. So I guess the path forward is getting a verifier and then figuring out the actual API ergonomics.

[jean-airoldie]

@TheButlah

I do want to say though, that if the panicking version stays and isn't removed from the API, it is not ok to remove its bounds checks unless it will require the user to wrap the accessor with an unsafe block - its considered a very large no no to expose potentially unsafe behavior in a safe API. Instead having it as an unsafe fn is the move there.

Just so we're clear, I'm talking about bounds check that would not be required if the type is known, as opposed to bounds check related to things that are not part of the type contract (i.e. the length of a vector).

Take for instance utf8::from_str_unchecked from the std lib. It returns a str which is a slice on bytes that are assumed to be utf8. So every operation that assume a character to be utf8 is potentially UB if the type contract was violated and the byte slice was not proper utf8 in the first place. However there are still are checked operations for things that are not assumed by the type (e.g. str::get vs str::get_unchecked since the len of the slice is not part of the type system).

[TheButlah]

@jean-airoldie I'm not sure I understand - utf8::from_str_unchecked is an unsafe fn. There is no way to call it without the user wrapping its invocation in a unsafe block. Any time the user wants to make an assumption that could in theory be wrong and introduce UB, then they must be forced to use an unsafe block. Flatbuffers should never allow the user to make a mistake on their assumption and get UB in safe rust, without forcing the user to explicity use an unsafe block. Maybe we are saying the same thing?

That's why I like having a VerifiedTable type or something of that nature - you can unsafely claim that it is verified (and the user must wrap such a call with an unsafe block), or safely prove that is is verified with a verifier. Then all future access is both safe and free of bounds checks. This could live alongside the regular Table type that exists now and uses panicking accessors.

[aardappel]

Not sure which Rust thread this belong to, but https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-35864
@rw @CasperN

[krojew]

That's not good.

[rw]

@aardappel Argh. That kind of thing is one of the reasons I want to have a "safe and panic-ful" mode of access, even when Verifiers are used more wildly, since we can always make unsafe coding mistakes.

I also think that adding miri is still a good idea, although I don't know if it would have caught that issue: #6203

[CasperN]

I have a draft PR that fixes the miri problems by backing the vector with Vec<u128>, which guarantees 16byte alignment, unlike Vec<u8>. That doesn't solve the fact that read_scalar and read_scalar_at can be misused. The right thing is to that is probably marking it unsafe, or just depend on byte_order (which is what flexbuffers does)

[CasperN]

As I work on this PR, I think maybe what should be done in Rust is to base everything on Vec<u8>, since it would be annoying to force users to use Vec<u128> or some custom vector since all the std::io stuff are based on [u8] or Vec<u8>, then we should remove alignment errors and then use byte_order to read scalars. Forcing users to avoid Vec<u8> due to potential alignment errors is too unergonomic.

The way byte_order does possibly-unaligned reads is via calls to core::ptr::copy_nonoverlapping, which we could reimplement if we want to avoid the dependency. We'd be trading some speed for ergonomics due to this copy, but I think Vec<u8> is too fundamental a type to force people to change from.

Thoughts?

[TheButlah]

I agree that having people use u8 should be a priority

[TheButlah]

@CasperN Thanks for the fix! Good to know that things are once again safe. However, did that PR actually implement buffer verification? If not this issue should probably remain open

[CasperN]

Verification was implemented in #6161