Stop sorting uid and gid ranges in id maps by dohse · Pull Request #39288 · moby/moby

dohse · 2019-05-30T14:49:59Z

Moby currently sorts uid and gid ranges in id maps. This causes subuid
and subgid files to be interpreted wrongly.

With user namespace remapping enabled like this:

> sudo cat /etc/docker/daemon.json
…
{
  …
  "userns-remap": "jonas"
}

the subuid file

> cat /etc/subuid
jonas:100000:1000
jonas:1000:1

configures that the container uids 0-999 are mapped to the host uids
100000-100999 and uid 1000 in the container is mapped to uid 1000 on the
host. The expected uid_map is:

> docker run ubuntu cat /proc/self/uid_map
         0     100000       1000
      1000       1000          1

Moby currently sorts the ranges by the first id in the range. Therefore
with the subuid file above the uid 0 in the container is mapped to uid
100000 on host and the uids 1-1000 in container are mapped to the uids
1-1000 on the host. The resulting uid_map is:

> docker run ubuntu cat /proc/self/uid_map
         0       1000          1
         1     100000       1000

The ordering was implemented to work around a limitation in Linux 3.8.
This is fixed since Linux 3.9 as stated on the user namespaces manpage
[1]:

In the initial implementation (Linux 3.8), this requirement was
satisfied by a simplistic implementation that imposed the further
requirement that the values in both field 1 and field 2 of successive
lines must be in ascending numerical order, which prevented some
otherwise valid maps from being created. Linux 3.9 and later fix this
limitation, allowing any valid set of nonoverlapping maps.

This fix changes the interpretation of subuid and subgid files which do
not have the ids of in the numerical order for each individual user.
This breaks users that rely on the current behaviour.

The desired mapping above - map low user ids in the container to high
user ids on the host and some higher user ids in the container to lower
user on host - can unfortunately not archived with the current
behaviour.

[1] http://man7.org/linux/man-pages/man7/user_namespaces.7.html

Signed-off-by: Jonas Dohse [email protected]

AkihiroSuda · 2019-06-03T10:44:51Z

Thanks, can we have unit tests?

AkihiroSuda · 2019-06-03T10:45:53Z

If the system is running on kernel < 3.9, can we fall back to the old behavior or error out?

thaJeztah · 2019-06-03T11:26:46Z

@estesp ptal

dohse · 2019-06-03T12:42:13Z

I added a unit test.

dohse · 2019-06-03T12:51:46Z

Regarding Linux < 3.9: According to the user namespaces manpage [1] writing an invalid [ug]id_map will fail:

Writes that violate the above rules fail with the error EINVAL.

Are Linux versions below 3.10 still supported?

[1] http://man7.org/linux/man-pages/man7/user_namespaces.7.html

thaJeztah · 2019-06-03T13:10:57Z

Looks like there's a linting failure;

12:51:38 pkg/idtools/idtools_test.go:1::warning: file is not gofmted with -s (gofmt)
12:51:38 pkg/idtools/idtools_test.go:1::warning: file is not goimported (goimports)
12:51:39 Build step 'Execute shell' marked build as failure

Are Linux versions below 3.10 still supported?

I don't think anything older is still supported; RHEL 7 / CentOS 7 use kernel 3.10 (well, their variation of it), which is the oldest that's supported.

AkihiroSuda

LGTM after lint fix

Moby currently sorts uid and gid ranges in id maps. This causes subuid and subgid files to be interpreted wrongly. The subuid file ``` > cat /etc/subuid jonas:100000:1000 jonas:1000:1 ``` configures that the container uids 0-999 are mapped to the host uids 100000-100999 and uid 1000 in the container is mapped to uid 1000 on the host. The expected uid_map is: ``` > docker run ubuntu cat /proc/self/uid_map 0 100000 1000 1000 1000 1 ``` Moby currently sorts the ranges by the first id in the range. Therefore with the subuid file above the uid 0 in the container is mapped to uid 100000 on host and the uids 1-1000 in container are mapped to the uids 1-1000 on the host. The resulting uid_map is: ``` > docker run ubuntu cat /proc/self/uid_map 0 1000 1 1 100000 1000 ``` The ordering was implemented to work around a limitation in Linux 3.8. This is fixed since Linux 3.9 as stated on the user namespaces manpage [1]: > In the initial implementation (Linux 3.8), this requirement was > satisfied by a simplistic implementation that imposed the further > requirement that the values in both field 1 and field 2 of successive > lines must be in ascending numerical order, which prevented some > otherwise valid maps from being created. Linux 3.9 and later fix this > limitation, allowing any valid set of nonoverlapping maps. This fix changes the interpretation of subuid and subgid files which do not have the ids of in the numerical order for each individual user. This breaks users that rely on the current behaviour. The desired mapping above - map low user ids in the container to high user ids on the host and some higher user ids in the container to lower user on host - can unfortunately not archived with the current behaviour. [1] http://man7.org/linux/man-pages/man7/user_namespaces.7.html Signed-off-by: Jonas Dohse <[email protected]>

