fix(mme): Null terminates string to stop buffer overflow #11925

electronjoe · 2022-03-03T14:06:49Z

Addresses one finding (more exist) of #11826.

Zero-initialized all instances of plmn_array[PLMN_BYTES] (so that they will be null terminated) and enlarged the array by one char to accommodate the null termination.

Fixes the finding:

[ RUN      ] TestAMFStateConverter.TestUEm5gmmContextToProto
=================================================================
==15482==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffee811fc86 at pc 0x7f3038dada6d bp 0x7ffee811faa0 sp 0x7ffee811f248
READ of size 7 at 0x7ffee811fc86 thread T0
    #0 0x7f3038dada6c  (/lib/x86_64-linux-gnu/libasan.so.5+0x67a6c)
    #1 0x7f302e641e9b in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::basic_string(char const*, std::allocator<char> const&) (/lib/x86_64-linux-gnu/libstdc++.so.6+0x145e9b)
    #2 0x7f30383b85f6 in magma::lte::oai::Tai::set_mcc_mnc(char const*) bazel-out/k8-dbg/bin/lte/protos/oai/nas_state_cpp_proto_pb/lte/protos/oai/nas_state.pb.h:11239

Test Plan

Using prototype Bazel build with --config=asan validated ASAN finding
is resolved.

Signed-off-by: Scott Moeller electrojoe@gmail.com

github-actions · 2022-03-03T14:12:24Z

Thanks for opening a PR! 💯

A couple initial guidelines

All commits must be signed off. This is enforced by DCO check.
All PR titles must follow the semantic commits format. This is enforced by Semantic PR.

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 mappings for Magma maintainers for guidance on how to contact maintainers on Slack

github-actions · 2022-03-03T14:22:27Z

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

Howto

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

github-actions · 2022-03-03T14:36:00Z

feg-workflow

    2 files 202 suites 33s ⏱️
363 tests 363 ✔️ 0 💤 0 ❌
377 runs 377 ✔️ 0 💤 0 ❌

Results for commit 4d7982e.

♻️ This comment has been updated with latest results.

github-actions · 2022-03-03T14:54:13Z

agw-workflow

  54 files   84 suites 3m 51s ⏱️
738 tests 729 ✔️ 9 💤 0 ❌
923 runs 914 ✔️ 9 💤 0 ❌

Results for commit 4d7982e.

♻️ This comment has been updated with latest results.

ssanadhya

lgtm

ssanadhya · 2022-03-04T05:13:14Z

@electronjoe , since you modified the PLMN_BYTES macro, can you update the changes made in https://github.com/magma/magma/pull/11891/files too?

ssanadhya

Please update the usage of PLMN_BYTES in state_converter.cpp too

Addresses one finding (more exist) of magma#11826. Test Plan Using prototype Bazel build with `--config=asan` validated ASAN finding is resolved. Signed-off-by: Scott Moeller <electronjoe@gmail.com>

electronjoe · 2022-03-04T12:19:13Z

Please update the usage of PLMN_BYTES in state_converter.cpp too

Thanks for the corrective advice Shruti! I've migrated all instances to zero-init (so that we get the null termination). Please take a look at the updated PR!

LKreutzer

Tested locally

ssanadhya

LGTM

github-actions · 2022-03-05T14:33:40Z

Unit Test Results

  26 files   26 suites 26m 9s ⏱️
176 tests 172 ✔️ 3 💤 0 ❌ 1 🔥

For more details on these errors, see this check.

Results for commit 331551e.

♻️ This comment has been updated with latest results.

Addresses one finding (more exist) of #11826. Zero-initialized all instances of `plmn_array[PLMN_BYTES]` (so that they will be null terminated) and enlarged the array by one char to accommodate the null termination. Fixes the finding: ``` [ RUN ] TestAMFStateConverter.TestUEm5gmmContextToProto ================================================================= ==15482==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffee811fc86 at pc 0x7f3038dada6d bp 0x7ffee811faa0 sp 0x7ffee811f248 READ of size 7 at 0x7ffee811fc86 thread T0 #0 0x7f3038dada6c (/lib/x86_64-linux-gnu/libasan.so.5+0x67a6c) #1 0x7f302e641e9b in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::basic_string(char const*, std::allocator<char> const&) (/lib/x86_64-linux-gnu/libstdc++.so.6+0x145e9b) #2 0x7f30383b85f6 in magma::lte::oai::Tai::set_mcc_mnc(char const*) bazel-out/k8-dbg/bin/lte/protos/oai/nas_state_cpp_proto_pb/lte/protos/oai/nas_state.pb.h:11239 ``` ## Test Plan Using prototype Bazel build with `--config=asan` validated ASAN finding is resolved. Signed-off-by: Scott Moeller <electrojoe@gmail.com>

Addresses one finding (more exist) of magma#11826. Zero-initialized all instances of `plmn_array[PLMN_BYTES]` (so that they will be null terminated) and enlarged the array by one char to accommodate the null termination. Fixes the finding: ``` [ RUN ] TestAMFStateConverter.TestUEm5gmmContextToProto ================================================================= ==15482==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffee811fc86 at pc 0x7f3038dada6d bp 0x7ffee811faa0 sp 0x7ffee811f248 READ of size 7 at 0x7ffee811fc86 thread T0 #0 0x7f3038dada6c (/lib/x86_64-linux-gnu/libasan.so.5+0x67a6c) magma#1 0x7f302e641e9b in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::basic_string(char const*, std::allocator<char> const&) (/lib/x86_64-linux-gnu/libstdc++.so.6+0x145e9b) magma#2 0x7f30383b85f6 in magma::lte::oai::Tai::set_mcc_mnc(char const*) bazel-out/k8-dbg/bin/lte/protos/oai/nas_state_cpp_proto_pb/lte/protos/oai/nas_state.pb.h:11239 ``` ## Test Plan Using prototype Bazel build with `--config=asan` validated ASAN finding is resolved. Signed-off-by: Scott Moeller <electrojoe@gmail.com>

electronjoe requested review from a team and ssanadhya March 3, 2022 14:06

pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines. label Mar 3, 2022

electronjoe changed the title ~~fix(mme): Null terminates string to stop bufer overflow~~ fix(mme): Null terminates string to stop buffer overflow Mar 3, 2022

github-actions bot added the component: agw Access gateway-related issue label Mar 3, 2022

lionelgo approved these changes Mar 3, 2022

View reviewed changes

electronjoe force-pushed the pr-asan-amf branch from c6e203f to 80f503a Compare March 3, 2022 15:27

ssanadhya approved these changes Mar 3, 2022

View reviewed changes

ssanadhya requested changes Mar 4, 2022

View reviewed changes

fix(mme): Null terminates string to stop bufer overflow

4d7982e

Addresses one finding (more exist) of magma#11826. Test Plan Using prototype Bazel build with `--config=asan` validated ASAN finding is resolved. Signed-off-by: Scott Moeller <electronjoe@gmail.com>

electronjoe force-pushed the pr-asan-amf branch from 80f503a to 4d7982e Compare March 4, 2022 12:16

pull-request-size bot added size/S Denotes a PR that changes 10-29 lines. and removed size/XS Denotes a PR that changes 0-9 lines. labels Mar 4, 2022

electronjoe requested a review from ssanadhya March 4, 2022 12:18

LKreutzer mentioned this pull request Mar 4, 2022

Explore failing //lte/gateway/c/core/oai/test/nas:nas_converter_test with --config=asan #11827

Closed

LKreutzer approved these changes Mar 4, 2022

View reviewed changes

ssanadhya approved these changes Mar 4, 2022

View reviewed changes

electronjoe merged commit 331551e into magma:master Mar 5, 2022

ardzoht linked an issue Mar 7, 2022 that may be closed by this pull request

[mme] Crash running multiple EXTENDED_TESTS S1AP cases #11977

Closed

themarwhal linked an issue Mar 7, 2022 that may be closed by this pull request

Explore failing //lte/gateway/c/core/oai/test/nas:nas_converter_test with --config=asan #11827

Closed

themarwhal added the backport-v1.7 MME fixes to be backported for 1.7 label Mar 9, 2022

ardzoht removed the backport-v1.7 MME fixes to be backported for 1.7 label Mar 11, 2022

MAAHamid mentioned this pull request Apr 7, 2022

[Sigscale OCS integration] After succesful attach prepaid UE can't download or access internet #11157

Closed

electronjoe deleted the pr-asan-amf branch April 15, 2022 19:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(mme): Null terminates string to stop buffer overflow #11925

fix(mme): Null terminates string to stop buffer overflow #11925

electronjoe commented Mar 3, 2022 •

edited

Loading

github-actions bot commented Mar 3, 2022

github-actions bot commented Mar 3, 2022 •

edited

Loading

github-actions bot commented Mar 3, 2022 •

edited

Loading

github-actions bot commented Mar 3, 2022 •

edited

Loading

ssanadhya left a comment

ssanadhya commented Mar 4, 2022

ssanadhya left a comment

electronjoe commented Mar 4, 2022

LKreutzer left a comment

ssanadhya left a comment

github-actions bot commented Mar 5, 2022 •

edited

Loading

fix(mme): Null terminates string to stop buffer overflow #11925

fix(mme): Null terminates string to stop buffer overflow #11925

Conversation

electronjoe commented Mar 3, 2022 • edited Loading

Test Plan

github-actions bot commented Mar 3, 2022

Howto

More info

github-actions bot commented Mar 3, 2022 • edited Loading

Howto

github-actions bot commented Mar 3, 2022 • edited Loading

feg-workflow

github-actions bot commented Mar 3, 2022 • edited Loading

agw-workflow

ssanadhya left a comment

Choose a reason for hiding this comment

ssanadhya commented Mar 4, 2022

ssanadhya left a comment

Choose a reason for hiding this comment

electronjoe commented Mar 4, 2022

LKreutzer left a comment

Choose a reason for hiding this comment

ssanadhya left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 5, 2022 • edited Loading

Unit Test Results

electronjoe commented Mar 3, 2022 •

edited

Loading

github-actions bot commented Mar 3, 2022 •

edited

Loading

github-actions bot commented Mar 3, 2022 •

edited

Loading

github-actions bot commented Mar 3, 2022 •

edited

Loading

github-actions bot commented Mar 5, 2022 •

edited

Loading