fix(userspace/libsinsp): prevent infinite loop in ancillary data pars…#2764
fix(userspace/libsinsp): prevent infinite loop in ancillary data pars…#2764poiana merged 1 commit intofalcosecurity:masterfrom
Conversation
…ing due to integer overflow Add validation in ppm_cmsg_nxthdr to ensure cmsg_aligned_len is at least sizeof(ppm_cmsghdr) after alignment calculation. This prevents an infinite loop when malformed ancillary data contains cmsg_len = 0xFFFFFFFFFFFFFFFF, which causes integer overflow in PPM_CMSG_ALIGN macro, resulting in cmsg_aligned_len = 0 and preventing forward progress in the loop. Signed-off-by: Francesco Emmi <francesco.emmi@sysdig.com>
3fc00b0 to
e67ecdd
Compare
Perf diff from master - unit testsHeap diff from master - unit testsHeap diff from master - scap fileBenchmarks diff from master |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2764 +/- ##
==========================================
- Coverage 76.90% 74.57% -2.34%
==========================================
Files 296 292 -4
Lines 30875 30026 -849
Branches 4693 4658 -35
==========================================
- Hits 23745 22392 -1353
- Misses 7130 7634 +504
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
deepskyblue86
left a comment
There was a problem hiding this comment.
It's a defensive check and perfectly makes sense.
I wonder when this could happen (and I doubt anyone is still trying to leverage CVE-2004-1334)...
/approve
|
LGTM label has been added. DetailsGit tree hash: f5039f0a91f83a92295d55ef1254688638cfae88 |
| } | ||
|
|
||
| size_t const cmsg_aligned_len = PPM_CMSG_ALIGN(cmsg_len); | ||
| // Guard against infinite loop: ensure we advance by at least sizeof(ppm_cmsghdr) |
There was a problem hiding this comment.
Sorry, my logic fails here. Line 3205 checks if cmsg_len is greater than sizeof(ppm_cmsghdr) and if PPM_CMSG_ALIGN just rounds up cmsg_lento the next multiple of size_t, can cmsg_aligned_len ever be less than sizeof(ppm_cmsghdr)?
There was a problem hiding this comment.
I wonder when this could happen (and I doubt anyone is still trying to leverage CVE-2004-1334)...
I now saw the reference to the old overflow CVE, but is overflow still a valid case?
There was a problem hiding this comment.
@terror96 , I added a More details section to the description. I think it should answer your questions.
What I described really happens, this is a real case I observed.
I didn’t have the chance yet to verify if this is related to any malicious activity or not.
There was a problem hiding this comment.
Thank you for the further clarification. Your observation confirms that overflow can still occur and it is indeed better to protect against it. Good job.
|
Hey @fremmi , thank you for this important fix! 😄 I'm reviewing the PR and evaluating inclusion in 0.23.0. Since this is an important fix, I'm pretty sure this will be included in this next release, but please @falcosecurity/libs-maintainers, remember we are in code freeze. /hold |
|
/hold |
|
|
||
| size_t const cmsg_aligned_len = PPM_CMSG_ALIGN(cmsg_len); | ||
| // Guard against infinite loop: ensure we advance by at least sizeof(ppm_cmsghdr) | ||
| if(cmsg_aligned_len < sizeof(ppm_cmsghdr)) { |
There was a problem hiding this comment.
Isn't checking for 0 enough?
There was a problem hiding this comment.
This is consistent with the check above, I think it's better to keep it this way.
There was a problem hiding this comment.
yes. But being this a defensive approach, and get stuck on a tight loop would happen not only if csm_align_len is 0 but if it is < than sizeof(ppm_cmsghdr) (I don't even care here if this is a possible case) I preferred doing this way
|
Your check indeed covers an uncovered case. While still wondering how is possible that, in the context of a successful |
|
I'm including this fix in the upcoming 0.23.0 release as I believe this for sure fixes the discussed issue, and we want to release libs by tomorrow. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deepskyblue86, ekoops, fremmi The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
If I'm following correctly the way glibc fixes the issue that solution also confirms that the buffer holds enough data. Let us imagine |
|
Totally agree with you @terror96 . I'll open the syncing PR after libs |
What type of PR is this?
/kind bug
Any specific area of the project related to this PR?
/area libsinsp
Does this PR require a change in the driver versions?
What this PR does / why we need it:
Add validation in
ppm_cmsg_nxthdrto ensurecmsg_aligned_lenis atleast sizeof(ppm_cmsghdr)after alignment calculation.This prevents an infinite loop when malformed ancillary data contains
cmsg_len = 0xFFFFFFFFFFFFFFFF, which causes integer overflow inPPM_CMSG_ALIGNmacro, resulting incmsg_aligned_len = 0and preventing forward progress in the loop insiderocess_recvmsg_ancillary_dataMore details:
I experienced a malfunctioning caused by an infinite tight loop when we process ancillary data from
recvmsg() / recvmmsg().Look at ppm_cmsg_nxthdr
If a control message contains a broken
cmsg_lenvalue equal to0xFFFFFFFFFFFFFFFF, we hit an integer overflow during the alignment calculation. Because of this, the aligned length becomes 0The addition
0xFFFFFFFFFFFFFFFF + 7wraps around to0x0000000000000006due to integer overflow. After masking, the result is 0.The infinite loop happens here:
sinsp_parser::process_recvmsg_ancillary_data
When
cmsg_aligned_len = 0, the pointer advancement soon before the for statement becameWhich issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: