fix(mme): [S1 handover] fix bugs in security context for HandoverRequest Msg by talkhasib · Pull Request #7985 · magma/magma

talkhasib · 2021-07-11T20:15:40Z

Signed-off-by: Tariq Al-Khasib [email protected]

Summary

Security context next hop info were not properly computed or populated for HandoverRequest Msg. This PR resolves the following issues

1- A copy of emm_context was used to update next_hop and nhcc. These changes did not persist
2- We only copied 1 byte of the next_hop vector into the security context ie
3- We needed to bump the NHCC and next_hop before we copied into security context

The last point above needs investigation as noted with a comment on the code. We are either missing a bump to NHCC and next_hop somewhere else. Or this is the correct behaviour

Another place that needs investigation is the path switch msg for X2 handover as it populates a similar security context.

Test Plan

Tested on local setup with 2 baicells eNBs and a pixel3 phone
pcap is attached
S1HO_UE_Baicells.pcap.zip

github-actions · 2021-07-11T20:15:54Z

Thanks for opening a PR! 💯 Please note that all commits must be signed off. This is enforced by the DCO check.

Howto

Reviews. The "Reviewers" listed for this PR are the Magma maintainers who will shepherd it.
Checks. All required CI checks must pass before merge.
Merge. Once approved and passing CI checks, use the ready2merge label to indicate the maintainers can merge your PR.

More info

Please take a moment to read through the Magma project's

Contributing Conventions for norms around contributed code

If this is your first Magma PR, also consider reading

Developer Onboarding for onboarding as a new Magma developer
Development Workflow for guidance on your first pull request
CI Checks for points of contact for failing or flaky CI checks
GitHub to Slack ID mappings for Magma maintainers for guidance on how to contact maintainers on Slack

ulaskozat · 2021-07-12T04:48:59Z

lte/gateway/c/core/oai/tasks/mme_app/mme_app_bearer.c

+
+  /* TODO: investigate
+     1- Should we bump next_hop and use it in this message (as is)
+     2- or, use the previously calculated next_hop and bump up for next
+        iteration. In this case, we are missing a bump to next_hop someewhere
+        else, per testing
+  */


TL;DR We should bump up next_hop and ncc, store these new values and send them to target eNB in S1 Handover Request.

Longer explanation:

https://www.etsi.org/deliver/etsi_ts/133400_133499/133401/15.07.00_60/ts_133401v150700p.pdf section 7.2.8.4.3 indicates that (source and target MMEs in this case are the same):

"Upon reception of the HANDOVER REQUIRED message the source MME shall increase its locally kept NCC value
by one and compute a fresh NH from its stored data"

"The source MME shall store that fresh pair and send it to the target MME in the S10 FORWARD RELOCATION REQUEST message." (here S10 can be ignored as source and target MMEs are the same.

"The target MME shall store locally the {NH, NCC} pair received from the source MME.The target MME shall then send the received {NH, NCC} pair to the target eNB within the S1 HANDOVER REQUEST."

ulaskozat

I think bump up behavior is correct as noted in my comment.

…t msg Signed-off-by: Tariq Al-Khasib <[email protected]>

ulaskozat · 2021-07-12T14:56:18Z

@talkhasib Why is the context release command is going to enb with ip ending 245 but not to enab with 249 (the requester)?

ulaskozat

Context release is ending up at the wrong eNB?

talkhasib · 2021-07-12T15:05:01Z

Context release is ending up at the wrong eNB?

Good catch
There is another issue, MME needs to send the release command to the source eNB once it gets the handover notify. We currently rely on the source eNB sending a release request.

I think these issues need to be addressed in a separate PR

shaddi · 2021-07-12T15:35:58Z

Thank you for finally finding the fix for this. :-)

ulaskozat · 2021-07-12T15:57:07Z

Context release is ending up at the wrong eNB?

Good catch
There is another issue, MME needs to send the release command to the source eNB once it gets the handover notify. We currently rely on the source eNB sending a release request.

I think these issues need to be addressed in a separate PR

Okay, then let me stamp it again and you can merge it. I assume you did run the integ tests on this locally?

ulaskozat

LGTM. The remaining issues will be handled in a separate PR.

