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

[We are refactoring and getting rid of the custom logic, see PR 1533] fix(libsinsp): avoid possible underflow in rewind_to_parent_path util #1530

Closed

Conversation

incertum
Copy link
Contributor

@incertum incertum commented Dec 1, 2023

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind bug

/kind cleanup

/kind design

/kind documentation

/kind failing-test

/kind feature

Any specific area of the project related to this PR?

Uncomment one (or more) /area <> lines:

/area API-version

/area build

/area CI

/area driver-kmod

/area driver-bpf

/area driver-modern-bpf

/area libscap-engine-bpf

/area libscap-engine-gvisor

/area libscap-engine-kmod

/area libscap-engine-modern-bpf

/area libscap-engine-nodriver

/area libscap-engine-noop

/area libscap-engine-source-plugin

/area libscap-engine-savefile

/area libscap

/area libpman

/area libsinsp

/area tests

/area proposals

Does this PR require a change in the driver versions?

/version driver-API-version-major

/version driver-API-version-minor

/version driver-API-version-patch

/version driver-SCHEMA-version-major

/version driver-SCHEMA-version-minor

/version driver-SCHEMA-version-patch

What this PR does / why we need it:

Observed stack-buffer-overflow error (underflow) on real servers:

ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7fff8fe2730f at pc 0x55e12ec57717 bp 0x7fff8fe26ec0 sp 0x7fff8fe26eb8
READ of size 1 at 0x7fff8fe2730f thread T0
#0 0x55e12ec57716 in rewind_to_parent_path(char*, char**, char const**, unsigned int) /build/falcosecurity-libs-repo/falcosecurity-libs-prefix/src/falcosecurity-libs/userspace/libsinsp/utils.cpp:611
 #1 0x55e12ec57d6a in copy_and_sanitize_path(char*, char*, char const*, char) /build/falcosecurity-libs-repo/falcosecurity-libs-prefix/src/falcosecurity-libs/userspace/libsinsp/utils.cpp:678
#2 0x55e12ec58303 in sinsp_utils::concatenate_paths(char*, unsigned int, char const*, unsigned int, char const*, unsigned int) /build/falcosecurity-libs-repo/falcosecurity-libs-prefix/src/falcosecurity-libs/userspace/libsinsp/utils.cpp:753
#3 0x55e12ed71aa5 in sinsp_parser::parse_open_openat_creat_exit(sinsp_evt*) /build/falcosecurity-libs-repo/falcosecurity-libs-prefix/src/falcosecurity-libs/userspace/libsinsp/parsers.cpp:2771

Address 0x7fff8fe2730f is located in stack of thread T0 at offset 639 in frame
    #0 0x55e12ed70b7b in sinsp_parser::parse_open_openat_creat_exit(sinsp_evt*) /build/falcosecurity-libs-repo/falcosecurity-libs-prefix/src/falcosecurity-libs/userspace/libsinsp/parsers.cpp:2586

  This frame has 9 object(s):
    [48, 49) '<unknown>'
    [64, 80) 'name' (line 2588)
    [96, 112) 'enter_evt_name' (line 2589)
    [128, 160) 'sdir' (line 2594)
    [192, 224) '<unknown>'
    [256, 288) '<unknown>'
    [320, 352) '<unknown>'
    [384, 568) 'fdi' (line 2592)
    [640, 1664) 'fullpath' (line 2769) <== Memory access at offset 639 underflows this variable

First added few more tests to concatenate_paths, then I tried to fix the error at line 611, but I am not fully sure as so far I failed getting the right unit tests in place for rewind_to_parent_path. @LucaGuerra kindly asking for some help if possible, thank you!

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>
Change logic to ensure valid memory access as (*tc - 1) might move the pointer beyond the allocated memory.

ERROR OBSERVED

AddressSanitizer: stack-buffer-overflow
READ of size 1 at 0x7fff8fe2730f thread T0

0x55e12ec57716 in rewind_to_parent_path(char*, char**, char const**, unsigned int) /build/falcosecurity-libs-repo/falcosecurity-libs-prefix/src/falcosecurity-libs/userspace/libsinsp/utils.cpp:611

0x55e12ec57d6a in copy_and_sanitize_path(char*, char*, char const*, char) /build/falcosecurity-libs-repo/falcosecurity-libs-prefix/src/falcosecurity-libs/userspace/libsinsp/utils.cpp:678

0x55e12ec58303 in sinsp_utils::concatenate_paths(char*, unsigned int, char const*, unsigned int, char const*, unsigned int) /build/falcosecurity-libs-repo/falcosecurity-libs-prefix/src/falcosecurity-libs/userspace/libsinsp/utils.cpp:753

0x55e12ed71aa5 in sinsp_parser::parse_open_openat_creat_exit(sinsp_evt*) /build/falcosecurity-libs-repo/falcosecurity-libs-prefix/src/falcosecurity-libs/userspace/libsinsp/parsers.cpp:2771

frame:
[384, 568) 'fdi' (line 2592)
[640, 1664) 'fullpath' (line 2769) <== Memory access at offset 639 underflows this variable

Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>
@poiana
Copy link
Contributor

poiana commented Dec 1, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: incertum

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@incertum incertum changed the title fix(libsinsp): avoid possible underflow in rewind_to_parent_path util [We are refactoring and getting rid of the custom logic, see PR 1533] fix(libsinsp): avoid possible underflow in rewind_to_parent_path util Dec 2, 2023
@incertum incertum closed this Dec 2, 2023
@incertum incertum deleted the fix-rewind-parent-path branch December 8, 2023 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants