feat(proto-gtpv2c): type Node Identifier IE on receive - #583
Merged
Conversation
TS 29.274 V18.8.0 Table 8.1-1 assigns type 176 to Node Identifier, "Extendable / 8.107", and Figure 8.107-1 encodes it as a one-octet Length of Node Name and Node Name followed by a one-octet Length of Node Realm and Node Realm. The crate modelled no such IE, so a peer-supplied Node Identifier decoded as TypedIeValue::Raw and its declared subfield lengths were never inspected: a Node Name length of 0xff inside a three-octet value was indistinguishable from a well-formed one. NodeIdentifier keeps both subfields private behind a validated constructor, which bounds each to the 255 octets its length field can express and makes encoding infallible without a truncating cast. Decode rejects a subfield length that overruns the IE value, or an absent length octet, as Truncated, reporting the absolute offset of the offending subfield Length octet under one rule for both failure paths. Trailing octets are ignored rather than rejected because Table 8.1-1 marks 176 Extendable and clause 8.1 requires a legacy receiver to ignore the Figure 8.107-1 (q+1) to (n+4) octets. Either subfield may be empty: clause 8.107 requires a non-zero length only for its SGSN Identifier and MME Identifier cases, and the encoding carries no discriminator distinguishing them from the 3GPP AAA Server Identifier case this profile receives. Receive admission follows Table 7.2.1-1, which lists the IE once for this profile: 3GPP AAA Server Identifier, presence O, Create Session Request, top level, instance 0. Exactly one receive rule was added; the remaining Node Identifier rows are clause 7.3 S3/S10/S16 mobility messages this profile does not model. Admitting 176 into the receive grammar also narrows the sender profile, because the three request builders gate caller-supplied additional_ies on the same disposition, so a hand-rolled raw IE 176 is now accepted only in that slot. The response builders gate nothing; that looseness is pre-existing and is documented rather than changed here. Rejecting a malformed optional IE instead of discarding it diverges from clauses 7.7.7 and 7.7.8, which both direct discard-and-continue. The divergence is deliberate and keeps the crate-wide typed-IE contract, but its radius is wider than the Table 7.2.1-1 slot: outside ProcedureAware there is no instance filter, so a malformed IE 176 fails the whole S2bMessage decode at any instance, in any modelled message type, and nested inside a Bearer Context. CHANGELOG and CONFORMANCE now state that radius, quote both clauses, and record that this is not what the crate's own pco.rs selection rule would pick. The encoder differential over 7520 points is now asserted rather than merely enumerated: the harness digests the ordered (label, outcome) list per block against committed literals and pins its axis sizes as literals instead of recomputing them from the enumerator's own inputs. Re-running it against the parent commit shows 608 changed points, all on IE 176 injection rows, zero on any committed fixture, and zero raw-preserving byte changes.
VerifiedOrganic
changed the title
Contributor
Author
|
Holding this PR. The clause 7.7.8 divergence documented in the description is being corrected rather than disclosed. TS 29.274 clause 7.7.8 says shall: a receiver of an optional IE with a bad value "shall discard this IE, but shall treat the rest of the message as if this IE was absent and continue processing." Node Identifier is presence O on Table 7.2.1-1. Failing the whole message decode is a conformance violation, and "crate consistency" is not a reason to override a normative requirement. The uniformity argument also does not hold on inspection: the other typed IEs cited as precedent (Cause, F-TEID, PAA) are Mandatory or Conditional, and the spec buckets receiver behaviour by presence - mandatory/conditional fail, optional discard-and-continue. That is the rule, not an exception for 176. The crate's own written selection rule, quoted from |
TS 29.274 V18.8.0 clause 7.7.8 states, of an optional IE: "The receiver of a GTP signalling message including an optional information element with a Value that is not in the range defined for this information element value shall discard this IE, but shall treat the rest of the message as if this IE was absent and continue processing", and "All semantically incorrect optional information elements in a GTP signalling message shall be treated as not present in the message." Clause 7.7.7 reaches the same disposition for a length inconsistency in an Extendable IE, owing the "Invalid length" response only "if the IE was received as a Mandatory IE or a verifiable Conditional IE in a Request message". Table 7.2.1-1 gives Node Identifier presence O, so the receiver rule is discard-and-continue. The parent commit failed the whole message decode instead, which is a normative violation and a peer-controlled DoS surface: two octets in an optional IE cost an otherwise-valid Create Session Request. The justification given was crate consistency with Cause, F-TEID, PAA, and TWAN Identifier. Those are Mandatory or Conditional on the messages where this profile receives them, and TS 29.274 buckets receiver behaviour by presence: mandatory or verifiable conditional means reject and name a Cause, optional means discard and continue. Handling the two differently is the specification's own rule, so the mandatory and conditional IEs keep failing closed and only the optional bucket changes. The parent commit had already quoted the crate's own selection rule from pco.rs and noted that by that rule IE 176 belongs in the skip bucket; it now is in it. Discard reuses the clause 7.7.9 instance-filter precedent exactly: the IE is absent from the typed view and nothing is recorded, because S2bReceiveDiagnostics carries duplicate-IE evidence only and clause 7.7.8 requires no log for the optional case. Raw-preserving encoding still reproduces the malformed octets byte-exact, which is observable at the S2b layer at all now that the decode succeeds. The disposition holds at Structural, Strict, and ProcedureAware, at every instance 0-15, in every modelled message type, and nested inside a Bearer Context; Strict is not an opt-in stricter-than-spec mode, and for an optional IE clause 7.7.8 is the range rule Strict enforces. Both clauses bind "the receiver of a GTP signalling message", so the canonical builder's sender-side self-check keeps rejecting: a caller-supplied raw IE 176 with a malformed value is a build failure, since those octets are already in the message and dropping the IE from the typed view would emit them anyway. TypedIe::decode_from_raw, which by contract cannot represent a deliberate omission, likewise keeps returning the error. The tests that pinned the old behaviour are rewritten to assert the new one rather than deleted, and now assert the second half of the requirement too: the injected IE is spliced ahead of every fixture IE and the resulting typed projection must equal the pristine one. Four tests are added for the raw-preserving surface, the UnknownIePolicy axis, the builder boundary, and a malformed mandatory F-TEID that must still fail. Eight mutations were run against the suite and all eight were killed, including one that widens the discard to a mandatory IE and one that lets it reach the builder. Over the committed 7520-point encoder differential, 330 points change from decode_err:Truncated to a successful decode, all on IE 176 rows: 320 at Structural and 10 at ProcedureAware, where instances 1-15 were already clause 7.7.9 discards and a non-zero spare nibble fails earlier. Half of those are raw-preserving points that return byte-for-byte to the pre-IE-176 outcome, so across the whole change set raw-preserving encoding is now byte-identical to the parent of the parent commit at every one of its points, and the 608-point canonical delta drops to 443: 165 clause 7.7.9 discards, 165 clause 7.7.8 discards, 96 spare-nibble zeroings, 17 Extendable-suffix strippings. Block A, the committed fixture corpus, stays at zero changed points. CHANGELOG, CONFORMANCE, and README no longer describe a deliberate divergence, because there no longer is one.
VerifiedOrganic
changed the title
The clause 7.7.8 discard added in 3cafb65 ran one statement too late. `seen.push((key, offset))` happened before the value decode, so a discarded Node Identifier kept its `(ie_type, instance)` slot and re-routed every later IE at that key into the clause 7.7.10 duplicate machinery. "Treat the rest of the message as if this IE was absent" is a claim about the whole remaining decode, not only about the returned sequence. For the wire `[IE 176 inst 0 malformed][IE 176 inst 0 well-formed]` that meant, before this commit: DuplicateIePolicy::Reject -> Err(DuplicateIe) [spec: Ok, one IE] DuplicateIePolicy::First -> Ok([]) [spec: Ok, one IE] DuplicateIePolicy::Last -> Ok([(176, 0)]) [correct] `Reject` is what `DecodeContext::conservative()` selects for untrusted network input, so the peer-controlled denial of service the parent commit exists to remove was still reachable -- with two malformed IEs and no valid one it cost four octets instead of two. Under the S2b receive profile, which forces `First`, the failure was open rather than closed: the genuine 3GPP AAA Server Identifier was silently dropped and `S2bReceiveDiagnostics` reported a `DuplicateIeEvidence` the wire did not contain, so the only diagnostic named the wrong cause. Record the key only after the value decode succeeds. A discard now leaves the key free; a retained occurrence still makes a later one at that key a genuine clause 7.7.10 repeat, which the caller's policy governs. Also: - Thread `MalformedOptionalIePolicy` through `IeDecodePolicy::scoped` instead of hardcoding `Discard` on an invariant only a comment held. - Narrow the discard to the error codes the clause 8.107 value decoder raises, so the `checked_add_offset` overflow guard is no longer swallowed with them. - Extend the encoder differential's Block B to all three validation levels. It stopped at two, leaving the 96 `Strict` canonical IE 176 points outside the digest: the whole-change canonical delta is 539, of which only 443 were guarded. `Strict` is the level `conservative()` selects. Raw-preserving encoding is byte-identical to `dae1919a` at all 3079 checked points across all three levels. - Correct the CHANGELOG, CONFORMANCE and README sentences that claimed the discard behaved "exactly as for a clause 7.7.9 instance discard" and introduced "no new diagnostic surface", restore "and critical IE rules" to the quoted `ValidationLevel::Strict` definition, and rescope the sender-side reject claim from the request builders to every S2b builder, which is where `build_s2b_profile_message` actually enforces it. Tests: 280 -> 286.
This was referenced Jul 27, 2026
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.
Problem
opc-proto-gtpv2cmodelled no typed IE for Node Identifier (176). Emission was never blocked — the raw-preserving path handles it — but on receive a peer message carrying 176 decoded as preserved-unknown, so the crate could not structurally validate the §8.107 shape and a consumer wanting to read a peer-supplied value had to hand-parse the same peer-controlled bytes the crate already walks.What landed
IE_TYPE_NODE_IDENTIFIER = 176, aNodeIdentifiertyped value with private fields behind a validated constructor andname()/realm()accessors, a redactingDebug, infallible canonical encode, and aTypedIeValue::NodeIdentifiervariant wired through all four exhaustive matches. 176 added toKNOWN_RECEIVE_IE_TYPESand exactly oneReceiveIeRule— Create Session Request, top level, instance 0,max_occurrences: ONE.Receiver behaviour follows the spec, by presence
A malformed Node Identifier is discarded and the rest of the message processed as if it were absent. TS 29.274 clause 7.7.8, verbatim:
Node Identifier is presence O on Table 7.2.1-1. Clause 7.7.7 conditions the error response on presence in the same way — mandatory or verifiable-conditional gets "Invalid length", optional does not.
The first revision of this PR failed the whole message instead, justified as consistency with the crate's other typed IEs. That was wrong: the IEs cited as precedent (Cause, F-TEID, PAA) are Mandatory or Conditional, so the spec's own presence split is the rule and not an exception for 176. Corrected in
3cafb65f.Implemented by reusing the same
continuethe clause 7.7.9 instance filter already uses indecode_typed_ie_sequence_at— not a new mechanism. Discard applies at all three validation levels; neitherStructuralnorStrictis documented as an opt-in stricter-than-TS-29.274 mode, so neither carves out.Sender side still rejects. Both clauses open "The receiver of a GTP signalling message", so
S2bDecodePurpose::CanonicalBuilder— the builders' self-check — keeps failing a caller-supplied malformed raw IE 176. The octets are already in the message being built; discarding from the typed view would emit them regardless.TypedIe::decode_from_rawstill returnsTruncated; its doc already states it cannot represent deliberate omission.Two places the issue's premise was wrong
"The value ends at the Node Realm" — no. Table 8.1-1 marks 176 Extendable and Figure 8.107-1 reserves
(q+1)..(n+4): "These octet(s) is/are present only if explicitly specified". An exact-length check would reject conforming future-release traffic. Decode stops at Node Realm and ignores the suffix; canonical encode emits only the understood Release 18 prefix; raw-preserving keeps the suffix byte-exact."Neither length shall be zero" — not for this case. That sentence appears only under the SGSN Identifier and MME Identifier bullets, not under 3GPP AAA Server Identifier. Empty subfields are accepted.
Wire compatibility
11040-point differential — the harness was written against pre-change APIs first, baselined, then re-run. Block B originally excluded
Strict; that gap is closed, so all three validation levels are now guarded.Against
dae1919a, 539 changed points, all canonical mode, all IE 176:Block A: 0 changed points. Raw-preserving mode: 0 changed points across the entire PR. Well-formed IE 176 at instance 0 is byte-identical in every mode at every level.
The grid widening was verified additive rather than assumed: 0 pre-existing labels missing, 0 pre-existing labels with a changed outcome, 3520 new rows all
strict, and the 2-level subset hashes identically before and after. So no regression can hide behind the digest change.Review
Four adversarial passes across three commits.
The first found the flagship 7520-point differential asserted none of its outcomes — it checked only a point count derived from its own inputs, plus label uniqueness. It now pins every axis size as a literal and digests the ordered
(label, outcome)list per block. That digest has since demonstrably earned its place: a one-bitheader.rschange was caught by the digest alone, changing no labels.The last found the discard was implemented one statement too late in the loop.
seen.pushran before the discardcontinue, so a discarded IE still permanently claimed its(ie_type, instance)slot. Consequences, all reproduced: under the crate's own ProcedureAware receive profile a later well-formed Node Identifier was silently dropped and a fabricatedDuplicateIeEvidenceemitted; underDecodeContext::conservative()— documented as "suitable for untrusted network input" — the whole message failed withDuplicateIe. The peer-controlled DoS this PR exists to remove was still reachable, at four octets instead of two. Fixed in0a723390; restoring the bug kills the three new tests and none of the pre-existing ones, confirming nothing caught it before.Mutation testing throughout. The survivors are equivalent mutants and are identified as such rather than counted as passes — one arm is unreachable by construction, and one parameter can only take a single value at its sole live call site today.
Three defects were found in the tests themselves and fixed rather than reported as clean: a boundary test derived its
255from the very constant under test; one mutation's anchor missed so it never ran; and both message-level tests appended the malformed IE last, where acontinue→breaktruncation is invisible.Every quoted spec string was grep-confirmed verbatim against TS 29.274 V18.8.0.
One finding that changes the issue's framing
Table 7.2.1-1's condition reads "The ePDG/TWAN may include this IE on the S2a/S2b interface ... to the PGW". On S2b an ePDG-role stack is the sender, never the receiver — so this receive hardening benefits the crate's PGW-side decode surface, not an ePDG's exposure to peer-controlled bytes. The work stands (the crate models both roles and validating receive is correct), but the
epdgframing on the issue does not hold for this direction.Still unverified, as for the filer: whether any deployed PGW actually sends 176.
Follow-up filed, not folded in
#585 — four presence-O slots elsewhere in the crate still fail the whole message on a malformed value, against the same clauses. Not fixable by extending this PR's approach: it keys the discard on IE type, which is only sound because 176 is optional at every slot the profile admits. Those four vary by slot, so a correct decision must key on
(procedure, direction, scope, ie_type, instance)— answerable only atProcedureAware, which makes it a crate-wide contract change.Gates
cargo fmt --all -- --checkclean · clippy clean under--all-featuresand default ·cargo test -p opc-proto-gtpv2c --all-features252 → 286 ·cargo build --workspace --all-targetsclean.Closes #582