fix: enforce read-only volume restrictions (#1272)

Allow `valid_volumes` entries ending in `:ro` to permit only read-only mounts, so a workflow can no longer mount writable a host path the runner only shares read-only, like a certificate store. `**:ro` allows any volume read-only. An entry without `:ro` that matches too still allows writable mounts.

A named volume allowed only through `:ro` is mounted with `nocopy`, because Docker otherwise fills an empty volume from the image even when it is mounted read-only.

Fixes: https://gitea.com/gitea/runner/issues/407
Co-authored-by: silverwind <[email protected]>
Reviewed-on: https://gitea.com/gitea/runner/pulls/1272
Reviewed-by: silverwind <[email protected]>
Co-authored-by: bircni <[email protected]>

act/container/docker_run.go

@@ -8,6 +8,7 @@ package container
 
 import (
 	"bytes"
+	"cmp"
 	"context"
 	"errors"
 	"fmt"
@@ -1178,7 +1179,10 @@ func (cr *containerReference) sanitizeConfig(ctx context.Context, config *contai
 				sanitizedBinds = append(sanitizedBinds, bind)
 				continue
 			}
-			if matcher.isValid(parsed.Source, mount.Type(parsed.Type)) {
+			if matcher.isValid(parsed.Source, mount.Type(parsed.Type), parsed.ReadOnly) {
+				if matcher.needsNoCopy(parsed.Source, mount.Type(parsed.Type)) && (parsed.Volume == nil || !parsed.Volume.NoCopy) {
+					bind += ",nocopy"
+				}
 				sanitizedBinds = append(sanitizedBinds, bind)
 			} else {
 				logger.Warnf("[%s] is not a valid volume, will be ignored", parsed.Source)
@@ -1194,7 +1198,12 @@ func (cr *containerReference) sanitizeConfig(ctx context.Context, config *contai
 	// sanitize mounts
 	sanitizedMounts := make([]mount.Mount, 0, len(hostConfig.Mounts))
 	for _, mt := range hostConfig.Mounts {
-		if matcher.isValid(mt.Source, mt.Type) {
+		readOnly := mt.ReadOnly && (mt.BindOptions == nil || !mt.BindOptions.ReadOnlyNonRecursive) // bind-recursive=writable leaves submounts writable
+		if matcher.isValid(mt.Source, mt.Type, readOnly) {
+			if matcher.needsNoCopy(mt.Source, mt.Type) {
+				mt.VolumeOptions = cmp.Or(mt.VolumeOptions, &mount.VolumeOptions{})
+				mt.VolumeOptions.NoCopy = true
+			}
 			sanitizedMounts = append(sanitizedMounts, mt)
 		} else {
 			logger.Warnf("[%s] is not a valid volume, will be ignored", mt.Source)
@@ -1240,26 +1249,37 @@ func overlayVolumes(dst, src *container.HostConfig) {
 }
 
 type validVolumeMatcher struct {
-	allowAll bool
+	allowAll         bool
-	named    []string
+	allowAllReadOnly bool
-	host     []string
+	named            []validVolumeRule
+	host             []validVolumeRule
+}
+
+type validVolumeRule struct {
+	pattern  string
+	readOnly bool
 }
 
 func newValidVolumeMatcher(ctx context.Context, validVolumes []string) validVolumeMatcher {
 	logger := common.Logger(ctx)
 	ret := validVolumeMatcher{
-		named: make([]string, 0, len(validVolumes)),
+		named: make([]validVolumeRule, 0, len(validVolumes)),
-		host:  make([]string, 0, len(validVolumes)),
+		host:  make([]validVolumeRule, 0, len(validVolumes)),
 	}
 
 	for _, v := range validVolumes {
+		v, readOnly := strings.CutSuffix(v, ":ro")
 		if v == "**" {
-			ret.allowAll = true
+			if readOnly {
+				ret.allowAllReadOnly = true
+			} else {
+				ret.allowAll = true
+			}
 			continue
 		}
 		if !isHostVolumePattern(v) {
 			if doublestar.ValidatePattern(v) {
-				ret.named = append(ret.named, v)
+				ret.named = append(ret.named, validVolumeRule{pattern: v, readOnly: readOnly})
 			} else {
 				logger.Errorf("invalid volume pattern %s", v)
 			}
@@ -1271,7 +1291,7 @@ func newValidVolumeMatcher(ctx context.Context, validVolumes []string) validVolu
 			continue
 		}
 		if doublestar.ValidatePathPattern(normalized) {
-			ret.host = append(ret.host, normalized)
+			ret.host = append(ret.host, validVolumeRule{pattern: normalized, readOnly: readOnly})
 		} else {
 			logger.Errorf("invalid volume pattern %s", normalized)
 		}
@@ -1280,33 +1300,34 @@ func newValidVolumeMatcher(ctx context.Context, validVolumes []string) validVolu
 	return ret
 }
 
-func (m validVolumeMatcher) isValid(source string, sourceType mount.Type) bool {
+func (m validVolumeMatcher) isValid(source string, sourceType mount.Type, readOnly bool) bool {
-	if m.allowAll {
+	if m.allowAll || (m.allowAllReadOnly && readOnly) {
 		return true
 	}
 	if sourceType == mount.TypeTmpfs && source == "" { // backed by memory, exposes nothing from the host, same as --tmpfs
 		return true
 	}
+	rules, match := m.named, doublestar.MatchUnvalidated
 	if isHostVolumeSource(source, sourceType) {
 		normalized, err := normalizeHostVolumePath(source)
 		if err != nil {
 			return false
 		}
-		for _, pattern := range m.host {
+		rules, match, source = m.host, doublestar.PathMatchUnvalidated, normalized
-			if doublestar.PathMatchUnvalidated(pattern, normalized) {
-				return true
-			}
-		}
-		return false
 	}
-	for _, pattern := range m.named {
+	for _, rule := range rules {
-		if doublestar.MatchUnvalidated(pattern, source) {
+		if (readOnly || !rule.readOnly) && match(rule.pattern, source) {
 			return true
 		}
 	}
 	return false
 }
 
+// needsNoCopy reports a volume only `:ro` rules allow, as dockerd fills an empty volume from the image even when mounted read-only.
+func (m validVolumeMatcher) needsNoCopy(source string, sourceType mount.Type) bool {
+	return sourceType == mount.TypeVolume && !m.isValid(source, sourceType, false)
+}
+
 func isHostVolumePattern(pattern string) bool {
 	return filepath.IsAbs(pattern) ||
 		strings.HasPrefix(pattern, "."+string(filepath.Separator)) ||

act/container/docker_run_test.go

@@ -519,10 +519,12 @@ var _ ExecutionsEnvironment = &containerReference{}
 
 func TestCheckVolumes(t *testing.T) {
 	testCases := []struct {
-		desc          string
+		desc           string
-		validVolumes  []string
+		validVolumes   []string
-		binds         []string
+		binds          []string
-		expectedBinds []string
+		expectedBinds  []string
+		mounts         []mount.Mount
+		expectedMounts []mount.Mount
 	}{
 		{
 			desc:         "match all volumes",
@@ -578,6 +580,27 @@ func TestCheckVolumes(t *testing.T) {
 				"/opt/app/cache:/cache",
 			},
 		},
+		{
+			desc:          "read-only rules allow only read-only mounts and copy nothing into volumes",
+			validVolumes:  []string{"/etc/ssl:ro", "data:ro", "shared"},
+			binds:         []string{"/etc/ssl:/ssl:ro", "/etc/ssl:/ssl", "data:/data:ro", "data:/data", "shared:/shared:ro"},
+			expectedBinds: []string{"/etc/ssl:/ssl:ro", "data:/data:ro,nocopy", "shared:/shared:ro"},
+			mounts: []mount.Mount{
+				{Type: mount.TypeBind, Source: "/etc/ssl", Target: "/ssl", ReadOnly: true},
+				{Type: mount.TypeBind, Source: "/etc/ssl", Target: "/ssl", ReadOnly: true, BindOptions: &mount.BindOptions{ReadOnlyNonRecursive: true}},
+				{Type: mount.TypeVolume, Source: "data", Target: "/data", ReadOnly: true},
+			},
+			expectedMounts: []mount.Mount{
+				{Type: mount.TypeBind, Source: "/etc/ssl", Target: "/ssl", ReadOnly: true},
+				{Type: mount.TypeVolume, Source: "data", Target: "/data", ReadOnly: true, VolumeOptions: &mount.VolumeOptions{NoCopy: true}},
+			},
+		},
+		{
+			desc:          "read-only wildcard keeps writable rules writable",
+			validVolumes:  []string{"**:ro", "shared"},
+			binds:         []string{"/etc/ssl:/ssl:ro", "data:/data", "shared:/shared"},
+			expectedBinds: []string{"/etc/ssl:/ssl:ro", "shared:/shared"},
+		},
 	}
 	for _, tc := range testCases {
 		t.Run(tc.desc, func(t *testing.T) {
@@ -588,9 +611,10 @@ func TestCheckVolumes(t *testing.T) {
 					ValidVolumes: tc.validVolumes,
 				},
 			}
-			_, hostConf := cr.sanitizeConfig(ctx, &container.Config{}, &container.HostConfig{Binds: tc.binds})
+			_, hostConf := cr.sanitizeConfig(ctx, &container.Config{}, &container.HostConfig{Binds: tc.binds, Mounts: tc.mounts})
 			assert.Equal(t, tc.expectedBinds, hostConf.Binds)
-			assert.Len(t, hook.AllEntries(), len(tc.binds)-len(tc.expectedBinds)) // every drop is warned about
+			assert.ElementsMatch(t, tc.expectedMounts, hostConf.Mounts)
+			assert.Len(t, hook.AllEntries(), len(tc.binds)+len(tc.mounts)-len(tc.expectedBinds)-len(tc.expectedMounts)) // every drop is warned about
 		})
 	}
 }
@@ -771,7 +795,7 @@ func TestSanitizeConfigKeepsTmpfsMounts(t *testing.T) {
 	hostConfig, _ := mergeOptions(t, "", "--mount type=tmpfs,dst=/var/lib/mysql", false)
 	require.Len(t, hostConfig.Mounts, 1)
 
-	for _, validVolumes := range [][]string{nil, {"data"}} {
+	for _, validVolumes := range [][]string{nil, {"data"}, {"**:ro"}} {
 		logger, hook := test.NewNullLogger()
 		_, sanitized := (&containerReference{input: &NewContainerInput{ValidVolumes: validVolumes}}).sanitizeConfig(common.WithLogger(context.Background(), logger), &container.Config{}, &container.HostConfig{Mounts: []mount.Mount{
 			hostConfig.Mounts[0],

internal/pkg/config/config.example.yaml

@@ -239,10 +239,12 @@ container:
   #workdir_parent:
   # Volumes and bind mounts containers may mount, as globs (https://github.com/bmatcuk/doublestar#patterns).
   # `*` matches within a path segment, `**` across segments. Empty allows none. tmpfs mounts are always allowed.
-  # For example, to allow the `data` volume and all json files in `/src`:
+  # A `:ro` suffix allows only read-only mounts, unless an entry without it matches too.
+  # For example, to allow the `data` volume and all json files in `/src`, and `/etc/ssl/certs` only read-only:
   # valid_volumes:
   #   - data
   #   - /src/*.json
+  #   - /etc/ssl/certs:ro
   # To allow any volume:
   # valid_volumes:
   #   - '**'