adding an event on modify container entrypoint #191
Conversation
| // Write "written by event-generator" to /proc/self/fd/1 | ||
| data := []byte("written by event-generator") | ||
| _, err = file.Write(data) |
There was a problem hiding this comment.
Does anything continue to work after this?
The action must be self-contained and should leave the system in a functional state (e.g. if it writes a file, then it removes it; if it modifies the system, then it undoes the change, etc...)
There was a problem hiding this comment.
That's a valid point, @leogr. However, as I proposed the container spawning capability will include the ability to delete containers. So, if we are altering the container filesystem and then deleting the container, does it matter to delete that change in the file? Also, it's a comment. WDYT about it please provide feedback.
There was a problem hiding this comment.
So, if we are altering the container filesystem and then deleting the container, does it matter to delete that change in the file?
It does not matter in the specific case you mentioned, but since the user can also run the action in any container they want (think when we deploy the event generator in a K8s cluster), we should not make such an assumption when implementing an action. Even the h.InContainer() guard isn't enough since we don't know if it's running in an ephemeral container or not.
(consideration: I know this is a limitation, but it's also a way to prevent unsafe usage of this tool, which is already dangerous, potentially 😇 )
There was a problem hiding this comment.
Yes that sound good to me. Thank you @leogr, I will make changes.
There was a problem hiding this comment.
@leogr please take a look at it again. One possibility is to use os.truncate . WDYT?
`os.truncate`: truncate any file
`file.truncate`: truncate file objects which have been created or opened for writing, as it's a method of the file object.
There was a problem hiding this comment.
What's the advantage of using truncate? We will still modify /proc/self/fd/1 and leave the container tampered with, right?
Just an alternative idea: we may append some data to /proc/self/fd/1 and the revert it. Not sure if it's a good idea, but it's the only that I can find right now.
There was a problem hiding this comment.
@leogr I made changes, and took this approach:
- Open
/proc/self/fd/1in WRONLY mode - Get the current size of file
- Write data(at last) into it
- Truncate the file to its initial size to remove the written content
This event triggering rule successfully too
To check its only removing written content by event, I tried this on /tmp directory by creating /tmp/test.go inside container and main.go includes this event. When I run this event, its only removing written content by this event not previously written content,
$ sudo docker run -it container sh
/app # echo "FALCO FALCO FALCO" > /tmp/test.go
/app # cat /tmp/test.go
FALCO FALCO FALCO
/app # go run main.go
Modification completed successfully.
/app # cat /tmp/test.go
FALCO FALCO FALCO
/app #
There was a problem hiding this comment.
The latest approach SMGT. I've just to look deeper into it.
cc @loresuso wdyt?
There was a problem hiding this comment.
Yes, I looked into this and after a refresher on the vuln the last approach sounds good to me as well to reproduce and trigger the rule
Fun fact is how this involve proc filesystem pretty much like latest leaky vessels somehow, although different effects :)
Signed-off-by: h4l0gen <[email protected]> squashed commits Signed-off-by: h4l0gen <[email protected]>
|
LGTM label has been added. Git tree hash: 086d75de9178696d3e7345cbbf39d7b9825b3fb7
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: h4l0gen, leogr 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 |
What type of PR is this?
/kind feature
Any specific area of the project related to this PR?
/area events
What this PR does / why we need it:
this event must to have as it try to create environment of
Container Breakout Exploit (CVE-2019-5736)Which issue(s) this PR fixes:
Fixes #186
Special notes for your reviewer: