Skip to content

Comments

fix: ensure first NGAP message is NGSetup to prevent crash#192

Merged
Alonza0314 merged 6 commits intofree5gc:mainfrom
wiwi878:fix/ngap-initial-message-validation
Feb 23, 2026
Merged

fix: ensure first NGAP message is NGSetup to prevent crash#192
Alonza0314 merged 6 commits intofree5gc:mainfrom
wiwi878:fix/ngap-initial-message-validation

Conversation

@wiwi878
Copy link
Contributor

@wiwi878 wiwi878 commented Jan 7, 2026

This PR is for issue#768

Comment on lines 50 to 57
if ran.RanId == nil {
if pdu.Present != ngapType.NGAPPDUPresentInitiatingMessage ||
pdu.InitiatingMessage.ProcedureCode.Value != ngapType.ProcedureCodeNGSetup {
ran.Log.Warn("Received non-NGSetup message on uninitialized connection")
return
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @wiwi878, This check can be removed. If we properly refactor the logic above, when AmfRanFindByConn() successfully finds a ran, it must have already completed NGSetup, so RanId should never be nil.

Comment on lines 33 to 43
@@ -36,16 +42,19 @@ func Dispatch(conn net.Conn, msg []byte) {
return
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current Problem: At line 25, we execute ran = amfSelf.NewAmfRan(conn) to create a new AmfRan, but we don't check if len(msg) == 0 until line 33.

This causes an issue: when a new connection is established and immediately closed (msg is empty), the program will first create an AmfRan at L25, then delete it at L33-36. This is unnecessary resource overhead and could potentially be exploited for DoS attacks.

Suggested refactoring (after L15 amfSelf := context.GetSelf()):

func Dispatch(conn net.Conn, msg []byte) {
    amfSelf := context.GetSelf()

    // Step 1: Handle connection close (len(msg) == 0) first
    if len(msg) == 0 {
        ran, ok := amfSelf.AmfRanFindByConn(conn)
        if !ok {
            // Connection closed before NGSetup, no RAN context exists
            logger.NgapLog.Warnf("Connection closed before NGSetup: %s", conn.RemoteAddr())
            return
        }
        // Registered RAN closing connection
        ran.Log.Infof("RAN close the connection.")
        ran.Remove()
        return
    }

    // Step 2: Normal message processing - find or create AmfRan
    ran, ok := amfSelf.AmfRanFindByConn(conn)
    if !ok {
        addr := conn.RemoteAddr()
        if addr == nil {
            logger.NgapLog.Warn("Addr of new NG connection is nil")
            return
        }
        logger.NgapLog.Infof("Create a new NG connection for: %s", addr.String())
        ran = amfSelf.NewAmfRan(conn)
    }

    // Step 3: Decode and dispatch...
    pdu, err := ngap.Decoder(msg)
    // ...
    dispatchMain(ran, pdu)
}

@wiwi878 wiwi878 force-pushed the fix/ngap-initial-message-validation branch from b7ca6a9 to e99c1a1 Compare February 11, 2026 06:14
@wiwi878 wiwi878 force-pushed the fix/ngap-initial-message-validation branch from e99c1a1 to 00d6d44 Compare February 11, 2026 06:26
@c9274326
Copy link

@Alonza0314 LGTM!

@Alonza0314 Alonza0314 merged commit 92b7a3f into free5gc:main Feb 23, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants