-
Notifications
You must be signed in to change notification settings - Fork 164
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
new: add container.host_pid
container.host_network
and container.host_ipc
fields
#2047
Conversation
4a2fdc3
to
52b9754
Compare
Perf diff from master - unit tests
Heap diff from master - unit tests
Heap diff from master - scap file
Benchmarks diff from master
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2047 +/- ##
=======================================
Coverage 73.69% 73.70%
=======================================
Files 253 253
Lines 31914 31974 +60
Branches 5627 5656 +29
=======================================
+ Hits 23519 23566 +47
- Misses 8375 8408 +33
+ Partials 20 0 -20
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
/milestone 0.19.0 |
… to container_info Signed-off-by: Lorenzo Susini <susinilorenzo1@gmail.com>
…ation from docker socket Signed-off-by: Lorenzo Susini <susinilorenzo1@gmail.com>
…ation from CRI runtimes Signed-off-by: Lorenzo Susini <susinilorenzo1@gmail.com>
Signed-off-by: Lorenzo Susini <susinilorenzo1@gmail.com>
Signed-off-by: Lorenzo Susini <susinilorenzo1@gmail.com>
52b9754
to
9383c5c
Compare
Signed-off-by: Lorenzo Susini <susinilorenzo1@gmail.com>
9383c5c
to
3188e9e
Compare
case TYPE_CONTAINER_HOST_PID: | ||
case TYPE_CONTAINER_HOST_NETWORK: | ||
case TYPE_CONTAINER_HOST_IPC: | ||
if(tinfo->m_container_id.empty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we use is_host
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! As a side note, I initially skipped using this because it uses the pid_namespace
bool is_host = tinfo->m_container_id.empty() && !tinfo->is_in_pid_namespace();
That part is not 100% true in my opinion, but I think it's there for a reason (skipped execve events where we miss container_id coming from the group?). When we miss that, the pid namespace check is not enough to say we are in host or not, since container can be started sharing the host pid namespace
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is when that check was modified last time #1604
@jasondellaluce I agree with your fix but at the same time, I'm not feeling 100% convinced since containers can be started sharing namespaces with the host.
Namespaces are shared, but cgroups should always be there, and our extraction of the container id too IMO.
Otherwise, something that will work in 99% of the cases would check also for the other shareable namespace here (will not work when all the three of them are shared, which is rare but possible)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/hold for @jasondellaluce response
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/hold for @jasondellaluce response
👍
anyway, I believe any further fix regarding is_host
should be addressed by a new PR.
Signed-off-by: Lorenzo Susini <susinilorenzo1@gmail.com>
2fa464a
to
aa264ca
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
LGTM label has been added. Git tree hash: 5699feaf2ee19bcc674eed81ed9990f42b822fc6
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/unhold
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: FedeDP, jasondellaluce, loresuso 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 |
See falcosecurity/libs#2047. Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
See falcosecurity/libs#2047. Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
What type of PR is this?
/kind feature
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:
This PR introduces the
container.host_pid
container.host_network
andcontainer.host_ipc
fields. Namespaces are the way the Linux kernel enforce isolation for containers. Sometimes, developers might want to turn off bits of this isolation by sharing pid, network or IPC namespaces with the host. This introduces several risks from a security perspective, and might be worth it monitoring and offering users the possibility to understand if a container was started with some namespaces shared with host. This PR was tested against Docker and CRI-compatible runtimes (CRI-O, in particular).Some example on how this can be (mis)used: https://bishopfox.com/blog/kubernetes-pod-privilege-escalation
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: