[lxc-devel] [lxd/master] idmapping: disallow hostids intersecting subids
brauner on Github
lxc-bot at linuxcontainers.org
Sun Sep 17 04:54:18 UTC 2017
A non-text attachment was scrubbed...
Name: not available
Type: text/x-mailbox
Size: 1247 bytes
Desc: not available
URL: <http://lists.linuxcontainers.org/pipermail/lxc-devel/attachments/20170917/12a43dae/attachment.bin>
-------------- next part --------------
From 1bcc41a70cc496b3c5f0241058cbabc6bc27b9f8 Mon Sep 17 00:00:00 2001
From: Christian Brauner <christian.brauner at ubuntu.com>
Date: Sun, 17 Sep 2017 05:57:06 +0200
Subject: [PATCH 1/2] idmapping: disallow hostids intersecting subids
Our code has explicitly disallowed this for a long time but somehow we have
never correctly errored out in these cases like we should.
After having discussed this with Eric at Plumbers I think this should be
considered a security liability. The problem I see with this is that it
accidently allows users to escalate a container that is unprivileged to a
privileged container without their knowledge. The most obvious case is when
the allocated range of subids is accidently 0-65536 which is rare but basically
runs afoul of creating an unprivileged container but there are more subtle
problems when using "{uid,gid,both} <id> <id>" directly so I prefer to disallow
this for now. Especially since this feature is currently not very useful since
the maximum number of allowed idmappings is currently capped at five so this
would overflow quite easy.
Closes #3416.
Signed-off-by: Christian Brauner <christian.brauner at ubuntu.com>
---
lxd/container_lxc.go | 49 +++++++++++++++++++++++++++++++++++-------
shared/idmap/idmapset_linux.go | 4 +++-
2 files changed, 44 insertions(+), 9 deletions(-)
diff --git a/lxd/container_lxc.go b/lxd/container_lxc.go
index eba02ad72..1724d20d1 100644
--- a/lxd/container_lxc.go
+++ b/lxd/container_lxc.go
@@ -642,12 +642,22 @@ func findIdmap(state *state.State, cName string, isolatedStr string, configBase
return nil, 0, err
}
+ // Check that this is not an identical mapping.
+ for _, v := range state.OS.IdmapSet.Idmap {
+ if v.Nsid == v.Hostid {
+ return nil, 0, idmap.ErrHostIdIsSubId
+ }
+ }
+
if !isolated {
newIdmapset := idmap.IdmapSet{Idmap: make([]idmap.IdmapEntry, len(state.OS.IdmapSet.Idmap))}
copy(newIdmapset.Idmap, state.OS.IdmapSet.Idmap)
for _, ent := range rawMaps {
- newIdmapset.AddSafe(ent)
+ err := newIdmapset.AddSafe(ent)
+ if err != nil && err == idmap.ErrHostIdIsSubId {
+ return nil, 0, err
+ }
}
return &newIdmapset, 0, nil
@@ -658,17 +668,20 @@ func findIdmap(state *state.State, cName string, isolatedStr string, configBase
return nil, 0, err
}
- mkIdmap := func(offset int64, size int64) *idmap.IdmapSet {
+ mkIdmap := func(offset int64, size int64) (*idmap.IdmapSet, error) {
set := &idmap.IdmapSet{Idmap: []idmap.IdmapEntry{
{Isuid: true, Nsid: 0, Hostid: offset, Maprange: size},
{Isgid: true, Nsid: 0, Hostid: offset, Maprange: size},
}}
for _, ent := range rawMaps {
- set.AddSafe(ent)
+ err := set.AddSafe(ent)
+ if err != nil && err == idmap.ErrHostIdIsSubId {
+ return nil, err
+ }
}
- return set
+ return set, nil
}
if configBase != "" {
@@ -677,7 +690,12 @@ func findIdmap(state *state.State, cName string, isolatedStr string, configBase
return nil, 0, err
}
- return mkIdmap(offset, size), offset, nil
+ set, err := mkIdmap(offset, size)
+ if err != nil && err == idmap.ErrHostIdIsSubId {
+ return nil, 0, err
+ }
+
+ return set, offset, nil
}
idmapLock.Lock()
@@ -735,7 +753,12 @@ func findIdmap(state *state.State, cName string, isolatedStr string, configBase
continue
}
- return mkIdmap(offset, size), offset, nil
+ set, err := mkIdmap(offset, size)
+ if err != nil && err == idmap.ErrHostIdIsSubId {
+ return nil, 0, err
+ }
+
+ return set, offset, nil
}
if mapentries[i-1].Hostid+mapentries[i-1].Maprange > offset {
@@ -745,13 +768,23 @@ func findIdmap(state *state.State, cName string, isolatedStr string, configBase
offset = mapentries[i-1].Hostid + mapentries[i-1].Maprange
if offset+size < mapentries[i].Hostid {
- return mkIdmap(offset, size), offset, nil
+ set, err := mkIdmap(offset, size)
+ if err != nil && err == idmap.ErrHostIdIsSubId {
+ return nil, 0, err
+ }
+
+ return set, offset, nil
}
offset = mapentries[i].Hostid + mapentries[i].Maprange
}
if offset+size < state.OS.IdmapSet.Idmap[0].Hostid+state.OS.IdmapSet.Idmap[0].Maprange {
- return mkIdmap(offset, size), offset, nil
+ set, err := mkIdmap(offset, size)
+ if err != nil && err == idmap.ErrHostIdIsSubId {
+ return nil, 0, err
+ }
+
+ return set, offset, nil
}
return nil, 0, fmt.Errorf("Not enough uid/gid available for the container.")
diff --git a/shared/idmap/idmapset_linux.go b/shared/idmap/idmapset_linux.go
index aa9241c1e..3b68d0872 100644
--- a/shared/idmap/idmapset_linux.go
+++ b/shared/idmap/idmapset_linux.go
@@ -344,6 +344,8 @@ func (m IdmapSet) ValidRanges() ([]*IdRange, error) {
return ranges, nil
}
+var ErrHostIdIsSubId = fmt.Errorf("Host id is in the range of subids")
+
/* AddSafe adds an entry to the idmap set, breaking apart any ranges that the
* new idmap intersects with in the process.
*/
@@ -357,7 +359,7 @@ func (m *IdmapSet) AddSafe(i IdmapEntry) error {
}
if e.HostidsIntersect(i) {
- return fmt.Errorf("can't map the same host ID twice")
+ return ErrHostIdIsSubId
}
added = true
From 6ce5edeb59730849e19ad972bbeeb5537e2b9b0f Mon Sep 17 00:00:00 2001
From: Christian Brauner <christian.brauner at ubuntu.com>
Date: Sun, 17 Sep 2017 06:52:05 +0200
Subject: [PATCH 2/2] idmap: add test for {hostid,subid} overlap
Signed-off-by: Christian Brauner <christian.brauner at ubuntu.com>
---
test/suites/idmap.sh | 35 +++++++++++++++++++++++++++++++++++
1 file changed, 35 insertions(+)
diff --git a/test/suites/idmap.sh b/test/suites/idmap.sh
index edff94cb3..d4a23f117 100644
--- a/test/suites/idmap.sh
+++ b/test/suites/idmap.sh
@@ -1,11 +1,13 @@
test_idmap() {
# Check that we have a big enough range for this test
if [ ! -e /etc/subuid ] && [ ! -e /etc/subgid ]; then
+ SUBID_FILES_EXISTED=0
UIDs=1000000000
GIDs=1000000000
UID_BASE=1000000
GID_BASE=1000000
else
+ SUBID_FILES_EXISTED=1
UIDs=0
GIDs=0
UID_BASE=0
@@ -219,4 +221,37 @@ EOF
[ "$(stat -c '%u:%g' "/proc/${PID}/root/d")" = "$((UID_BASE+29)):$((GID_BASE+29))" ]
lxc delete idmap --force
+
+ # Check that we have a big enough range for this test
+ if [ "${SUBID_FILES_EXISTED}" -eq 0 ]; then
+ UIDs=1000000000
+ GIDs=1000000000
+ UID_BASE=0
+ GID_BASE=0
+ else
+ UIDs=1000000000
+ GIDs=1000000000
+ UID_BASE=0
+ GID_BASE=0
+
+ mv /etc/subuid /etc/subuid_backup
+ mv /etc/subgid /etc/subgid_backup
+
+ echo "lxd:"${UID_BASE}":${UIDs}" > /etc/subuid
+ echo "lxd:"${UID_BASE}":${GIDs}" > /etc/subgid
+ echo "root:"${GID_BASE}":${UIDs}" > /etc/subuid
+ echo "root:"${GID_BASE}":${GIDs}" > /etc/subgid
+ fi
+
+ cleanup() {
+ if [ "${SUBID_FILES_EXISTED}" -eq 1 ]; then
+ mv /etc/subuid_backup /etc/subuid
+ mv /etc/subgid_backup /etc/subgid
+ fi
+ }
+
+ trap cleanup EXIT HUP INT TERM
+
+ ! lxc launch testimage idmap
+ ! lxc launch testimage idmap -c raw.idmap="uid 1000 1000"
}
More information about the lxc-devel
mailing list