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

fix(amf): Correct MSIN decoding for SUCI Profile B #15499

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kmu-tm
Copy link

@kmu-tm kmu-tm commented Aug 23, 2024

Summary

This simple PR fixes the issue with SUCI Profile B, as reported in #15444. The problem was the incorrect decoding of the MSIN. For example, the decoded IMSI was 99970008000, but the actual IMSI was 999700000068375 (as shown in the logs)

To sum up, the AMF failed to correctly parse the MSIN from the database which led to the IMSI being incorrect and the authentication procedure failing. The MSIN is received as 5 bytes of raw binary data in BCD (Binary Coded Decimal) format, with each byte representing two digits in reverse order. The original code assumed that the MSIN was a string of 10 digits and attempted to convert this in a wrong format (this resulted in the values being decoded incorrectly).

This PR changes the following:

  • Modifies the amf_decrypt_msin_info_answer function to properly handle BCD-encoded data.
  • Adds additional debug logs (some of the debug/error logs was unnecessary?).
  • Removed the old incorrect ASCII-based decoding approach.
  • Also, the unit test was also wrong. Thanks to Bruno for pointing that out in his fix. I have included his modifications of the test to this PR aswell.

There will be a new PR submitted for the SUCI documentation (there are some confusion regarding key generation).

Test Plan

The UEs that were tested w/ Profile B were IPhone and IPad using test keys from 3GPP TS.

image

Additional Information

EDIT: I am just going through the updates from v1.8. I do find it a bit odd that this issue was mentioned in the release notes here. Could there have been a rollback?

Security Considerations

@kmu-tm kmu-tm requested a review from a team as a code owner August 23, 2024 10:54
@kmu-tm kmu-tm requested a review from maxhbr August 23, 2024 10:54
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines. label Aug 23, 2024
@github-actions github-actions bot added the component: agw Access gateway-related issue label Aug 23, 2024
Copy link
Contributor

Thanks for opening a PR! 💯

A couple initial guidelines

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

If this is your first Magma PR, also consider reading

@brunohcfaria
Copy link
Collaborator

The fix works for MSIN decoding with ProfileA as well. Tested with UERANSIM version 2134f6b

@brunohcfaria
Copy link
Collaborator

@jordanvrtanoski Please check this PR. From my side, it's ready to go.

- Update M5GSUCIRegistrationServiceClient.cpp for proper SUCI handling
- Modify nas_proc.cpp to fix MSIN decoding process
- Adjust test_amf_procedures.cpp for debugging

Signed-off-by: kmu-tm <kamar.mubasier@telenormaritime.com>
Copy link
Contributor

@lucasgonze lucasgonze left a comment

Choose a reason for hiding this comment

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

From a security POV the positive is better logging and less fragile memory access and the negative is more complexity / surface area.

Overall this is a clear win.

@lucasgonze lucasgonze requested review from brunohcfaria and removed request for maxhbr October 30, 2024 15:32
Copy link
Contributor

DP Lint & Test

0 tests   0 ✔️  0s ⏱️
0 suites  0 💤
0 files    0

Results for commit a65903b.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: agw Access gateway-related issue size/M Denotes a PR that changes 30-99 lines.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants