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.

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

fix permission denied #2086

Merged
merged 1 commit into from Dec 17, 2019
Merged

fix permission denied #2086

merged 1 commit into from Dec 17, 2019

Conversation

win-t
Copy link
Contributor

@win-t win-t commented Jul 17, 2019

when exec as root and config.Cwd is not owned by root, exec will fail
because root doesn't have the caps.

So, Chdir should be done before setting the caps.

to reproduce this error

  1. create Dockerfile
FROM alpine:latest

RUN mkdir /lala && chown nobody:nobody /lala && chmod 0700 /lala
USER nobody
WORKDIR /lala

ENTRYPOINT ["sh", "-c", "mkfifo lala; cat lala"]
  1. build and run the container
docker build -t asdf .
docker run -it --rm --name hai asdf
  1. docker exec from another terminal
docker exec -it -u 0:0 hai sh

libcontainer/init_linux.go Outdated Show resolved Hide resolved
when exec as root and config.Cwd is not owned by root, exec will fail
because root doesn't have the caps.

So, Chdir should be done before setting the caps.

Signed-off-by: Kurnia D Win <kurnia.d.win@gmail.com>
@crosbymichael
Copy link
Member

crosbymichael commented Sep 10, 2019

LGTM

Approved with PullApprove

@siscia
Copy link

siscia commented Dec 16, 2019

ping!

@cyphar
Copy link
Member

cyphar commented Dec 17, 2019

LGTM.

Approved with PullApprove

@Random-Liu
Copy link

Random-Liu commented Feb 14, 2020

Does this introduce a regression?
See containerd/cri#1397 (comment) and moby/moby#36587

@crosbymichael @cyphar

@justincormack
Copy link
Contributor

I believe this is incorrect as @Random-Liu suggests; I am unsure why you accepted a PR with a repro just from docker exec not runc as docker exec has some interesting properties in that it inherits some properties from the original run.

@win-t
Copy link
Contributor Author

win-t commented Feb 27, 2020

@justincormack I can reproduce this just with old version of runc
it is just more easy with docker

@cyphar
Copy link
Member

cyphar commented Mar 3, 2020

I can see the argument for this new behaviour being wrong (in theory you could bypass some DAC controls in very odd setups by setting your working directory to be inside a path which is world-rx but has a parent which the use doesn't have rx permissions on.

I'm okay with reverting this, but I don't really feel that either behaviour is necessarily wrong. After all, we've always said that being able to run custom container configurations is equivalent to running as root (not that anyone listens...).

@kolyshkin
Copy link
Contributor

At least we should not merge PRs with no runc repro / integration test case.

This was some arbitrary change that fixed one workflow and broke another (#2685), and there is no test case to see if the fix from #2685 results in any regressions :(

@kolyshkin
Copy link
Contributor

OK here is the test case which fails before and passes after this PR.

@test "runc exec --user with no access to cwd" {
        requires root

        chown 42 rootfs/root
        chmod 700 rootfs/root

        update_config '   .process.cwd = "/root"
                        | .process.user.uid = 42
                        | .process.args |= ["sleep", "1h"]'

        runc run -d --console-socket "$CONSOLE_SOCKET" test_busybox
        [ "$status" -eq 0 ]

        runc exec --user 0 test_busybox true
        [ "$status" -eq 0 ]
}

kolyshkin added a commit to kolyshkin/runc that referenced this pull request Dec 18, 2020
Commit 5e0e67d ("fix permission denied") modified some code but
did not provide a test case.

This is a test case that was tested to fail before and succeed after
the above commit.

For more details, see opencontainers#2086

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
ctalledo pushed a commit to ctalledo/sysbox-runc that referenced this pull request Jan 6, 2021
Commit 5e0e67d ("fix permission denied") modified some code but
did not provide a test case.

This is a test case that was tested to fail before and succeed after
the above commit.

For more details, see opencontainers/runc#2086

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
dqminh pushed a commit to dqminh/runc that referenced this pull request Feb 3, 2021
Commit 5e0e67d ("fix permission denied") modified some code but
did not provide a test case.

This is a test case that was tested to fail before and succeed after
the above commit.

For more details, see opencontainers#2086

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants