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

Potential symlink/directory traversal issue in user ID extraction #66

Closed

fweimer opened this issue Apr 12, 2017 · 11 comments

Assignees

Labels

locked - please file new issue/PR

fweimer commented Apr 12, 2017

Commit b1bb73e (“ Teach "Run" to dig user IDs out of containers”) added the fopenContainerFile function. It does not correctly deal with symbolic links, which could point to something outside of the container root.

The text was updated successfully, but these errors were encountered:

nalind referenced this issue in nalind/buildah


          fopenContainerFile: switch to using EvalSymlinks

97ca24a

Switch fopenContainerFile from using Stat/Lstat after opening the file
to using EvalSymlinks to resolve the file's path as a symlink, checking
that the result is inside of the container's root fs, and then opening
the link's target instead.  This should resolve #66.

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

Member

nalind commented Apr 12, 2017

Are you suggesting something like nalind@97ca24a? What did we not catch with the (possibly over-restrictive) lstat-after-opening method?

Member

nalind commented Apr 12, 2017

(8) [Buildah] Add "dockah build-using-dockerfile" aka "buildah bud"

1 similar comment

Member

nalind commented Apr 12, 2017

(8) [Buildah] Add "dockah build-using-dockerfile" aka "buildah bud"

nalind referenced this issue in nalind/buildah


          fopenContainerFile: switch to using EvalSymlinks

2c38752

Switch fopenContainerFile from using Stat/Lstat after opening the file
to using EvalSymlinks to resolve the file's path as a symlink, checking
that the result is inside of the container's root fs, and then opening
the link's target instead.  This should resolve #66.

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

Member

nalind commented Apr 27, 2017

@fweimer The patch with the fix I'm looking at had a conflict in it, so I've rebased it to nalind@2c38752. I'd appreciate any guidance you can offer.

Author

fweimer commented Apr 28, 2017 •

edited

Loading

@nalind, when this code is run, is the file system directory tree dead and inaccessible to unprivileged processes, or is concurrent modification of the tree possible?

Member

nalind commented May 1, 2017

The filesystem tree is mounted on the host, so modification is possible for users with the necessary privileges.

Author

fweimer commented May 3, 2017

@nalind, in this case, the code still has a time-of-check-time-of-use race condition because after symbolic link resolution, the file system tree could change and contain symbolic links again.

You either have to use chroot to restrict pathname lookups (no longer needs root in current Fedora), or reimplement kernel pathname lookup using openat and O_PATH.

nalind self-assigned this

Member

nalind commented May 9, 2017

[buildah] Fix potential symlink traversal problem when looking up numeric IDs for USER settings

nalind referenced this issue in nalind/buildah


          fopenContainerFile: scope filename lookups better

cc84857

Switch fopenContainerFile from using Stat/Lstat after opening the file
to using openat() to walk the given path, resolving links to keep them
from escaping the container's root fs.  This should resolve #66.

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

nalind referenced this issue in nalind/buildah


          fopenContainerFile: scope filename lookups better

c2011c2

Switch fopenContainerFile from using Stat/Lstat after opening the file
to using openat() to walk the given path, resolving links to keep them
from escaping the container's root fs.  This should resolve #66.

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

nalind referenced this issue in nalind/buildah


          fopenContainerFile: scope filename lookups better

494bebe

Switch fopenContainerFile from using Stat/Lstat after opening the file
to using openat() to walk the given path, resolving links to keep them
from escaping the container's root fs.  This should resolve #66.

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

Member

rhatdan commented Jul 14, 2017

@nalind have you done anything else for this issue?

Member

rhatdan commented Sep 28, 2017

@nalind any update on this?

nalind referenced this issue in nalind/buildah


          fopenContainerFile: scope filename lookups better

d1b0ed8

Switch fopenContainerFile from using Stat/Lstat after opening the file
to using openat() to walk the given path, resolving links to keep them
from escaping the container's root fs.  This should resolve #66.

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

Member

nalind commented Nov 3, 2017

Alright, I'm beginning to think there's no way around having to use chroot(). master...nalind:user2 contains what I'm thinking should prevent the directory traversal problem.

nalind referenced this issue in nalind/buildah


          fopenContainerFile: scope filename lookups better

21a5136

Switch fopenContainerFile from using Stat/Lstat after opening the file
to using openat() to walk the given path, resolving links to keep them
from escaping the container's root fs.  This should resolve #66.

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

nalind referenced this issue in nalind/buildah


          Rework how we do UID resolution in images

8cd668a

* Use chroot() instead of trying to read the right file ourselves.

This should resolve #66.

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

nalind referenced this issue in nalind/buildah


          Rework how we do UID resolution in images

8996f14

* Use chroot() instead of trying to read the right file ourselves.

This should resolve #66.

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

nalind referenced this issue in nalind/buildah


          Rework how we do UID resolution in images

e52ba20

* Use chroot() instead of trying to read the right file ourselves.

This should resolve #66.

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

nalind mentioned this issue

Potential symlink/directory traversal issue in user ID extraction containers/podman#29

Closed

nalind referenced this issue in nalind/buildah


          Rework how we do UID resolution in images

c9729f9

* Use chroot() instead of trying to read the right file ourselves.

This should resolve #66.

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

nalind referenced this issue in nalind/buildah


          fopenContainerFile: scope filename lookups better

7f14125

Switch fopenContainerFile from using Stat/Lstat after opening the file
to using openat() to walk the given path, resolving links to keep them
from escaping the container's root fs.  This should resolve #66.

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

nalind referenced this issue in nalind/buildah


          Rework how we do UID resolution in images

aa53607

* Use chroot() instead of trying to read the right file ourselves.

This should resolve #66.

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

nalind referenced this issue in nalind/buildah


          Rework how we do UID resolution in images

599d977

* Use chroot() instead of trying to read the right file ourselves.

This should resolve #66.

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

nalind referenced this issue in nalind/buildah


          Rework how we do UID resolution in images

f851bc6

* Use chroot() instead of trying to read the right file ourselves.

This should resolve #66.

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

nalind referenced this issue in nalind/buildah


          fopenContainerFile: scope filename lookups better

41b9b14

Switch fopenContainerFile from using Stat/Lstat after opening the file
to using openat() to walk the given path, resolving links to keep them
from escaping the container's root fs.  This should resolve #66.

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

nalind referenced this issue in nalind/buildah


          Rework how we do UID resolution in images

0c7eca9

* Use chroot() instead of trying to read the right file ourselves.

This should resolve #66.

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

rh-atomic-bot pushed a commit that referenced this issue


          Rework how we do UID resolution in images

52e2737

* Use chroot() instead of trying to read the right file ourselves.

This should resolve #66.

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

Closes: #313
Approved by: rhatdan

rh-atomic-bot closed this as completed in

9e129fd

nalind pushed a commit that referenced this issue


          Update kpod pause and unpause to use new container state

b1a3b03

Signed-off-by: umohnani8 <umohnani@redhat.com>

Closes: #66
Approved by: mheon

nalind mentioned this issue

Add --chown option to add/copy #336

Closed

nalind mentioned this issue

migrate from chrootuser to libpod/pkg/lookup #1141

Closed

github-actions bot added the locked - please file new issue/PR label

github-actions bot locked as resolved and limited conversation to collaborators

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.