Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

artifact: protect against unbounded artifact decompression (1.4.x) #16126

Merged
merged 3 commits into from
Feb 13, 2023
Merged

artifact: protect against unbounded artifact decompression (1.4.x) #16126

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0db43aa
artifact: protect against unbounded artifact decompression
shoenig Feb 10, 2023
dc95fb9
fixup CR comments
shoenig Feb 10, 2023
baaf54e
deps: update to go-getter 1.7.0
shoenig Feb 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions .changelog/16126.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:security
artifact: Provide mitigations against unbounded artifact decompression
```
1 change: 1 addition & 0 deletions client/alloc_watcher_e2e_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()}
}
Expand Down
6 changes: 5 additions & 1 deletion client/allocrunner/taskrunner/getter/getter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand All @@ -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),
}
}

Expand Down
30 changes: 23 additions & 7 deletions client/allocrunner/taskrunner/getter/getter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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) {
Expand Down
12 changes: 8 additions & 4 deletions client/allocrunner/taskrunner/getter/testing.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
//go:build !release
// +build !release

package getter

Expand All @@ -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)
}
1 change: 1 addition & 0 deletions client/allocrunner/taskrunner/task_runner_hooks.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
12 changes: 12 additions & 0 deletions client/config/artifact.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
}

Expand Down
28 changes: 13 additions & 15 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member Author

@shoenig shoenig Feb 10, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will switch to v1.7.0 once the tag exists

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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down