fix(proto-gtpv2c): fail closed on malformed IE 176 in the profile-less decoder - #590
Closed
VerifiedOrganic wants to merge 5 commits into
Closed
fix(proto-gtpv2c): fail closed on malformed IE 176 in the profile-less decoder#590VerifiedOrganic wants to merge 5 commits into
VerifiedOrganic wants to merge 5 commits into
Conversation
…s decoder (#585) decode_typed_ie_sequence hardcoded MalformedOptionalIePolicy::Discard, so the TS 29.274 clause 7.7.8 discard for IE 176 (Node Identifier) applied to every caller of the crate's two exported profile-less sequence decoders. Neither takes a procedure, a direction, or a message type, so neither can establish the IE's Table 7.2.x-1 presence -- and presence is the condition both clauses attach the discard to. Clause 7.7.7, quoted in crates/opc-proto-gtpv2c/CONFORMANCE.md, is explicit that the IE "shall be discarded and if the IE was received as a Mandatory IE or a verifiable Conditional IE in a Request message, an appropriate error response with Cause IE value set to "Invalid length" together with the type and instance of the offending IE shall be returned to the sender." Hardcoding one profile's presence judgement into a profile-less exported API therefore did two things at once: it silently elided a peer-controlled IE from an SDK consumer's typed view, and it destroyed the identity the clause requires an error response to carry. The entry point now selects Reject. This is a deliberate fail-closed step and not a presence-keyed rule -- keying the disposition on presence needs a procedure and direction these signatures do not carry, and is left to separate work. BREAKING: a malformed IE 176 that previously vanished from the sequence returned by decode_typed_ie_sequence or TypedIe::decode_sequence now surfaces an error. No signature moved, so nothing downstream fails to compile; this note is the only signal callers get. Callers that want the clause 7.7.8 discard must decode through S2bMessage::decode, which selects S2bDecodePurpose::Receive and has resolved the procedure and direction the rule depends on. DecodeError gains an optional offending_ie identity, set through with_offending_ie and read through offending_ie, so the failure names the IE clause 7.7.7 owes the sender instead of leaving Gtpv2cOffendingIe to be invented by the caller. It is set-if-absent, so a grouped IE's member keeps naming the innermost element that actually offended rather than its container. The field is private and DecodeError::new is the only constructor, so the addition is source-compatible; it stores a type octet and a four-bit instance only, both already in the clear on the wire, so the type's "never stores raw packet bytes" logging guarantee is unchanged. The S2b production path is untouched: s2b.rs computes malformed_optional from S2bDecodePurpose and passes it explicitly at all three of its sequence entry points, none of which routes through decode_typed_ie_sequence. Every S2b test binary passes with its count unchanged. IeDecodePolicy::discards_malformed is byte-identical, which is how the LengthOverflow carve-out is preserved: LengthOverflow reports the offset arithmetic guarding the decode rather than the received octets, and must still fail the message even where a discard is licensed. It is now pinned by a unit test, verified red by admitting LengthOverflow to that matches!. The pin has to be a unit test because LengthOverflow is raised only by checked_add_offset and is unreachable from any input a public decoder can be handed. Six tests in tests/node_identifier.rs asserted the discard through the generic decoder. They move to S2bMessage::decode, which still owns it, so the clause 7.7.8 evidence stays on the path that still applies it; the one whose subject is the profile-less path asserts the fail-closed error and the offending identity instead. Two of them needed a carrier built from the crate's own builder rather than the committed fixture, which carries a private IE that UnknownIePolicy::Reject and DecodeContext::conservative() fail on for reasons unrelated to clause 7.7.8. CONFORMANCE.md's "the discard is uniform across the decode surface ... at every validation level" bullet is rewritten: the axis that selects the discard is the decode entry point, not the validation level. The rustdoc on TypedIe::decode_sequence and TypedIe::decode_from_raw, the crate README, and the MalformedOptionalIePolicy variant docs are corrected in the same commit, the last to describe the disposition rather than a role now that callers on both sides of the wire select Reject. The clause 7.7.7 and 7.7.8 text quoted here and in CONFORMANCE.md is the in-tree quotation. No 3GPP artifacts exist on this machine, so it is NOT RE-DERIVABLE ON THIS MACHINE.
…585) Follow-up to 06ef176. Adversarial review found that the CONFORMANCE.md text that commit added was itself false in two places, that it left the pre-change contract standing verbatim in a doc comment 24 lines above the test that inverts it, and that the set-if-absent guard it introduced in DecodeError::with_offending_ie had no test in any crate. CONFORMANCE.md claimed all three profile-less decoders return an error carrying DecodeError::offending_ie and named TypedIe::decode_from_raw among them, but decode_from_raw never called with_offending_ie and returned None. A caller holding a single RawIe -- the one surface reached for when you already have the IE -- therefore had nothing to put in the clause 7.7.7 "Invalid length" Cause IE, which is exactly the identity the parent fix exists to preserve. Of the two remedies, annotating decode_from_raw is chosen over narrowing the prose. The prose describes the product this crate should ship: clause 7.7.7 owes the sender "the type and instance of the offending IE", and a conversion handed exactly one IE knows that identity for certain. Every existing test that exercises decode_from_raw asserts on code, offset, or spec_ref -- none compares a whole DecodeError -- so no existing expectation moves. The annotation is placed in the public wrapper rather than in decode_from_raw_with_evidence so the sequence loop keeps a single annotation site. Because the attachment is set-if-absent, a grouped IE handed to decode_from_raw still names the member that failed, not the container. Verified red first: node_identifier_value_decode_reports_the_malformed_length_pair, extended to sweep instances 0-15 and assert the identity, failed with "left: None, right: Some((176, 0))" before the map_err and passes after. The set-if-absent guard now has coverage in both crates, written mutation-first. Replacing the is_none() check with an unconditional assignment turns decode_error_offending_ie_keeps_the_first_identity_attached (opc-protocol) and a_grouped_ie_failure_names_the_offending_member_not_the_container (opc-proto-gtpv2c) red, both reporting Some((93, 1)) where Some((73, 2)) is required; reverting it turns them green. Before this commit opc-protocol had no test that mentioned offending_ie at all, so the guard could have been deleted there unnoticed. 06ef176 touched crates/opc-protocol/src/error.rs but its gate ran only cargo test -p opc-proto-gtpv2c; both crates are gated here. Corrected documentation, each verified against the code rather than reasoned from the previous text: * CONFORMANCE.md's error-response bullet said the offending IE's type and instance "are supplied by the decoder itself through DecodeError::offending_ie rather than being reconstructed by the caller". error_response.rs contains zero references to DecodeError; the boundary takes a caller-built Gtpv2cOffendingIe. The bullet now says so and describes reading the identity off DecodeError::offending_ie as the caller's step. * tests/node_identifier.rs said "The sequence decoders apply TS 29.274 clause 7.7.8 to the same failure and discard the IE instead; that is pinned separately below" -- the pre-change contract, which 06ef176 inverted without touching this paragraph. It now says the profile-less sequence decoders reach the same disposition, and points at the two tests that actually pin each half. * typed.rs and README.md said the clause 7.7.8 discard "is applied by S2bMessage::decode alone" / "reached only through S2bMessage::decode". S2bMessage::decode_with_diagnostics (s2b.rs:3517) and S2bMessage::from_message (s2b.rs:3523) also select S2bDecodePurpose::Receive, and decode reaches it only through from_message, so from_message is the wider surface. All three are now named wherever the profile is described. * The LengthOverflow unit test's justification claimed LengthOverflow "is raised only by checked_add_offset" and is "unreachable from any input a public decoder can be handed". Both are false: it is constructed directly at header.rs:449, message.rs:90/212, ie.rs:45/234/252/315 and several sites in typed.rs, and TypedIe::decode_from_raw(raw_176, ctx, 0, usize::MAX) returns it (observed by probe). The guard the test pins is still correct, so only the comment changed: it now argues reachability for this guard's own inputs and says explicitly that the claim does not extend to the crate. * IeDecodePolicy::scoped still explained the parameter as "the caller passes the side it is on" after Reject became a both-sides disposition; it now says the caller passes its profile. * DISCARD_MALFORMED_OPTIONAL_IE_TYPES' header read as the disposition itself; it now says eligibility, membership being one of the three conjuncts discards_malformed requires. * CHANGELOG "The S2b production path is untouched" was false as written: all three S2b entry points share decode_typed_ie_sequence_at, so S2b errors now carry an offending_ie payload they did not before. Split into the disposition (unmoved) and the payload (changed, with the observable consequence stated). * The [Unreleased] #583 entry kept two sentences asserting the old crate-wide discard while the supersession note named only one. The note now names all three claims it supersedes. 06ef176's own commit message repeats the LengthOverflow and "S2b production path is untouched" claims corrected above; it is left unamended and this message is the correction of record. The clause 7.7.7 and 7.7.8 text quoted here and in CONFORMANCE.md is the in-tree quotation. No 3GPP artifacts exist on this machine, so it is NOT RE-DERIVABLE ON THIS MACHINE.
Live probes against HEAD refute several claims this branch's docs make about
`DecodeError::offending_ie`:
- `decode_typed_ie_sequence` on Bearer Context (93, instance 1) wrapping a
truncated member header returns `offending_ie: Some((93, 1))` -- the
container, not the member. Only the value-decode arm annotates; the
framing, structural and depth arms return unannotated, so the enclosing
loop stamps the container identity.
- A top-level framing overrun returns `offending_ie: None`, as does
`DuplicateIe` under `DuplicateIePolicy::Reject`.
- `S2bDecodePurpose::Receive` is selected at two sites (s2b.rs:3517, 3523),
not by the three entry points the rustdoc enumerated, and that list is not
exhaustive over the public surfaces that reach them.
- `DecodeError` derives `Debug`, so `{:?}` output did change; only `Display`
is unchanged.
The unqualified grouped-IE sentence, the "each of the three" universal, the
entry-point enumeration and the "sees no difference" consequence are removed
rather than restated. The rustdoc on `TypedIe::decode_from_raw` already carries
the qualified form ("a member that failed its own value decode") and is left
as is.
Also scopes the CHANGELOG's remaining unscoped discard bullet to the S2b
receive profile, drops the count from the supersession note so the enumeration
no longer claims to be complete, and deletes the claim that
`TypedIe::decode_from_raw` is the surface the canonical builder's self-check
runs on -- it has no production caller; the self-check routes through
`build_s2b_profile_message` under `S2bDecodePurpose::CanonicalBuilder`.
No behaviour change.
The CHANGELOG said the error from `TypedIe::decode_from_raw` "names the IE it
was handed". A probe refutes it: handed `RawIe { ie_type: 93, instance: 1 }`
wrapping a malformed IE 176 at instance 5, the returned error carries
`offending_ie = Some((176, 5))` -- the nested member, not the container.
Handed the same malformed IE 176 directly, it carries `Some((176, 5))` too.
`a_grouped_ie_failure_names_the_offending_member_not_the_container` in
tests/s2b_typed.rs already pins the equivalent for a Bearer Context wrapping
an EBI.
This was a truncation artifact: a previous edit removed the trailing qualifier
and left the stem asserting the unqualified form. Narrowed in place rather
than deleted, because the entry has to record that 440fcb6 added the
annotation at all. The stem "already returned the value-level error and still
does" was re-checked against e5abab7, where `decode_from_raw` already passed
`MalformedOptionalIePolicy::Reject` and applied no `with_offending_ie`.
The set-if-absent behaviour is already stated under `DecodeError::offending_ie`
in the Added section, so the new clause points at it rather than restating it.
No code or test changed.
`offending_ie` grew `DecodeError` from 104 to 112 bytes: the field is three bytes but the struct was exactly packed, so it cost a whole 8-byte alignment unit. `DecodeError` is returned by every protocol crate in the workspace, and at 112 bytes it pushed `opc-proto-pfcp`'s `ProfileValidationError` past `clippy::result_large_err`, failing CI in 38 places in a crate this change never touched. Narrows `offset` to `u32` so `offending_ie` occupies the bytes it frees. The protocols this type serves bound offsets by 16- and 32-bit length fields, so the range is unreachable; `new` saturates rather than wrapping so a pathological offset cannot report a small wrong one. `offset()` still returns `usize`, and every field stays private, so no public API changes. Rejected alternatives, both of which would have ADDED breakage: boxing `ProfileValidationError::TypedDecode::source` (clippy's own suggestion) changes a public enum in a published crate, and boxing `DecodeError::spec_ref` makes `with_spec_ref` non-const. Pins the size in a test, so the next field addition fails in the crate that owns the type rather than as a lint in a crate that uses it.
Contributor
Author
|
Closing as superseded by #603, which fixes the same two defects without this branch's regressions:
Closing rather than leaving open so the wrong branch cannot be merged. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs #585 — half (b) only. The presence-keying half is not addressed here.
Problem
decode_typed_ie_sequencehardcodedMalformedOptionalIePolicy::Discard, so every caller of the exported generic decoder inherited the S2b receiver profile's discard rule for IE 176 (Node Identifier), regardless of interface, procedure or direction. A malformed IE 176 was silently elided and the decode returned success. Regression introduced this session by #582/#583.Spec basis
TS 29.274 V18.8.0 clause 7.7.7's lead paragraph is presence-neutral, re-derived verbatim from the primary artifact for this change:
Presence rows re-derived against V18.8.0: IE 176 is CO at message level in Table 7.3.1-1 (Forward Relocation Request) and Table 7.3.6-1 (Context Response). The only M rows are the grouped sub-tables 7.3.1-5 / 7.3.6-5. This contradicts the presence table in #585's own comment, which asserts M at message level. The same pattern holds in Rel-19 V19.5.0.
Fix
The profile-less path now fails closed. It deliberately does not anticipate presence-keying: where the decoder cannot know a slot's presence, it fails rather than guessing. The LengthOverflow carve-out is preserved as message-fatal, with a regression test.
BREAKING
Behavioural change on
decode_typed_ie_sequenceandTypedIe::decode_sequence. No signature moves, so nothing downstream fails to compile — a malformed IE 176 that previously vanished from the returned sequence now surfaces an error. This declaration is the only signal callers get.Evidence
cargo test -p opc-proto-gtpv2c— 289 passed, 0 failedcargo test -p opc-protocol— 19 passed, 0 failedReview record
Three adversarial rounds. Round 1 found CONFORMANCE.md claiming
TypedIe::decode_from_rawcarriesDecodeError::offending_iewhen it returns None. Round 2 found the grouped-IE claim refuted by live probe — only the value-decode arm annotates, so a container annotates first for framing errors. Round 3 retracted the unverified claims by deletion rather than rewriting them.Limitations