dohse · 2019-06-03T13:52:23Z

I applied gofmt to idtools_test.go

thaJeztah

LGTM

estesp

LGTM

thaJeztah · 2019-06-04T18:11:59Z

Windows RS1 failure is unrelated

Fixes CVE-2018-15664: API endpoints behind the 'docker 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, because daemon/archive.go does not do archive operations on a frozen filesystem (or from within a chroot). And includes additional post-18.09.6 fixes: Builder - Fixed a panic error when building dockerfiles that contain only comments. moby/moby#38487 - Added a workaround for GCR authentication issue. moby/moby#38246 - Builder-next: Fixed a bug in the GCR token cache implementation workaround. moby/moby#39183 Runtime - Added performance optimizations in aufs and layer store that helps in massively parallel container creation and removal. moby/moby#39107, moby/moby#39135 - daemon: fixed a mirrors validation issue. moby/moby#38991 - Docker no longer supports sorting UID and GID ranges in ID maps. moby/moby#39288 Logging - Added a fix that now allows large log lines for logger plugins. moby/moby#39038 Signed-off-by: Peter Korsgaard <[email protected]> Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <[email protected]>

Fixes CVE-2018-15664: API endpoints behind the 'docker 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, because daemon/archive.go does not do archive operations on a frozen filesystem (or from within a chroot). And includes additional post-18.09.6 fixes: Builder - Fixed a panic error when building dockerfiles that contain only comments. moby/moby#38487 - Added a workaround for GCR authentication issue. moby/moby#38246 - Builder-next: Fixed a bug in the GCR token cache implementation workaround. moby/moby#39183 Runtime - Added performance optimizations in aufs and layer store that helps in massively parallel container creation and removal. moby/moby#39107, moby/moby#39135 - daemon: fixed a mirrors validation issue. moby/moby#38991 - Docker no longer supports sorting UID and GID ranges in ID maps. moby/moby#39288 Logging - Added a fix that now allows large log lines for logger plugins. moby/moby#39038 Signed-off-by: Peter Korsgaard <[email protected]> Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <[email protected]> (cherry picked from commit 13cf6f0) Signed-off-by: Peter Korsgaard <[email protected]>

Fixes CVE-2018-15664: API endpoints behind the 'docker 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, because daemon/archive.go does not do archive operations on a frozen filesystem (or from within a chroot). And includes additional post-18.09.6 fixes: Builder - Fixed a panic error when building dockerfiles that contain only comments. moby/moby#38487 - Added a workaround for GCR authentication issue. moby/moby#38246 - Builder-next: Fixed a bug in the GCR token cache implementation workaround. moby/moby#39183 Runtime - Added performance optimizations in aufs and layer store that helps in massively parallel container creation and removal. moby/moby#39107, moby/moby#39135 - daemon: fixed a mirrors validation issue. moby/moby#38991 - Docker no longer supports sorting UID and GID ranges in ID maps. moby/moby#39288 Logging - Added a fix that now allows large log lines for logger plugins. moby/moby#39038 Signed-off-by: Peter Korsgaard <[email protected]> Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <[email protected]> (cherry picked from commit cdbb3ce) Signed-off-by: Peter Korsgaard <[email protected]>

Fixes CVE-2018-15664: API endpoints behind the 'docker 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, because daemon/archive.go does not do archive operations on a frozen filesystem (or from within a chroot). And includes additional post-18.09.6 fixes: Builder - Fixed a panic error when building dockerfiles that contain only comments. moby/moby#38487 - Added a workaround for GCR authentication issue. moby/moby#38246 - Builder-next: Fixed a bug in the GCR token cache implementation workaround. moby/moby#39183 Runtime - Added performance optimizations in aufs and layer store that helps in massively parallel container creation and removal. moby/moby#39107, moby/moby#39135 - daemon: fixed a mirrors validation issue. moby/moby#38991 - Docker no longer supports sorting UID and GID ranges in ID maps. moby/moby#39288 Logging - Added a fix that now allows large log lines for logger plugins. moby/moby#39038 Signed-off-by: Peter Korsgaard <[email protected]> Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <[email protected]> (cherry picked from commit 13cf6f0) Signed-off-by: Peter Korsgaard <[email protected]>

Fixes CVE-2018-15664: API endpoints behind the 'docker 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, because daemon/archive.go does not do archive operations on a frozen filesystem (or from within a chroot). And includes additional post-18.09.6 fixes: Builder - Fixed a panic error when building dockerfiles that contain only comments. moby/moby#38487 - Added a workaround for GCR authentication issue. moby/moby#38246 - Builder-next: Fixed a bug in the GCR token cache implementation workaround. moby/moby#39183 Runtime - Added performance optimizations in aufs and layer store that helps in massively parallel container creation and removal. moby/moby#39107, moby/moby#39135 - daemon: fixed a mirrors validation issue. moby/moby#38991 - Docker no longer supports sorting UID and GID ranges in ID maps. moby/moby#39288 Logging - Added a fix that now allows large log lines for logger plugins. moby/moby#39038 Signed-off-by: Peter Korsgaard <[email protected]> Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <[email protected]> (cherry picked from commit cdbb3ce) Signed-off-by: Peter Korsgaard <[email protected]>

GordonTheTurtle added the status/0-triage label May 30, 2019

dohse mentioned this pull request Jun 3, 2019

User and group ids in /etc/subuid and /etc/subgid are not used in the order specified #39301

Closed

thaJeztah added area/security/userns impact/changelog status/2-code-review and removed status/0-triage labels Jun 3, 2019

thaJeztah added the area/security label Jun 3, 2019

AkihiroSuda approved these changes Jun 3, 2019

View reviewed changes

thaJeztah approved these changes Jun 3, 2019

View reviewed changes

thaJeztah added process/cherry-pick status/4-merge and removed status/2-code-review labels Jun 3, 2019

estesp approved these changes Jun 3, 2019

View reviewed changes

thaJeztah added the rebuild/* label Jun 3, 2019

GordonTheTurtle removed the rebuild/* label Jun 3, 2019

AkihiroSuda added the rebuild/janky label Jun 4, 2019

GordonTheTurtle removed the rebuild/janky label Jun 4, 2019

thaJeztah merged commit bd89b3f into moby:master Jun 4, 2019

This was referenced Jun 5, 2019

[18.09 backport] Stop sorting uid and gid ranges in id maps docker-archive/engine#265

Merged

[19.03 backport] Stop sorting uid and gid ranges in id maps docker-archive/engine#266

Merged

dohse deleted the do-not-order-uid-gid-mappings branch June 6, 2019 13:57

thaJeztah added process/cherry-picked and removed process/cherry-pick labels Jul 13, 2019

thaJeztah added this to the 20.03.0 milestone Apr 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stop sorting uid and gid ranges in id maps#39288

Stop sorting uid and gid ranges in id maps#39288
thaJeztah merged 1 commit intomoby:masterfrom
dohse:do-not-order-uid-gid-mappings

dohse commented May 30, 2019

Uh oh!

AkihiroSuda commented Jun 3, 2019

Uh oh!

AkihiroSuda commented Jun 3, 2019

Uh oh!

thaJeztah commented Jun 3, 2019

Uh oh!

dohse commented Jun 3, 2019

Uh oh!

dohse commented Jun 3, 2019

Uh oh!

thaJeztah commented Jun 3, 2019

Uh oh!

AkihiroSuda left a comment

Uh oh!

dohse commented Jun 3, 2019

Uh oh!

thaJeztah left a comment

Uh oh!

estesp left a comment

Uh oh!

thaJeztah commented Jun 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

dohse commented May 30, 2019

Uh oh!

AkihiroSuda commented Jun 3, 2019

Uh oh!

AkihiroSuda commented Jun 3, 2019

Uh oh!

thaJeztah commented Jun 3, 2019

Uh oh!

dohse commented Jun 3, 2019

Uh oh!

dohse commented Jun 3, 2019

Uh oh!

thaJeztah commented Jun 3, 2019

Uh oh!

AkihiroSuda left a comment

Choose a reason for hiding this comment

Uh oh!

dohse commented Jun 3, 2019

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

estesp left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Jun 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants