Address multiple cases of crashes due to malformed NGAP input#666
Merged
Conversation
Signed-off-by: Arrobo, Gabriel <gabriel.arrobo@intel.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens the AMF against malformed NGAP/NAS inputs that previously could trigger panics, and adds regression tests to ensure handlers safely ignore missing/invalid fields.
Changes:
- Add nil/empty checks across NGAP dispatcher/handlers and NAS security parsing to prevent crashes on malformed messages.
- Make
PlmnIdStringToModelsvalidate input and return an error; update GMM SUCI handling to propagate “invalid SUCI” errors. - Add targeted unit tests covering malformed inputs and panic-safety.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
util/convert.go |
Changes PLMN parsing to return (models.PlmnId, error) with validation to avoid slicing panics. |
util/convert_test.go |
Adds unit tests for PLMN parsing validation paths. |
gmm/handler.go |
Handles PLMN parse failures from SUCI decoding and returns a wrapped “invalid SUCI” error. |
gmm/handler_test.go |
Adds a new test intended to validate the new “invalid SUCI” path (currently problematic). |
ngap/handler.go |
Adds guardrails (nil checks/early returns) and safer helpers to avoid panics on malformed NGAP PDUs/IEs. |
ngap/handler_test.go |
Adds regression tests ensuring handlers/utilities don’t panic on missing IDs/cause members. |
ngap/dispatcher.go |
Adds nil checks for incoming LB messages, connections, and PDUs to prevent dispatcher crashes. |
ngap/dispatcher_test.go |
Adds regression tests for dispatcher nil-safety. |
nas/nas_security/security.go |
Prevents out-of-bounds access when deregistration identity contents are empty. |
metrics/telemetry.go |
Sanitizes Prometheus label values to ensure valid UTF-8 (hex-encodes invalid data). |
VERSION |
Bumps version to 2.2.0. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Arrobo, Gabriel <gabriel.arrobo@intel.com>
Signed-off-by: Arrobo, Gabriel <gabriel.arrobo@intel.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This was referenced Apr 23, 2026
3 tasks
gab-arrobo
pushed a commit
to donivtech/amf
that referenced
this pull request
Jun 3, 2026
Follow-up to PR omec-project#666. Three handlers still dereference mandatory IE pointers (rANUENGAPID, aMFUENGAPID) without a post-loop nil guard, so a malformed NGAP message that omits the IE entirely from ProtocolIEs.List crashes the AMF. The in-loop checks only catch "IE present but value nil" — they log and continue instead of returning, leaving the variable nil for the post-loop dereference. Handlers fixed: - HandlePDUSessionResourceNotify: guard rANUENGAPID, aMFUENGAPID - HandleHandoverFailure: guard aMFUENGAPID - HandleUplinkRanStatusTransfer: guard rANUENGAPID FetchRanUeContext dispatches PDUSessionResourceNotify and UplinkRANStatusTransfer but does not nil-check aMFUENGAPID for the former and does not parse IEs at all for the latter (empty case). HandoverFailure arrives in UnsuccessfulOutcome which the dispatcher does not pre-process. Signed-off-by: Vinod Patmanathan <vinod.patmanathan@forsway.com>
gab-arrobo
pushed a commit
that referenced
this pull request
Jun 3, 2026
* fix(ngap): add nil guards for three remaining NGAP handlers Follow-up to PR #666. Three handlers still dereference mandatory IE pointers (rANUENGAPID, aMFUENGAPID) without a post-loop nil guard, so a malformed NGAP message that omits the IE entirely from ProtocolIEs.List crashes the AMF. The in-loop checks only catch "IE present but value nil" — they log and continue instead of returning, leaving the variable nil for the post-loop dereference. Handlers fixed: - HandlePDUSessionResourceNotify: guard rANUENGAPID, aMFUENGAPID - HandleHandoverFailure: guard aMFUENGAPID - HandleUplinkRanStatusTransfer: guard rANUENGAPID FetchRanUeContext dispatches PDUSessionResourceNotify and UplinkRANStatusTransfer but does not nil-check aMFUENGAPID for the former and does not parse IEs at all for the latter (empty case). HandoverFailure arrives in UnsuccessfulOutcome which the dispatcher does not pre-process. Signed-off-by: Vinod Patmanathan <vinod.patmanathan@forsway.com> * fix(ngap): address Copilot review on PR #708 - HandlePDUSessionResourceNotify: also guard pDUSessionResourceNotifyList before iterating its .List field. Without the guard, a message that omits this mandatory IE (or includes it with a nil value) still panics on the post-loop range. - HandleHandoverFailure: synthesize a default Cause when the IE is missing. Cause is later dereferenced via *cause when calling SendHandoverPreparationFailure; falling back to the same default (causePresent / causeValue) already used for the SendUEContextRelease path keeps the function crash-safe. - Add tests for the three handlers, matching the style of the existing TestHandleHandoverNotifyIgnoresMissingIDs / TestHandleHandoverRequestAcknowledgeIgnoresMissingAmfUeNgapID. Signed-off-by: Vinod Patmanathan <vinod.patmanathan@forsway.com> * test(ngap): fix HandoverFailure outcome symbols for ngap/v2 v2.1.0 The TestHandleHandoverFailureIgnoresMissingIEs test referenced the ngap/v2 v2.0.0 names for the HandoverFailure unsuccessful outcome, which broke compilation after the dependency was bumped to v2.1.0 (via #711). In v2.1.0 the regenerated ngapType package renamed the outcome to HandoverResourceAllocation: - UnsuccessfulOutcomePresentHandoverFailure -> UnsuccessfulOutcomePresentHandoverResourceAllocation - UnsuccessfulOutcomeValue.HandoverFailure -> UnsuccessfulOutcomeValue.HandoverResourceAllocation The *HandoverFailure struct type itself is unchanged. This fixes the lint, staticcheck, and unit-tests CI failures on PR #708. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Vinod Patmanathan <vinod.patmanathan@forsway.com> --------- Signed-off-by: Vinod Patmanathan <vinod.patmanathan@forsway.com> Co-authored-by: Vinod Patmanathan <vinod.patmanathan@forsway.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
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.
No description provided.