Merge pull request #2345 from dtrudg/issue236

fix: oci: use correct bind options for user /dev[/xxx] binds (release-4.0)

CHANGELOG.md

@@ -13,6 +13,9 @@
 - Support parentheses in `test` / `[` commands in container startup scripts,
   via dependency update of mvdan.cc/sh.
 - Fix incorrect client timeout during remote build context upload.
+- When user requests a bind of `/dev:/dev` or `/dev/xxx:/dev/xxx` in OCI-mode,
+  ensure that it is bind mounted with appropriate flags so that it is usable in
+  the container.
 
 ## 4.0.1 \[2023-10-13\]
 

internal/pkg/runtime/launcher/oci/mounts_linux.go

@@ -561,6 +561,11 @@ func (l *Launcher) addSystemBindMounts(mounts *[]specs.Mount) error {
 }
 
 func (l *Launcher) addUserBindMounts(mounts *[]specs.Mount) error {
+	if !l.singularityConf.UserBindControl {
+		sylog.Warningf("Ignoring bind mount request(s): user bind control disabled by system administrator")
+		return nil
+	}
+
 	// First get binds from -B/--bind and env var
 	binds, err := bind.ParseBindPath(strings.Join(l.cfg.BindPaths, ","))
 	if err != nil {
@@ -576,10 +581,18 @@ func (l *Launcher) addUserBindMounts(mounts *[]specs.Mount) error {
 	}
 
 	for _, b := range binds {
-		if !l.singularityConf.UserBindControl {
-			sylog.Warningf("Ignoring bind mount request: user bind control disabled by system administrator")
-			return nil
+		// Special Case - user is manually requesting all of /dev to be bound
+		if b.Source == "/dev" {
+			if b.Destination != "/dev" {
+				return fmt.Errorf("host /dev may only be bound to /dev in container")
+			}
+			// Handle as we do in --no-compat, including devpts setup.
+			if err := l.addDevBind(mounts); err != nil {
+				return fmt.Errorf("while adding mount %q: %w", b.Source, err)
+			}
+			continue
 		}
+		// Anything else
 		if err := l.addBindMount(mounts, b, l.cfg.AllowSUID); err != nil {
 			return fmt.Errorf("while adding mount %q: %w", b.Source, err)
 		}
@@ -610,6 +623,11 @@ func (l *Launcher) addCwdMount(mounts *[]specs.Mount) error {
 }
 
 func (l *Launcher) addBindMount(mounts *[]specs.Mount, b bind.Path, allowSUID bool) (err error) {
+	// If request is for a /dev/xxx device, then we handle with device specific checks and flags.
+	if strings.HasPrefix(b.Source, "/dev") {
+		return addDevBindMount(mounts, b)
+	}
+
 	if b.ID() != "" || b.ImageSrc() != "" {
 		if !l.singularityConf.UserBindControl {
 			sylog.Warningf("Ignoring image bind mount request: user bind control disabled by system administrator")

internal/pkg/runtime/launcher/oci/mounts_linux_test.go

@@ -9,6 +9,7 @@
 package oci
 
 import (
+	"fmt"
 	"os"
 	"path/filepath"
 	"reflect"
@@ -17,6 +18,7 @@ import (
 
 	"github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/sylabs/singularity/v4/internal/pkg/runtime/launcher"
+	"github.com/sylabs/singularity/v4/internal/pkg/util/user"
 	"github.com/sylabs/singularity/v4/pkg/util/bind"
 	"github.com/sylabs/singularity/v4/pkg/util/singularityconf"
 )
@@ -153,6 +155,32 @@ func Test_addBindMount(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "Device",
+			b: bind.Path{
+				Source:      "/dev/null",
+				Destination: "/dev/null",
+			},
+			userbind: true,
+			wantMounts: &[]specs.Mount{
+				{
+					Source:      "/dev/null",
+					Destination: "/dev/null",
+					Type:        "none",
+					Options:     []string{"bind", "nosuid"},
+				},
+			},
+		},
+		{
+			name: "DeviceBadDest",
+			b: bind.Path{
+				Source:      "/dev/null",
+				Destination: "/notdev/null",
+			},
+			userbind:   true,
+			wantMounts: &[]specs.Mount{},
+			wantErr:    true,
+		},
 	}
 	for _, tt := range tests {
 		for _, m := range *tt.wantMounts {
@@ -181,7 +209,8 @@ func Test_addBindMount(t *testing.T) {
 	}
 }
 
-func TestLauncher_addBindMounts(t *testing.T) {
+//nolint:maintidx
+func TestLauncher_addUserBindMounts(t *testing.T) {
 	tests := []struct {
 		name       string
 		cfg        launcher.Options
@@ -366,15 +395,74 @@ func TestLauncher_addBindMounts(t *testing.T) {
 			wantMounts: &[]specs.Mount{},
 			wantErr:    true,
 		},
+		{
+			name: "FullDev",
+			cfg: launcher.Options{
+				BindPaths: []string{"/dev"},
+			},
+			userbind: true,
+			wantMounts: &[]specs.Mount{
+				{
+					Source:      "/dev",
+					Destination: "/dev",
+					Type:        "bind",
+					Options:     []string{"nosuid", "rbind", "rprivate", "rw"},
+				},
+				{
+					Source:      "devpts",
+					Destination: "/dev/pts",
+					Type:        "devpts",
+					Options:     ptsFlags(t),
+				},
+			},
+			wantErr: false,
+		},
+		{
+			name: "FullDevBadDest",
+			cfg: launcher.Options{
+				BindPaths: []string{"/dev:/notdev"},
+			},
+			userbind:   true,
+			wantMounts: &[]specs.Mount{},
+			wantErr:    true,
+		},
+		{
+			name: "SpecificDevice",
+			cfg: launcher.Options{
+				BindPaths: []string{"/dev/null"},
+			},
+			userbind: true,
+			wantMounts: &[]specs.Mount{
+				{
+					Source:      "/dev/null",
+					Destination: "/dev/null",
+					Type:        "none",
+					Options:     []string{"bind", "nosuid"},
+				},
+			},
+			wantErr: false,
+		},
+		{
+			name: "SpecificDeviceBadDest",
+			cfg: launcher.Options{
+				BindPaths: []string{"/dev/null:/notdev/null"},
+			},
+			userbind:   true,
+			wantMounts: &[]specs.Mount{},
+			wantErr:    true,
+		},
 	}
 	for _, tt := range tests {
 		for _, m := range *tt.wantMounts {
 			sort.Strings(m.Options)
 		}
 		t.Run(tt.name, func(t *testing.T) {
 			l := &Launcher{
-				cfg:             tt.cfg,
-				singularityConf: &singularityconf.File{},
+				cfg: tt.cfg,
+				singularityConf: &singularityconf.File{
+					// Required as full `/dev` userbind test involves a devpts mount onto the mounted /dev.
+					MountDevPts: true,
+				},
 			}
 			if tt.userbind {
 				l.singularityConf.UserBindControl = true
@@ -397,6 +485,21 @@ func TestLauncher_addBindMounts(t *testing.T) {
 	}
 }
 
+// Flags for /dev/pts depend on whether we are running as root, and what the tty GID is.
+func ptsFlags(t *testing.T) []string {
+	flags := []string{"nosuid", "noexec", "newinstance", "ptmxmode=0666", "mode=0620"}
+
+	if os.Geteuid() == 0 {
+		group, err := user.GetGrNam("tty")
+		if err != nil {
+			t.Fatalf("while identifying tty gid: %v", err)
+		}
+		flags = append(flags, fmt.Sprintf("gid=%d", group.GID))
+	}
+
+	return flags
+}
+
 func TestLauncher_addLibrariesMounts(t *testing.T) {
 	tmpDir, err := os.MkdirTemp("", "add-libraries-mounts")
 	if err != nil {