…t msg (magma#7985) Signed-off-by: Tariq Al-Khasib <[email protected]> Signed-off-by: Oriol Batalla <[email protected]>

…t msg (magma#7985) Signed-off-by: Tariq Al-Khasib <[email protected]>

…t msg (magma#7985) Signed-off-by: Tariq Al-Khasib <[email protected]> Signed-off-by: Ramon Melero <[email protected]>

…t msg (magma#7985) Signed-off-by: Tariq Al-Khasib <[email protected]>

…t msg (#7985) (#8461) Signed-off-by: Tariq Al-Khasib <[email protected]> Co-authored-by: Tariq Al-Khasib <[email protected]>

…t msg (magma#7985) Signed-off-by: Tariq Al-Khasib <[email protected]>

talkhasib requested review from a team, ardzoht, shaddi and ulaskozat July 11, 2021 20:15

pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines. label Jul 11, 2021

magmabot added the component: agw Access gateway-related issue label Jul 11, 2021

ulaskozat reviewed Jul 12, 2021

View reviewed changes

ulaskozat approved these changes Jul 12, 2021

View reviewed changes

fix(mme)(s1_handover) fix bugs in security context for HandoverReques…

8d7f10b

…t msg Signed-off-by: Tariq Al-Khasib <[email protected]>

talkhasib force-pushed the s1ho_fix branch from 097cbb9 to 8d7f10b Compare July 12, 2021 14:52

talkhasib added the ready2merge This PR is ready to be merged (is approved and passes all required checks) label Jul 12, 2021

ulaskozat requested changes Jul 12, 2021

View reviewed changes

ulaskozat approved these changes Jul 12, 2021

View reviewed changes

talkhasib added ready2merge This PR is ready to be merged (is approved and passes all required checks) and removed ready2merge This PR is ready to be merged (is approved and passes all required checks) labels Jul 12, 2021

talkhasib changed the title ~~fix(mme)(s1_handover) fix bugs in security context for HandoverRequest Msg~~ fix(mme): [S1 handover] fix bugs in security context for HandoverRequest Msg Jul 12, 2021

ulaskozat merged commit d976d3b into magma:master Jul 12, 2021

uri200 pushed a commit to uri200/magma that referenced this pull request Jul 13, 2021

fix(mme)(s1_handover) fix bugs in security context for HandoverReques…

330b22f

…t msg (magma#7985) Signed-off-by: Tariq Al-Khasib <[email protected]> Signed-off-by: Oriol Batalla <[email protected]>

talkhasib mentioned this pull request Jul 13, 2021

[lte][agw] S1 handover does not complete successfully with real handset #6429

Closed

amarpad pushed a commit to amarpad/magma that referenced this pull request Jul 13, 2021

fix(mme)(s1_handover) fix bugs in security context for HandoverReques…

fed9dea

…t msg (magma#7985) Signed-off-by: Tariq Al-Khasib <[email protected]>

talkhasib mentioned this pull request Jul 15, 2021

fix(mme): Send UE ctx release cmd to the source eNB after S1 Handover #8046

Merged

ulaskozat pushed a commit to ulaskozat/magma that referenced this pull request Aug 4, 2021

fix(mme)(s1_handover) fix bugs in security context for HandoverReques…

5ffd71b

…t msg (magma#7985) Signed-off-by: Tariq Al-Khasib <[email protected]>

ulaskozat added a commit that referenced this pull request Aug 4, 2021

fix(mme)(s1_handover) fix bugs in security context for HandoverReques…

5174045

…t msg (#7985) (#8461) Signed-off-by: Tariq Al-Khasib <[email protected]> Co-authored-by: Tariq Al-Khasib <[email protected]>

ulaskozat added the backported-v1.6 label Aug 4, 2021

ulaskozat pushed a commit to ulaskozat/magma that referenced this pull request Aug 4, 2021

fix(mme)(s1_handover) fix bugs in security context for HandoverReques…

6b4a16e

…t msg (magma#7985) Signed-off-by: Tariq Al-Khasib <[email protected]>

ulaskozat mentioned this pull request Aug 4, 2021

fix(mme): Backport s1 handover fixes by PRs 7985 and 8046 #8472

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(mme): [S1 handover] fix bugs in security context for HandoverRequest Msg#7985

fix(mme): [S1 handover] fix bugs in security context for HandoverRequest Msg#7985
ulaskozat merged 1 commit intomagma:masterfrom
talkhasib:s1ho_fix

talkhasib commented Jul 11, 2021

Uh oh!

github-actions bot commented Jul 11, 2021

Uh oh!

ulaskozat Jul 12, 2021 •

edited

Loading

Uh oh!

ulaskozat left a comment

Uh oh!

ulaskozat commented Jul 12, 2021

Uh oh!

ulaskozat left a comment

Uh oh!

talkhasib commented Jul 12, 2021 •

edited

Loading

Uh oh!

shaddi commented Jul 12, 2021

Uh oh!

ulaskozat commented Jul 12, 2021

Uh oh!

ulaskozat left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

talkhasib commented Jul 11, 2021

Summary

Test Plan

Uh oh!

github-actions bot commented Jul 11, 2021

Howto

More info

Uh oh!

ulaskozat Jul 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ulaskozat left a comment

Choose a reason for hiding this comment

Uh oh!

ulaskozat commented Jul 12, 2021

Uh oh!

ulaskozat left a comment

Choose a reason for hiding this comment

Uh oh!

talkhasib commented Jul 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shaddi commented Jul 12, 2021

Uh oh!

ulaskozat commented Jul 12, 2021

Uh oh!

ulaskozat left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ulaskozat Jul 12, 2021 •

edited

Loading

talkhasib commented Jul 12, 2021 •

edited

Loading