Skip to content

Commit 1e3d10d

Browse files
mbayntonmbaynton
committed
Make ovl idmap mounts read-only
This is a planned follow-on from containerd#10721 primarily at the request of @fuweid, exchanging MNT_DETACH at unmount time for MOUNT_ATTR_RDONLY at mount time. The effect is to increase risk of unmount failure due to EBUSY (as observed in the wild) but add an additional protection that the then-leaked bind mount does not act as a conduit for inadvertent modification of the underlying data, including our own efforts to clean up the mountpoint. Tests covering the lifecycle of the temporary idmap mounts and integrity of the underlying lower layer data is also included in the normal and failed-unmount case. Fixes containerd#10704 Signed-off-by: Mike Baynton <[email protected]>
1 parent 23500b8 commit 1e3d10d

5 files changed

Lines changed: 245 additions & 22 deletions

File tree

‎core/mount/mount_idmapped_linux.go‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -61,14 +61,19 @@ func parseIDMapping(mapping string) ([]syscall.SysProcIDMap, error) {
6161
}, nil
6262
}
6363

64-
// IDMapMount applies GID/UID shift according to gidmap/uidmap for target path
64+
// IDMapMount clones the mount at source to target, applying GID/UID idmapping of the user namespace for target path
6565
func IDMapMount(source, target string, usernsFd int) (err error) {
66+
return IDMapMountWithAttrs(source, target, usernsFd, 0, 0)
67+
}
68+
69+
// IDMapMountWithAttrs clones the mount at source to target with the provided mount options and idmapping of the user namespace.
70+
func IDMapMountWithAttrs(source, target string, usernsFd int, attrSet uint64, attrClr uint64) (err error) {
6671
var (
6772
attr unix.MountAttr
6873
)
6974

70-
attr.Attr_set = unix.MOUNT_ATTR_IDMAP
71-
attr.Attr_clr = 0
75+
attr.Attr_set = unix.MOUNT_ATTR_IDMAP | attrSet
76+
attr.Attr_clr = attrClr
7277
attr.Propagation = 0
7378
attr.Userns_fd = uint64(usernsFd)
7479

@@ -79,7 +84,7 @@ func IDMapMount(source, target string, usernsFd int) (err error) {
7984

8085
defer unix.Close(dFd)
8186
if err = unix.MountSetattr(dFd, "", unix.AT_EMPTY_PATH, &attr); err != nil {
82-
return fmt.Errorf("Unable to shift GID/UID for %s: %w", target, err)
87+
return fmt.Errorf("Unable to shift GID/UID or set mount attrs for %s: %w", target, err)
8388
}
8489

8590
if err = unix.MoveMount(dFd, "", -int(unix.EBADF), target, unix.MOVE_MOUNT_F_EMPTY_PATH); err != nil {

‎core/mount/mount_idmapped_linux_test.go‎

Lines changed: 67 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,12 +18,15 @@ package mount
1818

1919
import (
2020
"fmt"
21+
"io/fs"
2122
"os"
2223
"path/filepath"
2324
"sync"
2425
"syscall"
2526
"testing"
2627

28+
"golang.org/x/sys/unix"
29+
2730
kernel "github.com/containerd/containerd/v2/pkg/kernelversion"
2831
"github.com/containerd/continuity/testutil"
2932
"github.com/stretchr/testify/require"
@@ -84,6 +87,8 @@ func TestIdmappedMount(t *testing.T) {
8487
t.Run("GetUsernsFD", testGetUsernsFD)
8588

8689
t.Run("IDMapMount", testIDMapMount)
90+
91+
t.Run("IDMapMountWithAttrs", testIDMapMountWithAttrs)
8792
}
8893

8994
func testGetUsernsFD(t *testing.T) {
@@ -129,19 +134,60 @@ func testIDMapMount(t *testing.T) {
129134
require.NoError(t, err)
130135
defer usernsFD.Close()
131136

132-
srcDir, checkFunc := initIDMappedChecker(t, testUIDMaps, testGIDMaps)
137+
srcDir, checkFunc := initIDMappedChecker(t, testUIDMaps, testGIDMaps, true)
133138
destDir := t.TempDir()
134139
defer func() {
135140
require.NoError(t, UnmountAll(destDir, 0))
136141
}()
137142

138143
err = IDMapMount(srcDir, destDir, int(usernsFD.Fd()))
139-
usernsFD.Close()
140144
require.NoError(t, err)
141145
checkFunc(destDir)
142146
}
143147

144-
func initIDMappedChecker(t *testing.T, uidMaps, gidMaps []syscall.SysProcIDMap) (_srcDir string, _verifyFunc func(destDir string)) {
148+
func testIDMapMountWithAttrs(t *testing.T) {
149+
usernsFD, err := getUsernsFD(testUIDMaps, testGIDMaps)
150+
require.NoError(t, err)
151+
defer usernsFD.Close()
152+
153+
type testCase struct {
154+
name string
155+
srcDir string
156+
setAttr uint64
157+
checkFunc func(destDir string)
158+
}
159+
160+
cases := make([]testCase, 0)
161+
srcDir, checkFunc := initIDMappedChecker(t, testUIDMaps, testGIDMaps, true)
162+
cases = append(cases, testCase{
163+
name: "Writable idmapped mount",
164+
srcDir: srcDir,
165+
setAttr: 0,
166+
checkFunc: checkFunc,
167+
})
168+
169+
srcDir, checkFunc = initIDMappedChecker(t, testUIDMaps, testGIDMaps, false)
170+
cases = append(cases, testCase{
171+
name: "Readonly idmapped mount",
172+
srcDir: srcDir,
173+
setAttr: unix.MOUNT_ATTR_RDONLY,
174+
checkFunc: checkFunc,
175+
})
176+
177+
for _, tc := range cases {
178+
t.Run(tc.name, func(t *testing.T) {
179+
destDir := t.TempDir()
180+
defer func() {
181+
require.NoError(t, UnmountAll(destDir, 0))
182+
}()
183+
err := IDMapMountWithAttrs(tc.srcDir, destDir, int(usernsFD.Fd()), tc.setAttr, 0)
184+
require.NoError(t, err)
185+
tc.checkFunc(destDir)
186+
})
187+
}
188+
}
189+
190+
func initIDMappedChecker(t *testing.T, uidMaps, gidMaps []syscall.SysProcIDMap, expectWritable bool) (_srcDir string, _verifyFunc func(destDir string)) {
145191
testutil.RequiresRoot(t)
146192

147193
srcDir := t.TempDir()
@@ -159,6 +205,11 @@ func initIDMappedChecker(t *testing.T, uidMaps, gidMaps []syscall.SysProcIDMap)
159205
require.NoError(t, err, fmt.Sprintf("chown %v:%v for file %s", uid, gid, file))
160206
}
161207

208+
writableDir := filepath.Join(srcDir, "write-test")
209+
require.NoError(t, os.Mkdir(writableDir, os.ModePerm))
210+
require.NoError(t, os.Chmod(writableDir, os.ModePerm))
211+
require.NoError(t, os.Chown(writableDir, uidMaps[0].ContainerID, gidMaps[0].ContainerID))
212+
162213
return srcDir, func(destDir string) {
163214
for idx := range uidMaps {
164215
file := filepath.Join(destDir, fmt.Sprintf("%v", idx))
@@ -177,5 +228,18 @@ func initIDMappedChecker(t *testing.T, uidMaps, gidMaps []syscall.SysProcIDMap)
177228
require.Equal(t, uint32(gid), sysStat.Gid, fmt.Sprintf("check file %s gid", file))
178229
t.Logf("IDMapped File %s uid=%v, gid=%v", file, uid, gid)
179230
}
231+
232+
wf, err := os.Create(filepath.Join(destDir, "write-test", "1"))
233+
if err == nil {
234+
defer wf.Close()
235+
}
236+
if expectWritable {
237+
require.NoError(t, err, "create write-test file")
238+
} else {
239+
require.Error(t, err)
240+
pathErr, isPathErr := err.(*fs.PathError)
241+
require.True(t, isPathErr, "Expecting path error")
242+
require.Equal(t, unix.EROFS, pathErr.Err, "Expecting read-only filesystem error")
243+
}
180244
}
181245
}

‎core/mount/mount_linux.go‎

Lines changed: 15 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,12 @@ func prepareIDMappedOverlay(usernsFd int, options []string) ([]string, func(), e
6666
return options, nil, fmt.Errorf("failed to parse overlay lowerdir's from given options")
6767
}
6868

69-
tmpLowerdirs, idMapCleanUp, err := doPrepareIDMappedOverlay(lowerDirs, usernsFd)
69+
tempRemountsLocation, err := os.MkdirTemp(tempMountLocation, "ovl-idmapped")
70+
if err != nil {
71+
return options, nil, fmt.Errorf("failed to create temporary overlay lowerdir mount location: %w", err)
72+
}
73+
74+
tmpLowerdirs, idMapCleanUp, err := doPrepareIDMappedOverlay(tempRemountsLocation, lowerDirs, usernsFd)
7075
if err != nil {
7176
return options, idMapCleanUp, fmt.Errorf("failed to create idmapped mount: %w", err)
7277
}
@@ -244,39 +249,35 @@ func getUnprivilegedMountFlags(path string) (int, error) {
244249
return flags, nil
245250
}
246251

247-
func doPrepareIDMappedOverlay(lowerDirs []string, usernsFd int) (tmpLowerDirs []string, _ func(), _ error) {
248-
td, err := os.MkdirTemp(tempMountLocation, "ovl-idmapped")
249-
if err != nil {
250-
return nil, nil, err
251-
}
252+
func doPrepareIDMappedOverlay(tempRemountsLocation string, lowerDirs []string, usernsFd int) ([]string, func(), error) {
253+
tmpLowerDirs := make([]string, 0, len(lowerDirs))
254+
252255
cleanUp := func() {
253256
for _, lowerDir := range tmpLowerDirs {
254-
// Do a detached unmount so even if the resource is busy, the mount will be
255-
// gone (eventually) and we can safely delete the directory too.
256-
if err := unix.Unmount(lowerDir, unix.MNT_DETACH); err != nil {
257+
if err := unix.Unmount(lowerDir, 0); err != nil {
257258
log.L.WithError(err).Warnf("failed to unmount temp lowerdir %s", lowerDir)
258259
continue
259260
}
260261
// Using os.Remove() so if it's not empty, we don't delete files in the
261262
// rootfs.
262263
if err := os.Remove(lowerDir); err != nil {
263-
log.L.WithError(err).Warnf("failed to remove temporary overlay lowerdir's")
264+
log.L.WithError(err).Warnf("failed to remove temporary overlay lowerdir")
264265
}
265266
}
266267

267268
// This dir should be empty now. Otherwise, we don't do anything.
268-
if err := os.Remove(filepath.Join(tmpLowerDirs[0], "..")); err != nil {
269+
if err := os.Remove(tempRemountsLocation); err != nil {
269270
log.L.WithError(err).Infof("failed to remove temporary overlay dir")
270271
}
271272
}
272273
for i, lowerDir := range lowerDirs {
273-
tmpLowerDir := filepath.Join(td, strconv.Itoa(i))
274+
tmpLowerDir := filepath.Join(tempRemountsLocation, strconv.Itoa(i))
274275
tmpLowerDirs = append(tmpLowerDirs, tmpLowerDir)
275276

276-
if err = os.MkdirAll(tmpLowerDir, 0700); err != nil {
277+
if err := os.MkdirAll(tmpLowerDir, 0700); err != nil {
277278
return nil, cleanUp, fmt.Errorf("failed to create temporary dir: %w", err)
278279
}
279-
if err = IDMapMount(lowerDir, tmpLowerDir, usernsFd); err != nil {
280+
if err := IDMapMountWithAttrs(lowerDir, tmpLowerDir, usernsFd, unix.MOUNT_ATTR_RDONLY, 0); err != nil {
280281
return nil, cleanUp, err
281282
}
282283
}

‎core/mount/mount_linux_test.go‎

Lines changed: 152 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,12 +18,18 @@ package mount
1818

1919
import (
2020
"fmt"
21+
"io/fs"
2122
"os"
2223
"os/exec"
2324
"path/filepath"
2425
"reflect"
26+
"syscall"
2527
"testing"
2628

29+
kernel "github.com/containerd/containerd/v2/pkg/kernelversion"
30+
"github.com/stretchr/testify/assert"
31+
"github.com/stretchr/testify/require"
32+
2733
"github.com/containerd/continuity/testutil"
2834
"golang.org/x/sys/unix"
2935
)
@@ -198,6 +204,109 @@ func TestUnmountRecursive(t *testing.T) {
198204
}
199205
}
200206

207+
func TestDoPrepareIDMappedOverlay(t *testing.T) {
208+
testutil.RequiresRoot(t)
209+
210+
k512 := kernel.KernelVersion{Kernel: 5, Major: 12}
211+
ok, err := kernel.GreaterEqualThan(k512)
212+
require.NoError(t, err)
213+
if !ok {
214+
t.Skip("GetUsernsFD requires kernel >= 5.12")
215+
}
216+
217+
usernsFD, err := getUsernsFD(testUIDMaps, testGIDMaps)
218+
require.NoError(t, err)
219+
defer usernsFD.Close()
220+
221+
type testCase struct {
222+
name string
223+
injectUmountFault bool
224+
}
225+
226+
tcases := []testCase{
227+
{
228+
name: "normal",
229+
injectUmountFault: false,
230+
},
231+
{
232+
name: "umount-fault",
233+
injectUmountFault: true,
234+
},
235+
}
236+
237+
for _, tc := range tcases {
238+
t.Run(tc.name, func(t *testing.T) {
239+
fakeLowerDirsDir := t.TempDir()
240+
if !supportsIDMap(fakeLowerDirsDir) {
241+
t.Skip("IDmapped mounts not supported on filesystem selected by t.TempDir()")
242+
}
243+
244+
lowerDirs := []string{filepath.Join(fakeLowerDirsDir, "lower1"), filepath.Join(fakeLowerDirsDir, "lower2")}
245+
for _, dir := range lowerDirs {
246+
require.NoError(t, os.Mkdir(dir, 0755))
247+
require.NoError(t, os.WriteFile(filepath.Join(dir, filepath.Base(dir)), []byte("foo"), 0644))
248+
}
249+
250+
remountsLocation := t.TempDir()
251+
252+
tmpLowerDirs, cleanup, err := doPrepareIDMappedOverlay(remountsLocation, lowerDirs, int(usernsFD.Fd()))
253+
require.NoError(t, err)
254+
require.Len(t, tmpLowerDirs, len(lowerDirs))
255+
256+
lowerContents := make([][]byte, len(lowerDirs))
257+
258+
for i, dir := range lowerDirs {
259+
correspondingRemount := tmpLowerDirs[i]
260+
filename := filepath.Base(dir)
261+
262+
expectedFile, err := os.ReadFile(filepath.Join(dir, filename))
263+
require.NoError(t, err, "reading comparison test fixture file")
264+
lowerContents[i] = expectedFile
265+
266+
actualFile, err := os.ReadFile(filepath.Join(correspondingRemount, filename))
267+
require.NoError(t, err, "reading file in temporary remount")
268+
269+
assert.Equal(t, expectedFile, actualFile, "file content in temporary remount")
270+
}
271+
272+
var busyDh *os.File
273+
if tc.injectUmountFault {
274+
busyDh, err = os.Open(tmpLowerDirs[0])
275+
require.NoError(t, err)
276+
defer busyDh.Close()
277+
}
278+
279+
cleanup()
280+
281+
_, err = os.Stat(remountsLocation)
282+
283+
if tc.injectUmountFault {
284+
// We should have failed to remove the remounts location if the unmount failed.
285+
assert.NoError(t, err, "expected remounts location to still exist after unmount failure")
286+
} else {
287+
pathErr, isPathErr := err.(*fs.PathError)
288+
require.True(t, isPathErr, "expected a PathError")
289+
assert.Equal(t, unix.ENOENT, pathErr.Err, "temporary remounts should be cleaned up")
290+
}
291+
292+
// Original lowerdirs should be unaffected.
293+
for i, dir := range lowerDirs {
294+
filename := filepath.Base(dir)
295+
296+
actualFile, err := os.ReadFile(filepath.Join(dir, filename))
297+
require.NoError(t, err, "reading file in original lowerdir")
298+
assert.Equal(t, lowerContents[i], actualFile, "file content in original lowerdir")
299+
}
300+
301+
// If we blocked cleanup, allow it now so the test stays tidy.
302+
if tc.injectUmountFault {
303+
require.NoError(t, busyDh.Close())
304+
cleanup()
305+
}
306+
})
307+
}
308+
}
309+
201310
func setupMounts(t *testing.T) (target string, mounts []Mount) {
202311
dir1 := t.TempDir()
203312
dir2 := t.TempDir()
@@ -243,3 +352,46 @@ func setupMounts(t *testing.T) (target string, mounts []Mount) {
243352

244353
return target, mounts
245354
}
355+
356+
func supportsIDMap(path string) bool {
357+
treeFD, err := unix.OpenTree(-1, path, uint(unix.OPEN_TREE_CLONE|unix.OPEN_TREE_CLOEXEC))
358+
if err != nil {
359+
return false
360+
}
361+
defer unix.Close(treeFD)
362+
363+
// We want to test if idmap mounts are supported.
364+
// So we use just some random mapping, it doesn't really matter which one.
365+
// For the helper command, we just need something that is alive while we
366+
// test this, a sleep 5 will do it.
367+
cmd := exec.Command("sleep", "5")
368+
cmd.SysProcAttr = &syscall.SysProcAttr{
369+
Cloneflags: syscall.CLONE_NEWUSER,
370+
UidMappings: []syscall.SysProcIDMap{{ContainerID: 0, HostID: 65536, Size: 65536}},
371+
GidMappings: []syscall.SysProcIDMap{{ContainerID: 0, HostID: 65536, Size: 65536}},
372+
}
373+
if err := cmd.Start(); err != nil {
374+
return false
375+
}
376+
defer func() {
377+
_ = cmd.Process.Kill()
378+
_ = cmd.Wait()
379+
}()
380+
381+
usernsFD := fmt.Sprintf("/proc/%d/ns/user", cmd.Process.Pid)
382+
var usernsFile *os.File
383+
if usernsFile, err = os.Open(usernsFD); err != nil {
384+
return false
385+
}
386+
defer usernsFile.Close()
387+
388+
attr := unix.MountAttr{
389+
Attr_set: unix.MOUNT_ATTR_IDMAP,
390+
Userns_fd: uint64(usernsFile.Fd()),
391+
}
392+
if err := unix.MountSetattr(treeFD, "", unix.AT_EMPTY_PATH, &attr); err != nil {
393+
return false
394+
}
395+
396+
return true
397+
}

0 commit comments

Comments
 (0)