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.

Sign up for GitHub

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

Jump to bottom

Remove pkg/symlink from docker/docker and use cyphar/filepath-securejoin #1622

Merged
merged 1 commit into from
Nov 8, 2017
Merged

Remove pkg/symlink from docker/docker and use cyphar/filepath-securejoin #1622

merged 1 commit into from
Nov 8, 2017

Conversation

vdemeester
Copy link
Contributor

runc shouldn't depend on docker and be more self-contained — removing github.com/pkg/symlink dep is the first step to not depend on docker anymore.

The next one is pkg/mount 👼

/cc @jhowardmsft @crosbymichael @hqhq @mrunalp

@vdemeester vdemeester force-pushed the import-symlink-from-docker branch from 7cb524b to 4f90e1a Compare October 27, 2017 13:58
@cyphar
Copy link
Member

cyphar commented Oct 27, 2017

If we're gonna stop using the upstream pkg/symlink -- we should switch entirely to https://github.com/cyphar/filepath-securejoin. And it's versioned properly.

@dqminh
Copy link
Contributor

dqminh commented Oct 27, 2017 •
edited
Loading

Switching to https://github.com/cyphar/filepath-securejoin looks OK to me.

@vdemeester
Copy link
Contributor Author

@cyphar @dqminh oh nice, I would be ok with that too 👼 😉

@vdemeester
Copy link
Contributor Author

I'll update using it 👼

@cyphar
Copy link
Member

cyphar commented Oct 28, 2017

I can work up a PR to switch Docker to use filepath-securejoin as well.

@vdemeester vdemeester force-pushed the import-symlink-from-docker branch from 9341f36 to 41216d9 Compare October 29, 2017 14:01
@vdemeester
Copy link
Contributor Author

@cyphar @dqminh done 👼

@cyphar
Copy link
Member

cyphar commented Oct 29, 2017 •
edited
Loading

There's still a reference to symlink.FollowSymlinkInScope in rootfs_linux. And also it looks like the files aren't gofmt'd correctly. Also roofs is a typo. 😸

@vdemeester
Copy link
Contributor Author

ah yep 😱

@vdemeester vdemeester force-pushed the import-symlink-from-docker branch 2 times, most recently from b59dfbf to cfba712 Compare October 30, 2017 09:41
@vdemeester
Copy link
Contributor Author

oh @cyphar.. some unit-tests are failing 😅

@cyphar
Copy link
Member

cyphar commented Oct 30, 2017

/me will review.

cyphar
cyphar reviewed Oct 31, 2017
View reviewed changes
libcontainer/rootfs_linux.go
@@ -240,7 +240,7 @@ func mountToRootfs(m *configs.Mount, rootfs, mountLabel string) error {
// any previous mounts can invalidate the next mount's destination.
// this can happen when a user specifies mounts within other mounts to cause breakouts or other
// evil stuff to try to escape the container's rootfs.
if dest, err = symlink.FollowSymlinkInScope(dest, rootfs); err != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The correct ordering of arguments would be:

if dest, err = securejoin.SecureJoin(rootfs, m.Destination); err != nil {

However it feels like this section would be a better place to fix:

164 var (
165  	dest = m.Destination
166 )
167 if !strings.HasPrefix(dest, rootfs) {
168 	dest = filepath.Join(rootfs, dest)
169 }

And then the two calls for symlink.FollowSymlinkInScope shouldn't be necessary.

/cc @crosbymichael

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ohhh 😅

@thaJeztah
Copy link
Member

@vdemeester can you update the PR description to match the updated change?

@vdemeester
Copy link
Contributor Author

@thaJeztah oh right 👍

@vdemeester vdemeester changed the title Import pkg/symlink from docker/docker Remove pkg/symlink from docker/docker and use cyphar/filepath-securejoin Oct 31, 2017
@cyphar
Copy link
Member

cyphar commented Oct 31, 2017

Yeah, just to be clear: SecureJoin(a, b) is basically semantically similar to filepath.Join(a, b) except it scopes the output to a. The reason it is different to the usual EvalSymlinksInScope model that Docker used is because people who use EvalSymlinksInScope almost always also use Join -- so might as well combine the two (plus Join does a Clean which makes the results less accurate).

@vdemeester
Use cyphar/filepath-securejoin instead of docker pkg/symlink
5945014 
runc shouldn't depend on docker and be more self-contained.
Removing github.com/pkg/symlink dep is the first step to not depend on docker anymore

Signed-off-by: Vincent Demeester <[email protected]>
@vdemeester vdemeester force-pushed the import-symlink-from-docker branch from cfba712 to 5945014 Compare October 31, 2017 15:54
@tianon tianon mentioned this pull request Nov 2, 2017
Closed
@vdemeester
Copy link
Contributor Author

@cyphar updated 👼

@cyphar
Copy link
Member

cyphar commented Nov 8, 2017 •
edited by caniszczyk
Loading

LGTM.

Approved with PullApprove

@vdemeester vdemeester mentioned this pull request Nov 8, 2017
Merged
@crosbymichael
Copy link
Member

crosbymichael commented Nov 8, 2017 •
edited by caniszczyk
Loading

LGTM

Approved with PullApprove

@crosbymichael crosbymichael merged commit 2f010ec into opencontainers:master Nov 8, 2017
@vdemeester vdemeester deleted the import-symlink-from-docker branch November 8, 2017 15:09
@cyphar cyphar mentioned this pull request Feb 24, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cyphar cyphar cyphar left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@vdemeester @cyphar @dqminh @thaJeztah @crosbymichael