artifact: protect against unbounded artifact decompression

This PR enables mitigations provided by go-getter against payloads which decompress into an unbounded size or file count. There are two new client config options under the artifact block: artifact.decompression_size_limit (e.g. "10GB") - the maximum amount of data that will be decompressed before triggering an error and cancelling the operation artifact.decompression_file_count_limit (e.g. 1024) - the maximum number of files that will be decompressed before triggering ana error and cancelling the operation.
hashicorp · Feb 10, 2023 · 18652fd · 18652fd
1 parent 2536c6f
commit 18652fd
Show file tree

Hide file tree

Showing 11 changed files with 390 additions and 111 deletions.
diff --git a/.changelog/16126.txt b/.changelog/16126.txt
@@ -0,0 +1,3 @@
+```release-note:security
+artifact: Provide mitigations against unbounded artifact decompression
+```
diff --git a/client/alloc_watcher_e2e_test.go b/client/alloc_watcher_e2e_test.go
@@ -55,6 +55,7 @@ func TestPrevAlloc_StreamAllocDir_TLS(t *testing.T) {
 			CertFile:             clientCertFn,
 			KeyFile:              clientKeyFn,
 		}
+
 		c.Client.Enabled = true
 		c.Client.Servers = []string{server.GetConfig().RPCAddr.String()}
 	}

diff --git a/client/allocrunner/taskrunner/getter/getter.go b/client/allocrunner/taskrunner/getter/getter.go
@@ -83,6 +83,7 @@ func (g *Getter) GetArtifact(taskEnv interfaces.EnvReplacer, artifact *structs.T
 	}
 
 	headers := getHeaders(taskEnv, artifact.GetterHeaders)
+
 	if err := g.getClient(ggURL, headers, mode, dest).Get(); err != nil {
 		return newGetError(ggURL, err, true)
 	}
@@ -99,8 +100,11 @@ func (g *Getter) getClient(src string, headers http.Header, mode gg.ClientMode,
 		Umask:   060000000,
 		Getters: g.createGetters(headers),
 
-		// This will prevent copying or writing files through symlinks
+		// This will prevent copying or writing files through symlinks.
 		DisableSymlinks: true,
+
+		// This will protect against decompression bombs.
+		Decompressors: gg.LimitedDecompressors(g.config.DecompressionLimitFileCount, g.config.DecompressionLimitSize),
 	}
 }
 

