-
Notifications
You must be signed in to change notification settings - Fork 948
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
feature: add pouch cp cmd #2879
feature: add pouch cp cmd #2879
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2879 +/- ##
==========================================
- Coverage 69.08% 68.77% -0.32%
==========================================
Files 287 291 +4
Lines 17947 18230 +283
==========================================
+ Hits 12399 12538 +139
- Misses 4128 4232 +104
- Partials 1420 1460 +40
|
daemon/mgr/container_copy.go
Outdated
if err != nil { | ||
return nil, err | ||
} | ||
defer mgr.Unmount(ctx, c) |
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.
Hi Yue,
Would you please take a look at moby/moby#39252 and (kubernetes/kubernetes#75037) at your convenience?
cp command are vulnerable to a symlink-exchange attack with Directory Traversal, giving attackers arbitrary read-write access to the host filesystem with root privileges.
The kubectl cp command allows copying files between containers and the user machine. To copy files from a container, Kubernetes creates a tar inside the container, copies it over the network, and kubectl unpacks it on the user’s machine.
If the tar binary in the container is malicious, it could run any code and output unexpected, malicious results. An attacker could use this to write files to any path on the user’s machine when kubectl cp is called, limited only by the system permissions of the local user.
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.
I think the latter one we don't have risk, since we don't support follow link now. But the former one issue in moby we have same risk. invite @allencloud @fuweid to see.
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.
I'm not sure it's necessary to do filesystem operations(mount/umount) in pouch cp
, I was wondering if it's possible to implement cp
by using tar/untar like kubectl cp
?
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.
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.
kubectl cp
doesn't work well when the container is in distroless mode, it also may frustrate by some malicious image with bad tar binary
@CodeJuan
Actually, the solution of this issue #39292 might solve the problem of TOCTTOU attack
, and I am really excited about the newCommand docker-tar
which is chroot tar. Whether it couldn't be merged asap due to it changes the core piece of docker ?@ZYecho
Is there another method to fix the problem with cp
? how about using another ephemeral container
to enter the namespace of target container ,and exchange data with it?
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.
- stopped container: cp doesn't work
- stopped container -> cp action -> restart: containerd will raise error like
root@ubuntu-xenial ~/g/s/g/a/pouch# pouch start 986486 5c253f9!?
Error: failed to start containers: {"message":"failed to create container(986486d7c6b6a64a987d6083192dc6e081fba9e504cf740ff28f9fd2e8e7beb3) on containerd: failed to create task for container(986486d7c6b6a64a987d6083192dc6e081fba9e504cf740ff28f9fd2e8e7beb3): mkdir /var/lib/pouch/containerd/state/io.containerd.runtime.v1.linux/default/986486d7c6b6a64a987d6083192dc6e081fba9e504cf740ff28f9fd2e8e7beb3: file exists: unknown"}
It seems that cleanup action is not right.
@fuweid PTAL. |
985ac07
to
73456e5
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.
LGTM
Signed-off-by: zhangyue <zy675793960@yeah.net>
Merging... |
Signed-off-by: zhangyue zy675793960@yeah.net
Ⅰ. Describe what this PR did
add pouch cp cmd.
take part of #2182
Ⅱ. Does this pull request fix one issue?
fix #2609
Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗?)
add later
Ⅳ. Describe how to verify it
add later
Ⅴ. Special notes for reviews
didn't support follow-link and copy uid/gid maps now.