Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/integrity check #109

Merged
merged 12 commits into from
Oct 1, 2023
Merged

Conversation

iamelisahi
Copy link
Contributor

Enhance the integrity check in the HandleUEContextTransferRequest function and incorporate the UE security context into the response.

@@ -215,6 +216,18 @@ func UEContextTransferRequest(
}
if transferReason == models.TransferReason_INIT_REG || transferReason == models.TransferReason_MOBI_REG {
var buf bytes.Buffer
if err = binary.Write(&buf, binary.BigEndian, ue.SecurityHeader.ProtocolDiscriminator); err != nil {

Choose a reason for hiding this comment

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

Is it possible to simply store the original regReq message (the byte array before unmarshalling) and fill it into req.BinaryDataN1Message?

_, integrityProtected, err := nas_security.Decode(ue, UeContextTransferReqData.AccessType,
ueContextTransferRequest.BinaryDataN1Message, true)
if err != nil {
ue.NASLog.Errorln(err)

Choose a reason for hiding this comment

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

Can the procedure keep going if err is nil? (Should function return here when there's an error?)

problemDetails := &models.ProblemDetails{
Status: http.StatusForbidden,
Cause: "INTEGRITY_CHECK_FAIL",
InvalidParams: []models.InvalidParam{

Choose a reason for hiding this comment

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

I think this is used to indicate there is an invalid param called "reason" for this api request, which in my opnion is not the case here.

@@ -336,11 +376,64 @@ func UEContextTransferProcedure(ueContextID string, ueContextTransferRequest mod
return ueContextTransferResponse, nil
}

func buildUEContextModel(ue *context.AmfUe) *models.UeContext {
func buildUEContextModel(ue *context.AmfUe, integrityProtected bool) *models.UeContext {

Choose a reason for hiding this comment

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

According to TS 29.518 5.2.2.2.1.1

2a. On success:

  • if the reason attribute is "INIT_REG" and integrity check is successful, the (source) AMF shall respond with
    the status code "200 OK". The payload of the response shall be an object of "UeContextTransferRspData"
    data type, containing:
    case a) the representation of the requested UE Context without PDU Session Contexts; or
    case b) the representation of the requested UE Context only containing the "supi" attribute, if the UE is
    registered in a different access type in the (source) AMF and the source AMF determines based on the
    PLMN ID of the (target) AMF that there is no possibility for relocating the N2 interface to the (target)
    AMF.
  • If the reason attribute is "MOBI_REG" and integrity check is successful, the (source) AMF shall respond
    with the status code "200 OK". The payload of the response shall be an object of
    "UeContextTransferRspData" data type, containing the representation of the complete UE Context including
    available PDU Session Contexts.

and 5.2.2.2.1.2

  1. Same as step 2a of figure 5.2.2.2.1.1-1, with following differences:
  • The (source) AMF shall skip integrity check and shall respond with the status code "200 OK "with the UE
    Context excluding SeafData and including available PDU Session Contexts

I think transfer reason is more suitable than the integrityProtected to control the items to be filled in here. If you pass the transfer reason instead of the integrityProtected in, you can even move the sessionContextList parts in case models.TransferReason_MOBI_REG_UE_VALIDATED into this api.

Comment on lines 286 to 316
_, integrityProtected, err := nas_security.Decode(ue, UeContextTransferReqData.AccessType,
ueContextTransferRequest.BinaryDataN1Message, false)
if err != nil {
ue.NASLog.Errorln(err)
}
if integrityProtected {
ueContextTransferRspData.UeContext = buildUEContextModel(ue, integrityProtected)
} else {
problemDetails := &models.ProblemDetails{
Status: http.StatusForbidden,
Cause: "INTEGRITY_CHECK_FAIL",
InvalidParams: []models.InvalidParam{
{
Param: "reason",
},
},
}
return nil, problemDetails
}

Choose a reason for hiding this comment

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

Please make this part a common api since it duplicates with that of case models.TransferReason_MOBI_REG_UE_VALIDATED.

@ss920386
Copy link

For contextTransferFromOldAmf, I think the implementation for TransferReason_MOBI_REG_UE_VALIDATED is lacking.
According to TS 23.502 4.2.2.2.2 step 4.

NOTE 4: The new AMF sets the indication that the UE is validated according to step 9a, if the new AMF has
performed successful UE authentication after previous integrity check failure in the old AMF.

and step 9a.

9a. After successful authentication in new AMF, which is triggered by the integrity check failure in old AMF at
step 5, the new AMF invokes step 4 above again and indicates that the UE is validated (i.e. through the reason
parameter as specified in clause 5.2.2.2.2).

In my opnion, currently our implementation can't follow the spec requests to perform the following proceduew

  1. MobRegReq with GUTI assigned by old AMF to new AMF
  2. Context transfer request with reason MOBI_REG to old AMF (GUTI + original RegReq)
  3. Old AMF replies 403 error with cause INTEGRITY_CHECK_FAIL
  4. new AMF initiates Id Req/Rsp + Auth Req/Rsp
  5. new AMF send context transfer with reason MOBI_REG_UE_VALIDATED to old AMF again (SUPI according to 23.502 4.2.2.2.2 step 4.)
  6. new AMF replies according to TS 29.518 5.2.2.2.1.2.

@ss920386
Copy link

For contextTransferFromOldAmf, I think the implementation for TransferReason_MOBI_REG_UE_VALIDATED is lacking. According to TS 23.502 4.2.2.2.2 step 4.

NOTE 4: The new AMF sets the indication that the UE is validated according to step 9a, if the new AMF has
performed successful UE authentication after previous integrity check failure in the old AMF.

and step 9a.

9a. After successful authentication in new AMF, which is triggered by the integrity check failure in old AMF at
step 5, the new AMF invokes step 4 above again and indicates that the UE is validated (i.e. through the reason
parameter as specified in clause 5.2.2.2.2).

In my opnion, currently our implementation can't follow the spec requests to perform the following proceduew

  1. MobRegReq with GUTI assigned by old AMF to new AMF
  2. Context transfer request with reason MOBI_REG to old AMF (GUTI + original RegReq)
  3. Old AMF replies 403 error with cause INTEGRITY_CHECK_FAIL
  4. new AMF initiates Id Req/Rsp + Auth Req/Rsp
  5. new AMF send context transfer with reason MOBI_REG_UE_VALIDATED to old AMF again (SUPI according to 23.502 4.2.2.2.2 step 4.)
  6. new AMF replies according to TS 29.518 5.2.2.2.1.2.

I think this may take some efforts to implement, so it doesn't need to be included in this PR. Please add this as a TODO.

ueContext := new(models.UeContext)
ueContext.Supi = ue.Supi
ueContext.SupiUnauthInd = ue.UnauthenticatedSupi

if integrityProtected {
var mmcontext models.MmContext
Copy link
Contributor

Choose a reason for hiding this comment

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

mmContext

@@ -561,7 +562,7 @@ func HandleRegistrationRequest(ue *context.AmfUe, anType models.AccessType, proc
// if failed, give up to retrieve the old context and start a new authentication procedure.
ue.ServingAmfChanged = false
context.GetSelf().AllocateGutiToUe(ue) // refresh 5G-GUTI
ue.SecurityContextAvailable = false // need to start authentication procedure later
// ue.SecurityContextAvailable = false // need to start authentication procedure later

Choose a reason for hiding this comment

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

remove unused code

Copy link
Collaborator

Choose a reason for hiding this comment

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

@iamelisahi missing this comment

return nil
}

func IdentityVerification(ue *context.AmfUe) bool {
return ue.Supi != "" || len(ue.Suci) != 0
// return len(ue.Suci) != 0

Choose a reason for hiding this comment

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

remove unused code

@@ -190,6 +194,17 @@ func ReleaseUEContextProcedure(ueContextID string, ueContextRelease models.UeCon

return nil
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

fix lint error

@@ -190,6 +194,17 @@ func ReleaseUEContextProcedure(ueContextID string, ueContextRelease models.UeCon

return nil
}
func HandleMobiRegUe(ue *context.AmfUe, ueContextTransferRspData *models.UeContextTransferRspData, ueContextTransferResponse *models.UeContextTransferResponse) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

fix lint error

}
ueContext.MmContextList = append(ueContext.MmContextList, mmContext)
}
if Reason == models.TransferReason_MOBI_REG_UE_VALIDATED || Reason == models.TransferReason_MOBI_REG {
Copy link
Collaborator

Choose a reason for hiding this comment

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

fix lint error

*sessionContextList = append(*sessionContextList, pduSessionContext)
return true
})

Copy link
Collaborator

Choose a reason for hiding this comment

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

fix lint error

* check integrity of the registration request included in ueContextTransferRequest
* copy security context to new amf
* handle INTEGRITY_CHECK_FAIL
Modify IdentityVerification
Modify HandleRegistrationRequest:
* assign value to ue.PlmnId in case "nasMessage.MobileIdentity5GSType5gGuti"
* since the MACs vary when the integrity check algorithms differ, the identical comparison should only be applied to nas.message.GmmMessage
* change input parameter from integrityProtected to models.TransferReason
* add TODO and delete comments
@iamelisahi iamelisahi force-pushed the feature/integrity-check branch from 7f2baae to 1a753f3 Compare September 18, 2023 11:09
@tim-ywliu tim-ywliu merged commit d00d96f into free5gc:main Oct 1, 2023
4 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.

4 participants