diff --git a/client/allocrunner/taskrunner/getter/getter_test.go b/client/allocrunner/taskrunner/getter/getter_test.go
@@ -86,20 +86,36 @@ func (u upperReplacer) ClientPath(p string, join bool) (string, bool) {
 }
 
 func TestGetter_getClient(t *testing.T) {
+	const fileCountLimit = 555
+	const fileSizeLimit = int64(666)
 	getter := NewGetter(hclog.NewNullLogger(), &clientconfig.ArtifactConfig{
-		HTTPReadTimeout: time.Minute,
-		HTTPMaxBytes:    100_000,
-		GCSTimeout:      1 * time.Minute,
-		GitTimeout:      2 * time.Minute,
-		HgTimeout:       3 * time.Minute,
-		S3Timeout:       4 * time.Minute,
+		HTTPReadTimeout:             time.Minute,
+		HTTPMaxBytes:                100_000,
+		GCSTimeout:                  1 * time.Minute,
+		GitTimeout:                  2 * time.Minute,
+		HgTimeout:                   3 * time.Minute,
+		S3Timeout:                   4 * time.Minute,
+		DecompressionLimitFileCount: fileCountLimit,
+		DecompressionLimitSize:      fileSizeLimit,
 	})
+
 	client := getter.getClient("src", nil, gg.ClientModeAny, "dst")
 
 	t.Run("check symlink config", func(t *testing.T) {
 		require.True(t, client.DisableSymlinks)
 	})
 
+	t.Run("check file size limits", func(t *testing.T) {
+		require.Equal(t, fileSizeLimit, client.Decompressors["zip"].(*gg.ZipDecompressor).FileSizeLimit)
+		require.Equal(t, fileCountLimit, client.Decompressors["zip"].(*gg.ZipDecompressor).FilesLimit)
+
+		require.Equal(t, fileSizeLimit, client.Decompressors["tar.gz"].(*gg.TarGzipDecompressor).FileSizeLimit)
+		require.Equal(t, fileCountLimit, client.Decompressors["tar.gz"].(*gg.TarGzipDecompressor).FilesLimit)
+
+		require.Equal(t, fileSizeLimit, client.Decompressors["xz"].(*gg.XzDecompressor).FileSizeLimit)
+		// xz does not support files count limit
+	})
+
 	t.Run("check http config", func(t *testing.T) {
 		require.True(t, client.Getters["http"].(*gg.HttpGetter).XTerraformGetDisabled)
 		require.Equal(t, time.Minute, client.Getters["http"].(*gg.HttpGetter).ReadTimeout)
@@ -151,7 +167,7 @@ func TestGetArtifact_getHeaders(t *testing.T) {
 }
 
 func TestGetArtifact_Headers(t *testing.T) {
-	file := "output.txt"
+	const file = "output.txt"
 
 	// Create the test server with a handler that will validate headers are set.
 	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {

diff --git a/client/allocrunner/taskrunner/getter/testing.go b/client/allocrunner/taskrunner/getter/testing.go
@@ -1,5 +1,4 @@
 //go:build !release
-// +build !release
 
 package getter
 
@@ -8,12 +7,17 @@ import (
 
 	"github.com/hashicorp/go-hclog"
 	clientconfig "github.com/hashicorp/nomad/client/config"
+	"github.com/hashicorp/nomad/helper/pointer"
 	"github.com/hashicorp/nomad/nomad/structs/config"
-	"github.com/stretchr/testify/require"
+	"github.com/shoenig/test/must"
 )
 
+// TestDefaultGetter creates a Getter suitable for unit test cases.
 func TestDefaultGetter(t *testing.T) *Getter {
-	getterConf, err := clientconfig.ArtifactConfigFromAgent(config.DefaultArtifactConfig())
-	require.NoError(t, err)
+	defaultConfig := config.DefaultArtifactConfig()
+	defaultConfig.DecompressionSizeLimit = pointer.Of("1MB")
+	defaultConfig.DecompressionFileCountLimit = pointer.Of(10)
+	getterConf, err := clientconfig.ArtifactConfigFromAgent(defaultConfig)
+	must.NoError(t, err)
 	return NewGetter(hclog.NewNullLogger(), getterConf)
 }
diff --git a/client/allocrunner/taskrunner/task_runner_hooks.go b/client/allocrunner/taskrunner/task_runner_hooks.go
@@ -58,6 +58,7 @@ func (tr *TaskRunner) initHooks() {
 	// Create the task directory hook. This is run first to ensure the
 	// directory path exists for other hooks.
 	alloc := tr.Alloc()
+
 	tr.runnerHooks = []interfaces.TaskHook{
 		newValidateHook(tr.clientConfig, hookLogger),
 		newTaskDirHook(tr, hookLogger),

diff --git a/client/config/artifact.go b/client/config/artifact.go
@@ -18,6 +18,9 @@ type ArtifactConfig struct {
 	GitTimeout time.Duration
 	HgTimeout  time.Duration
 	S3Timeout  time.Duration
+
+	DecompressionLimitSize      int64
+	DecompressionLimitFileCount int
 }
 
 // ArtifactConfigFromAgent creates a new internal readonly copy of the client
@@ -61,6 +64,15 @@ func ArtifactConfigFromAgent(c *config.ArtifactConfig) (*ArtifactConfig, error)
 	}
 	newConfig.S3Timeout = t
 
+	s, err = humanize.ParseBytes(*c.DecompressionSizeLimit)
+	if err != nil {
+		return nil, fmt.Errorf("error parsing DecompressionLimitSize: %w", err)
+	}
+	newConfig.DecompressionLimitSize = int64(s)
+
+	// no parsing its just an int
+	newConfig.DecompressionLimitFileCount = *c.DecompressionFileCountLimit
+
 	return newConfig, nil
 }
 

diff --git a/go.mod b/go.mod
@@ -54,7 +54,7 @@ require (
 	// versions.
 	github.com/hashicorp/go-discover v0.0.0-20220621183603-a413e131e836
 	github.com/hashicorp/go-envparse v0.0.0-20180119215841-310ca1881b22
-	github.com/hashicorp/go-getter v1.6.2
+	github.com/hashicorp/go-getter v1.6.3-0.20230210143508-0edab8534827
 	github.com/hashicorp/go-hclog v1.3.1
 	github.com/hashicorp/go-immutable-radix v1.3.1
 	github.com/hashicorp/go-kms-wrapping/v2 v2.0.5
@@ -120,7 +120,7 @@ require (
 	go.uber.org/goleak v1.2.0
 	golang.org/x/crypto v0.0.0-20220622213112-05595931fe9d
 	golang.org/x/exp v0.0.0-20220921164117-439092de6870
-	golang.org/x/sync v0.0.0-20220722155255-886fb9371eb4
+	golang.org/x/sync v0.0.0-20220929204114-8fcdb60fdcc0
 	golang.org/x/sys v0.2.0
 	golang.org/x/time v0.0.0-20220224211638-0e9765cccd65
 	google.golang.org/grpc v1.51.0
@@ -131,8 +131,10 @@ require (
 )
 
 require (
-	cloud.google.com/go v0.97.0 // indirect
-	cloud.google.com/go/storage v1.18.2 // indirect
+	cloud.google.com/go v0.104.0 // indirect
+	cloud.google.com/go/compute v1.10.0 // indirect
+	cloud.google.com/go/iam v0.5.0 // indirect
+	cloud.google.com/go/storage v1.27.0 // indirect
 	github.com/Azure/azure-sdk-for-go v56.3.0+incompatible // indirect
 	github.com/Azure/go-ansiterm v0.0.0-20210617225240-d185dfc1b5a1 // indirect
 	github.com/Azure/go-autorest v14.2.0+incompatible // indirect
@@ -165,15 +167,12 @@ require (
 	github.com/boltdb/bolt v1.3.1 // indirect
 	github.com/brianvoe/gofakeit/v6 v6.19.0
 	github.com/cenkalti/backoff/v3 v3.2.2 // indirect
-	github.com/census-instrumentation/opencensus-proto v0.3.0 // indirect
 	github.com/cespare/xxhash/v2 v2.1.2 // indirect
 	github.com/checkpoint-restore/go-criu/v5 v5.3.0 // indirect
 	github.com/cheggaaa/pb/v3 v3.0.5 // indirect
 	github.com/cilium/ebpf v0.9.1 // indirect
 	github.com/circonus-labs/circonus-gometrics v2.3.1+incompatible // indirect
 	github.com/circonus-labs/circonusllhist v0.1.3 // indirect
-	github.com/cncf/udpa/go v0.0.0-20210930031921-04548b0d99d4 // indirect
-	github.com/cncf/xds/go v0.0.0-20211011173535-cb28da3451f1 // indirect
 	github.com/containerd/cgroups v1.0.3 // indirect
 	github.com/containerd/console v1.0.3 // indirect
 	github.com/containerd/containerd v1.6.6 // indirect
@@ -187,8 +186,6 @@ require (
 	github.com/docker/go-connections v0.4.0 // indirect
 	github.com/docker/go-metrics v0.0.1 // indirect
 	github.com/docker/libtrust v0.0.0-20160708172513-aabc10ec26b7 // indirect
-	github.com/envoyproxy/go-control-plane v0.10.2-0.20220325020618-49ff273808a1 // indirect
-	github.com/envoyproxy/protoc-gen-validate v0.6.2 // indirect
 	github.com/felixge/httpsnoop v1.0.1 // indirect
 	github.com/go-ole/go-ole v1.2.6 // indirect
 	github.com/godbus/dbus/v5 v5.1.0 // indirect
@@ -199,7 +196,8 @@ require (
 	github.com/google/btree v1.0.0 // indirect
 	github.com/google/go-querystring v0.0.0-20170111101155-53e6ce116135 // indirect
 	github.com/google/uuid v1.3.0 // indirect
-	github.com/googleapis/gax-go/v2 v2.1.1 // indirect
+	github.com/googleapis/enterprise-certificate-proxy v0.2.0 // indirect
+	github.com/googleapis/gax-go/v2 v2.6.0 // indirect
 	github.com/gookit/color v1.3.1 // indirect
 	github.com/gophercloud/gophercloud v0.1.0 // indirect
 	github.com/gorilla/mux v1.8.0 // indirect
@@ -220,7 +218,7 @@ require (
 	github.com/jefferai/isbadcipher v0.0.0-20190226160619-51d2077c035f // indirect
 	github.com/jmespath/go-jmespath v0.4.0 // indirect
 	github.com/joyent/triton-go v0.0.0-20190112182421-51ffac552869 // indirect
-	github.com/klauspost/compress v1.13.6 // indirect
+	github.com/klauspost/compress v1.15.11 // indirect
 	github.com/linode/linodego v0.7.1 // indirect
 	github.com/lufia/plan9stats v0.0.0-20211012122336-39d0f177ccd0 // indirect
 	github.com/mattn/go-isatty v0.0.16 // indirect
@@ -269,14 +267,14 @@ require (
 	go.uber.org/atomic v1.9.0 // indirect
 	golang.org/x/mod v0.6.0-dev.0.20220419223038-86c51ed26bb4 // indirect
 	golang.org/x/net v0.1.0 // indirect
-	golang.org/x/oauth2 v0.0.0-20220223155221-ee480838109b // indirect
+	golang.org/x/oauth2 v0.1.0 // indirect
 	golang.org/x/term v0.1.0 // indirect
 	golang.org/x/text v0.4.0 // indirect
 	golang.org/x/tools v0.1.12 // indirect
-	golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1 // indirect
-	google.golang.org/api v0.60.0 // indirect
+	golang.org/x/xerrors v0.0.0-20220907171357-04be3eba64a2 // indirect
+	google.golang.org/api v0.100.0 // indirect
 	google.golang.org/appengine v1.6.7 // indirect
-	google.golang.org/genproto v0.0.0-20220314164441-57ef72a4c106 // indirect
+	google.golang.org/genproto v0.0.0-20221025140454-527a21cfbd71 // indirect
 	gopkg.in/fsnotify.v1 v1.4.7 // indirect
 	gopkg.in/resty.v1 v1.12.0 // indirect
 	gopkg.in/square/go-jose.v2 v2.6.0 // indirect