fix(mme): Enforce stricter input validation for S1AP/NGAP/NAS payloads #15401

nathaniel-bennett · 2024-04-01T16:47:51Z

Summary

The following patch resolves several reachable assertions, overflows, null-dereference and type confusion vulnerabilities present in S1AP, NGAP and NAS protocol handlers. The fixes themselves are generally one-liners that add an additional trivial check to ensure an index isn't out of bounds, or a mandatory IE isn't missing, or a reachable assertion is turned into a soft error.

Test Plan

Tested via fuzzing to ensure robustness.

Security Considerations

The following fix mitigates several vulnerabilities accessible by either malicious base station or unauthenticated UE.

github-actions · 2024-04-01T16:48:03Z

Thanks for opening a PR! 💯

A couple initial guidelines

All commits must be signed off. This is enforced by PR DCO check.
All PR titles must follow the semantic commits format. This is enforced by PR Check Title Or Commit Message.

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 PR
CI Checks for points of contact for failing or flaky CI checks
Code Review Process for information on requesting reviews and contacting maintainers

github-actions · 2024-04-01T16:48:10Z

✔️ The Semantic PR check ended with status success. See instructions on formatting your commit and pull request titles.

github-actions · 2024-04-01T16:49:31Z

lte/gateway/c/core/oai/lib/3gpp/3gpp_24.008_mm_ies.c

@@ -564,6 +564,7 @@ int decode_emergency_number_list_ie(
      buffer + decoded, EMERGENCY_NUMBER_MAX_DIGITS, len - decoded);

  e->lengthofemergencynumberinformation = *(buffer + decoded);
+  CHECK_LENGTH_DECODER(((int) len) - decoded, e->lengthofemergencynumberinformation);


[cpplint] _{reported by reviewdog 🐶}
Using C-style cast. Use static_cast(...) instead [readability/casting] [4]

github-actions · 2024-04-01T16:49:31Z

lte/gateway/c/core/oai/lib/3gpp/3gpp_24.008_mm_ies.c

@@ -577,7 +578,9 @@
       i < EMERGENCY_NUMBER_MAX_DIGITS; i++) {
    e->number_digit[i] = 0xFF;
  }
-  Fatal("TODO emergency_number_list_t->next");
+
+  // TODO implement emergency_number_list_t->next


[cpplint] _{reported by reviewdog 🐶}
Missing username in TODO; it should look like "// TODO(my_username): Stuff." [readability/todo] [2]

github-actions · 2024-04-01T16:49:31Z

lte/gateway/c/core/oai/lib/3gpp/3gpp_24.008_mm_ies.c

@@ -590,7 +593,9 @@
  uint32_t encoded = 0;
  emergency_number_list_t* e = emergencynumberlist;

-  Fatal("TODO Implement encode_emergency_number_list_ie");
+  // TODO implement encode_emergency_number_list_ie


[cpplint] _{reported by reviewdog 🐶}
Missing username in TODO; it should look like "// TODO(my_username): Stuff." [readability/todo] [2]

github-actions · 2024-04-01T16:49:32Z

lte/gateway/c/core/oai/lib/3gpp/3gpp_24.008_sm_ies.c

@@ -73,7 +73,7 @@ int decode_access_point_name_ie(access_point_name_t* access_point_name,

  ielen = *(buffer + decoded);
  decoded++;
-  CHECK_LENGTH_DECODER(len - decoded, ielen);
+  CHECK_LENGTH_DECODER(((int) len) - decoded, ielen);


[cpplint] _{reported by reviewdog 🐶}
Using C-style cast. Use static_cast(...) instead [readability/casting] [4]

github-actions · 2024-04-01T16:49:32Z

lte/gateway/c/core/oai/lib/3gpp/3gpp_24.008_sm_ies.c

@@ -236,6 +237,9 @@
  protocolconfigurationoptions->num_protocol_or_container_id = 0;

  while (3 <= ((int32_t)len - (int32_t)decoded)) {
+    if (protocolconfigurationoptions->num_protocol_or_container_id >= PCO_UNSPEC_MAXIMUM_PROTOCOL_ID_OR_CONTAINER_ID) {
+      return TLV_UNEXPECTED_IEI; // Maximum 30 options


[cpplint] _{reported by reviewdog 🐶}
At least two spaces is best between code and comments [whitespace/comments] [2]

github-actions · 2024-04-01T16:49:32Z

lte/gateway/c/core/oai/lib/3gpp/3gpp_24.008_sm_ies.c

@@ -803,7 +815,7 @@
    }
  }

-  if (len - decoded < 0) {
+  if (((int)len) - decoded < 0) {


[cpplint] _{reported by reviewdog 🐶}
Using C-style cast. Use static_cast(...) instead [readability/casting] [4]

github-actions · 2024-04-01T16:49:32Z

lte/gateway/c/core/oai/tasks/nas/ies/EsmMessageContainer.cpp

@@ -46,7 +46,7 @@ int decode_esm_message_container(EsmMessageContainer* esmmessagecontainer,
  }

  DECODE_LENGTH_U16(buffer + decoded, ielen, decoded);
-  CHECK_LENGTH_DECODER(len - decoded, ielen);
+  CHECK_LENGTH_DECODER(((int) len) - decoded, ielen);


[cpplint] _{reported by reviewdog 🐶}
Using C-style cast. Use static_cast(...) instead [readability/casting] [4]

github-actions · 2024-04-01T16:49:32Z

lte/gateway/c/core/oai/tasks/nas/ies/PdnAddress.cpp

  if (iei > 0) {
+    CHECK_LENGTH_DECODER(((int) len) - decoded, 1);


[cpplint] _{reported by reviewdog 🐶}
Using C-style cast. Use static_cast(...) instead [readability/casting] [4]

github-actions · 2024-04-01T16:49:32Z

lte/gateway/c/core/oai/tasks/nas/ies/PdnAddress.cpp

    CHECK_IEI_DECODER(iei, *buffer);
    decoded++;
  }

+  CHECK_LENGTH_DECODER(((int) len) - decoded, 2);


[cpplint] _{reported by reviewdog 🐶}
Using C-style cast. Use static_cast(...) instead [readability/casting] [4]

github-actions · 2024-04-01T16:49:32Z

lte/gateway/c/core/oai/tasks/nas/ies/PdnAddress.cpp

  ielen = *(buffer + decoded);
  decoded++;
-  CHECK_LENGTH_DECODER(len - decoded, ielen);
+  CHECK_LENGTH_DECODER(((int) len) - decoded, ielen);


[cpplint] _{reported by reviewdog 🐶}
Using C-style cast. Use static_cast(...) instead [readability/casting] [4]

panyogesh

LGTM

github-actions · 2024-04-04T02:32:19Z

Oops! Looks like you failed the PR Check DCO. Be sure to sign all your commits.

Howto

♻️ Updated: ✅ The check is passing the PR Check DCO after the last commit.

github-actions · 2024-04-04T02:36:01Z

FeG Lint & Test

    2 files 203 suites 39s ⏱️
374 tests 374 ✔️ 0 💤 0 ❌
388 runs 388 ✔️ 0 💤 0 ❌

Results for commit 4e9c20f.

♻️ This comment has been updated with latest results.

github-actions · 2024-04-04T02:51:10Z

DP Lint & Test

14 tests 14 ✔️ 2m 13s ⏱️
  1 suites   0 💤
  1 files   0 ❌

Results for commit 4e9c20f.

♻️ This comment has been updated with latest results.

panyogesh · 2024-04-04T03:12:48Z

@nathaniel-bennett : Please fix the DCO check : PR Check DCO / DCO Check (pull_request) .
Details are there in the link https://github.com/magma/magma/actions/runs/8511282479/job/23422184541?pr=15401

rdefosse · 2024-04-04T07:57:08Z

@panyogesh @nathaniel-bennett

for me there is a build issue.

See https://jenkins-oai.eurecom.fr/view/MAGMA/job/MAGMA-MME-production/22103/artifact/magma_logs.zip

mehul-jindal · 2024-04-04T11:31:36Z

Hi @nathaniel-bennett after the recent commit I'm observing just the one TC failure, could you work it out with the help of the logs attached?

//lte/gateway/c/core/oai/test/mme_app_task:mme_app_esm_encode_decode_test FAILED in 1.6s
=================================================================================
test.log
=================================================================================
exec ${PAGER:-/usr/bin/less} "$0" || exit 1
Executing tests from //lte/gateway/c/core/oai/test/mme_app_task:mme_app_esm_encode_decode_test
-----------------------------------------------------------------------------
Running main() from gmock_main.cc
[==========] Running 22 tests from 1 test suite.
[----------] Global test environment set-up.
[----------] 22 tests from ESMEncodeDecodeTest
[ RUN      ] ESMEncodeDecodeTest.TestActivateDefaultEpsBearerContextRequest
lte/gateway/c/core/oai/test/mme_app_task/test_mme_app_esm_encode_decode.cpp:255: Failure
Expected: (decoded) >= (0), actual: -9 vs 0
lte/gateway/c/core/oai/test/mme_app_task/test_mme_app_esm_encode_decode.cpp:256: Failure
Expected equality of these values:
  encoded
    Which is: 121
  decoded
    Which is: -9
lte/gateway/c/core/oai/test/mme_app_task/test_mme_app_esm_encode_decode.cpp:258: Failure
Expected equality of these values:
  original_msg->pdnaddress.pdntypevalue
    Which is: '\x3' (3)
  decoded_msg->pdnaddress.pdntypevalue
    Which is: '\0'
lte/gateway/c/core/oai/test/mme_app_task/test_mme_app_esm_encode_decode.cpp:262: Failure
Expected equality of these values:
  std::string((const char*)original_msg->accesspointname->data)
    Which is: "magma.ipv4"
  std::string((const char*)decoded_msg->accesspointname->data)
    Which is: "magma."

nathaniel-bennett · 2024-04-10T13:09:51Z

Alright, I've figured it out--I refactored the equality symbol of an assertion the wrong way. The corrected code no longer returns -9 (TLV_OCTET_STRING_TOO_LONG_FOR_IEI) unless the ielen is less than the length_apn, not more.

        if (ielen < length_apn) {
          // Mismatch in lengths remaining between IE length and APN length
          return TLV_OCTET_STRING_TOO_LONG_FOR_IEI;
        }

mehul-jindal · 2024-04-12T06:32:50Z

Hi @nathaniel-bennett, all required CI jobs except the one my screenshot refers to seem to be passing, to fix this, please commit using the --signoff flag, for instance:

git commit --signoff -s -m "fix(mme): Enforce stricter input validation for S1AP/NGAP/NAS payloads"

Please include the changes from my commit if you intend to push 'em in one go.
TIA!

Signed-off-by: Nathaniel <me@nathanielbennett.com>

lucasgonze · 2024-04-23T14:55:38Z

The merge was blocked on a review from @pshelar, who is not responsive. I removed him from the reviewers list.

rdefosse · 2024-04-25T14:50:53Z

@nathaniel-bennett @lucasgonze

i still see some compilation issues on my side

https://jenkins-oai.eurecom.fr/job/MAGMA-MME-production/22140/artifact/build_results_magma_oai_mme.html

nathaniel-bennett requested a review from a team as a code owner April 1, 2024 16:47

nathaniel-bennett requested a review from pshelar April 1, 2024 16:47

pull-request-size bot added the size/L Denotes a Pull Request that changes 100-499 lines. label Apr 1, 2024

github-actions bot added the component: agw Access gateway-related issue label Apr 1, 2024

github-actions bot reviewed Apr 1, 2024

View reviewed changes

nathaniel-bennett force-pushed the master branch 3 times, most recently from 60f5567 to 0831cdb Compare April 1, 2024 17:26

panyogesh approved these changes Apr 4, 2024

View reviewed changes

nathaniel-bennett force-pushed the master branch from 6fde504 to d3a1086 Compare April 10, 2024 13:08

nathaniel-bennett force-pushed the master branch from d3a1086 to 3ff68e7 Compare April 10, 2024 13:10

mehul-jindal force-pushed the master branch from efd4732 to 0e9c5ce Compare April 12, 2024 05:57

fix(mme): Enforce stricter input validation for S1AP/NGAP/NAS payloads

4e9c20f

Signed-off-by: Nathaniel <me@nathanielbennett.com>

nathaniel-bennett force-pushed the master branch from 0e9c5ce to 4e9c20f Compare April 23, 2024 13:40

lucasgonze removed the request for review from pshelar April 23, 2024 14:55

panyogesh mentioned this pull request May 20, 2024

Strengthening the NAS Library against Security Vulnerabilities #15388

Open

lucasgonze requested a review from brunohcfaria May 20, 2024 18:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(mme): Enforce stricter input validation for S1AP/NGAP/NAS payloads #15401

fix(mme): Enforce stricter input validation for S1AP/NGAP/NAS payloads #15401

nathaniel-bennett commented Apr 1, 2024

github-actions bot commented Apr 1, 2024

github-actions bot commented Apr 1, 2024 •

edited

github-actions bot Apr 1, 2024

github-actions bot Apr 1, 2024

github-actions bot Apr 1, 2024

github-actions bot Apr 1, 2024

github-actions bot Apr 1, 2024

github-actions bot Apr 1, 2024

github-actions bot Apr 1, 2024

github-actions bot Apr 1, 2024

github-actions bot Apr 1, 2024

github-actions bot Apr 1, 2024

panyogesh left a comment

github-actions bot commented Apr 4, 2024 •

edited

github-actions bot commented Apr 4, 2024 •

edited

github-actions bot commented Apr 4, 2024 •

edited

panyogesh commented Apr 4, 2024

rdefosse commented Apr 4, 2024

mehul-jindal commented Apr 4, 2024

nathaniel-bennett commented Apr 10, 2024

mehul-jindal commented Apr 12, 2024

lucasgonze commented Apr 23, 2024

rdefosse commented Apr 25, 2024

		if (iei > 0) {
		CHECK_LENGTH_DECODER(((int) len) - decoded, 1);

fix(mme): Enforce stricter input validation for S1AP/NGAP/NAS payloads #15401

Are you sure you want to change the base?

fix(mme): Enforce stricter input validation for S1AP/NGAP/NAS payloads #15401

Conversation

nathaniel-bennett commented Apr 1, 2024

Summary

Test Plan

Security Considerations

github-actions bot commented Apr 1, 2024

Howto

More info

github-actions bot commented Apr 1, 2024 • edited

github-actions bot Apr 1, 2024

Choose a reason for hiding this comment

github-actions bot Apr 1, 2024

Choose a reason for hiding this comment

github-actions bot Apr 1, 2024

Choose a reason for hiding this comment

github-actions bot Apr 1, 2024

Choose a reason for hiding this comment

github-actions bot Apr 1, 2024

Choose a reason for hiding this comment

github-actions bot Apr 1, 2024

Choose a reason for hiding this comment

github-actions bot Apr 1, 2024

Choose a reason for hiding this comment

github-actions bot Apr 1, 2024

Choose a reason for hiding this comment

github-actions bot Apr 1, 2024

Choose a reason for hiding this comment

github-actions bot Apr 1, 2024

Choose a reason for hiding this comment

panyogesh left a comment

Choose a reason for hiding this comment

github-actions bot commented Apr 4, 2024 • edited

Howto

github-actions bot commented Apr 4, 2024 • edited

FeG Lint & Test

github-actions bot commented Apr 4, 2024 • edited

DP Lint & Test

panyogesh commented Apr 4, 2024

rdefosse commented Apr 4, 2024

mehul-jindal commented Apr 4, 2024

nathaniel-bennett commented Apr 10, 2024

mehul-jindal commented Apr 12, 2024

lucasgonze commented Apr 23, 2024

rdefosse commented Apr 25, 2024

github-actions bot commented Apr 1, 2024 •

edited

github-actions bot commented Apr 4, 2024 •

edited

github-actions bot commented Apr 4, 2024 •

edited

github-actions bot commented Apr 4, 2024 •

edited