cloudflare / cloudflare/wirefilter

CPrimitiveType (a repr(u8) enum) is received directly by value across the C ABI with no validation

Open
#196 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Rust
Stars
1.2k
Forks
122
Avg merge
5h 20m
Merged PRs (30d)
4

Description

`wirefilter_create_primitive_type(ty: CPrimitiveType) -> CType` (`ffi/src/lib.rs:249`) takes the
`#[repr(u8)]` enum `CPrimitiveType` (valid discriminants `1..=4`, `ffi/src/lib.rs:38-45`) directly by
value across the `extern "C"` boundary. The generated header exposes the C parameter as a bare
`typedef uint8_t wirefilter_primitive_type;` in plain-C mode — nothing on the C side restricts it to
`1..=4`. Constructing an out-of-range enum value this way violates Rust's own enum validity invariant
— Undefined Behavior per the language specification, independent of what any particular compiler
happens to do with it today.

A related, separately-reachable gap: `impl From for Type` (`ffi/src/lib.rs:94`) does
`CPrimitiveType::try_from(cty.primitive).unwrap()` — a *checked* conversion, but on a plain
`pub primitive: u8` struct field with no C-side range enforcement. A zero-initialized
`wirefilter_type_t ty = {0};` (the natural C idiom) has `primitive == 0` and hits this `.unwrap()`.
Reachable via 4 entry points that never go through `wirefilter_create_primitive_type` at all.

**What was actually observed, dynamically, from real C callers** (compiled C test via this repo's own
`ffi/tests/ctests` harness, both debug and release):

- Construction does not crash on this target, in either profile. A real C call
`wirefilter_create_primitive_type((wirefilter_primitive_type)5)` returns normally with
`primitive == 5` intact.
- Using the resulting value subsequently hits the checked panic above: passing it into
`wirefilter_add_type_field_to_scheme` reaches `CPrimitiveType::try_from(5)` → `Err` → `.unwrap()`
panics, `SIGABRT`, in both debug and release, from the real C caller:

```
calling wirefilter_create_primitive_type((wirefilter_primitive_type)5) from real C ...
Survived call (unexpected), primitive byte = 5
calling wirefilter_add_type_field_to_scheme with the invalid-discriminant type ...

thread '...' panicked at ffi/src/lib.rs:94:67:
called `Result::unwrap()` on an `Err` value: TryFromPrimitiveError { number: 5 }
...
thread caused non-unwinding panic. aborting.
(signal: 6, SIGABRT: process abort signal)
```

This does **not** mean the raw-reception gap is "safe" in general — it means that on this specific
compiler and code path, the invalid value isn't consumed by anything that miscompiles around its
invalidity before reaching a checked conversion. Undefined Behavior isn't something a finite number of
test runs can rule out for every compiler version or future refactor that adds a `match` on
`CPrimitiveType` without going through `TryFromPrimitive`.

**Reachability:** any C caller or dynamic-binding layer invoking `wirefilter_create_primitive_type`
with an out-of-range byte, or constructing a raw `wirefilter_type_t` and passing it to any of the 4
consuming functions. Network/attacker reachability through any specific embedding is not established
here.

**Suggested fix:** change `wirefilter_create_primitive_type`'s parameter to a raw `u8` and validate it
with `TryFromPrimitive`, exactly as the `From for Type` path already does — this closes the
raw-ABI-reception gap and makes both paths consistent, removing the language-level UB regardless of
whether any currently-observed compiler exploits it. The remaining gap on the already-checked path is
just the missing `catch_panic` wrapper on its four call sites (same four as in the CompoundType depth
report).

Found with the [rust-in-peace](https://github.com/scadastrangelove/rust-in-peace) pipeline.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.