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

newuidmap requires CAP_SYS_ADMIN (rather than CAP_SET{U,G}ID)? #170

Closed
AkihiroSuda opened this issue Oct 8, 2018 · 7 comments
Closed

newuidmap requires CAP_SYS_ADMIN (rather than CAP_SET{U,G}ID)? #170

AkihiroSuda opened this issue Oct 8, 2018 · 7 comments
Labels

Comments

@AkihiroSuda
Copy link
Collaborator

AkihiroSuda commented Oct 8, 2018

apiVersion: v1
kind: Pod
metadata:
  labels:
    run: img
  name: img
  annotations:
    container.apparmor.security.beta.kubernetes.io/img: unconfined
spec:
  securityContext:
    runAsUser: 1000
  initContainers:
    # This container clones the desired git repo to the EmptyDir volume.
    - name: git-clone
      image: r.j3ss.co/jq
      args:
        - git
        - clone
        - --single-branch
        - --
        - https://github.com/jessfraz/dockerfiles
        - /repo # Put it in the volume
      securityContext:
        allowPrivilegeEscalation: false
        readOnlyRootFilesystem: true
      volumeMounts:
        - name: git-repo
          mountPath: /repo
  containers:
  - image: r.j3ss.co/img
    imagePullPolicy: Always
    name: img
    resources: {}
    workingDir: /repo
    command:
    - img
    - build
    - -t
    - irssi
    - irssi/
    securityContext:
      procMount: Unmasked
      capabilities:
        add:
        - SYS_ADMIN
    volumeMounts:
    - name: cache-volume
      mountPath: /tmp
    - name: git-repo
      mountPath: /repo
  volumes:
  - name: cache-volume
    emptyDir: {}
  - name: git-repo
    emptyDir: {}
  restartPolicy: Never

The YAML above (based on https://blog.jessfraz.com/post/building-container-images-securely-on-kubernetes/) works with Kubernetes 1.12.1 + Docker 18.06.1 (Minikube PR: kubernetes/minikube#3223).

However, when I add SETUID and SETGID to capabilities instead of SYS_ADMIN, newuidmap fails:
(EDIT: SETUID and SETGID are already in the default set, and adding them explicltly is just "NOP")

newuidmap: write to uid_map failed: Operation not permitted
nsenter: failed to use newuidmap: Invalid argument
nsenter: failed to sync with parent: SYNC_USERMAP_ACK: got 255: Invalid argument

cc @jessfraz @cyphar @giuseppe

@AkihiroSuda
Copy link
Collaborator Author

(Actually this question/issue is not specific to img, and I'm not sure this repo is the right place to ask this question :P)

@cyphar
Copy link

cyphar commented Oct 8, 2018

A better place to ask would be https://github.com/shadow-maint/shadow. But the answer is that the in-kernel check for /proc/self/uid_map requires both CAP_SETUID and CAP_SYS_ADMIN -- it's not just new_idmap_permitted but there's also a file_ns_capable(file, ns, CAP_SYS_ADMIN) check in map_write.

(Funnily enough this means that you could design a newuidmap that wouldn't need CAP_SYS_ADMIN if you passed a file descriptor from the container's user namespace -- but that would no longer easily allow newuidmap to verify that it is writing to a process's uid_map file.)

@giuseppe
Copy link
Contributor

giuseppe commented Oct 8, 2018

going quickly through the kernel user_namespace.c file, it seems that if the euid!=owner of the userns the kernel requires CAP_SYS_ADMIN in the parent user namespace.

I am not sure if the correct fix should be in the kernel, but this PR for newuidmap/newgidmap seems to do the trick as well: shadow-maint/shadow#132

@giuseppe
Copy link
Contributor

giuseppe commented Oct 8, 2018

@cyphar I've not seen your reply when preparing mine :-) so perhaps my proposed solution is not completely wrong

@jessfraz
Copy link
Collaborator

jessfraz commented Oct 8, 2018

wow good to know I am slightly weirded out I didn't hit this before...

@AkihiroSuda
Copy link
Collaborator Author

thank all for the info

the check seems added in 2013.. torvalds/linux@41c21e3

AkihiroSuda added a commit to AkihiroSuda/img that referenced this issue Oct 9, 2018
Applies shadow-maint/shadow#132 so that we
don't need to have CAP_SYS_ADMIN.

See also genuinetools#170 .

Signed-off-by: Akihiro Suda <[email protected]>
@AkihiroSuda
Copy link
Collaborator Author

PR: #171

jessfraz pushed a commit that referenced this issue Oct 9, 2018
* use Giuseppe's forked newuidmap/newgidmap

Applies shadow-maint/shadow#132 so that we
don't need to have CAP_SYS_ADMIN.

See also #170 .

Signed-off-by: Akihiro Suda <[email protected]>

* shut up codacy

Signed-off-by: Akihiro Suda <[email protected]>
AkihiroSuda added a commit to AkihiroSuda/img that referenced this issue Oct 29, 2018
jessfraz pushed a commit that referenced this issue Oct 29, 2018
Close #170

Signed-off-by: Akihiro Suda <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants