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.

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

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

Merged
merged 3 commits into from
Feb 13, 2023

Conversation

shoenig
Copy link
Member

@shoenig shoenig commented Feb 10, 2023

This PR enables mitigation provided by go-getter against payloads which decompress into an unbounded size. Nomad Client now exposes two new fields in 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.

Note that for the 1.4.x, 1.3.x and 1.2.x these values are left unset, meaning no
limit will be applied. Operators will need to opt-in to the mitigation if they choose
to do so.

In Nomad 1.5 these limits will start defaulting to some value (which may be a breaking
change for some users).

Targets: release/1.4.x, Backport to 1.3.x and 1.2.x

(not main, which changed all of this code and will get its own PR)

@shoenig
Copy link
Member Author

shoenig commented Feb 10, 2023

Spot check

nomad.hcl

server {
  enabled = true
}

client {
  enabled = true
  artifact {
    decompression_size_limit = "1M"
    decompression_file_count_limit = 1024
  }
}

bomb.nomad

job "bomb" {
  datacenters = ["dc1"]

  group "group" {
    task "task" {
      driver = "exec"

      artifact {
        source = "https://github.com/hashicorp/go-getter/raw/main/testdata/decompress-zip/bomb.zip"
        destination = "local/"
      }

      config {
        command = "/bin/sleep"
        args = ["infinity"]
      }

      resources {
        cpu    = 100
        memory = 256
      }
    }
  }
}

agent logs

    2023-02-10T19:15:39.292Z [ERROR] client.alloc_runner.task_runner: prestart failed: alloc_id=1ec4a313-aa89-aef2-3b18-efd60a21f6e0 task=task error="prestart hook \"artifacts\" failed: failed to download artifact \"https://github.com/hashicorp/go-getter/raw/main/testdata/decompress-zip/bomb.zip\": zip archive contains too many files: 65534 > 1024"

no count limit

client {
  enabled = true
  artifact {
    decompression_size_limit = "1M"
    decompression_file_count_limit = 0
  }
}

agent logs

    2023-02-10T19:19:33.560Z [ERROR] client.alloc_runner.task_runner: prestart failed: alloc_id=dcfa7246-9e18-d510-c1d1-d52d33528d81 task=task error="prestart hook \"artifacts\" failed: failed to download artifact \"https://github.com/hashicorp/go-getter/raw/main/testdata/decompress-zip/bomb.zip\": zip archive larger than limit: 1000000"

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.
go.mod Outdated
@@ -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

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

@shoenig shoenig marked this pull request as ready for review February 10, 2023 19:44
@shoenig shoenig requested review from tgross and picatz February 10, 2023 19:44
@shoenig shoenig added backport/1.2.x backport to 1.1.x release line backport/1.3.x backport to 1.3.x release line labels Feb 10, 2023
Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

LGTM

website/content/docs/configuration/client.mdx Outdated Show resolved Hide resolved
website/content/docs/configuration/client.mdx Show resolved Hide resolved
nomad/structs/config/artifact.go Outdated Show resolved Hide resolved
nomad/structs/config/artifact.go Outdated Show resolved Hide resolved
@shoenig shoenig changed the title artifact: protect against unbounded artifact decompression artifact: protect against unbounded artifact decompression (1.4.x) Feb 13, 2023
@shoenig shoenig merged commit 0c1bbf9 into release/1.4.x Feb 13, 2023
@shoenig shoenig deleted the 14x-go-getter-limits branch February 13, 2023 17:21
shoenig added a commit that referenced this pull request Feb 13, 2023
…16126)

* 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.

* fixup CR comments

* deps: update to go-getter 1.7.0
shoenig added a commit that referenced this pull request Feb 13, 2023
…16126)

* 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.

* fixup CR comments

* deps: update to go-getter 1.7.0
shoenig added a commit that referenced this pull request Feb 13, 2023
…16126) (#16157)

* 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.

* fixup CR comments

* deps: update to go-getter 1.7.0
shoenig added a commit that referenced this pull request Feb 13, 2023
…16126) (#16158)

* 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.

* fixup CR comments

* deps: update to go-getter 1.7.0
tgross pushed a commit that referenced this pull request Feb 14, 2023
…16126) (#16158)

* 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.

* fixup CR comments

* deps: update to go-getter 1.7.0
tgross pushed a commit that referenced this pull request Feb 14, 2023
…16126) (#16157)

* 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.

* fixup CR comments

* deps: update to go-getter 1.7.0
tgross pushed a commit that referenced this pull request Feb 14, 2023
…16126)

* 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.

* fixup CR comments

* deps: update to go-getter 1.7.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.2.x backport to 1.1.x release line backport/1.3.x backport to 1.3.x release line
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants