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

Build with Go 1.19.3 #32135

Merged
merged 1 commit into from Nov 2, 2022
Merged

Build with Go 1.19.3 #32135

merged 1 commit into from Nov 2, 2022

Conversation

apparentlymart
Copy link
Member

@apparentlymart apparentlymart commented Nov 1, 2022

This includes a small selection of security-related fixes which do not urgently impact Terraform's behavior but do close some potential avenues for unbounded resource usage or misbehavior with malicious input.

I've included some details below on each of the security-related fixes included in these updates (both Go 1.19.2 and Go 1.19.3) along with justifications for why they are not urgent issues in the context of Terraform, and so can therefore be released naturally in the next patch release rather than in an out-of-order security release.

Unbounded memory usage for crafted tar archive headers

golang/go#54853

Terraform accepts tar archives over HTTP as one of the many possible module source types. In principle an attacker could publish a module package in a tar archive with a crafted header and consume significant memory on the system where Terraform is running.

The impact of this is limited because it would require a Terraform configuration to already be depending on an untrustworthy module source referring to a tar archive. Most consumption of third-party modules is either indirect via the public Terraform Registry (which wraps GitHub) or directly from Git repositories, neither of which cause Terraform to retrieve and extract arbitrary tar archives that can be controlled by an attacker.

Unbounded memory usage for crafted patterns to Regular Expression functions

golang/go#55949

Terraform's built-in functions regex, regexall, and replace all rely on the Go standard library regular expression compiler for their work. In principle an attacker could publish a module which includes a crafted regular expression pattern which consumes 40,000 times more memory than the source input length, thereby consuming significant memory on the system where Terraform is running.

Terraform only evaluates functions at points where various other kinds of system access are already possible, and so this is a new example of an existing possible attack vector: Terraform modules under evaluation are effectively arbitrary code, and there are other existing ways for an untrusted module to take undesirable actions and so users should already be taking care to review any third-party modules they depend on.

However, it is true that those reviewing modules are unlikely to consider a regular expression pattern as a possible attack vector and so patching this will avoid that surprising possibility in future versions of Terraform.

Smuggling of Environment Variables on Windows systems

golang/go#56284

This bug in the Go standard library in principle allows making a child process of the Terraform process interpret its environment variable table differently than the Terraform process would have interpreted it. Terraform launches child processes for provider plugins, version control clients like git, and for any configured credentials helper.

There are no known situations where it would be a productive attack vector to "smuggle" an environment variable to a child process in this way, because Terraform CLI does not perform any screening of the environment passed to child processes and so it would be equivalent to just set the same environment variable directly, without exploiting this bug.


Because none of these problems seem to be significant issues for Terraform usage in particular, I propose that we record them in the changelog only as a series of normal bug fix entries, and not call them out under an explicit security header as we might do for a more significant advisory:

  • When installing remote module packages delivered in tar format, Terraform now limits the tar header block size to 1MiB to avoid unbounded memory usage for maliciously-crafted module packages. [Build with Go 1.19.3 #32135]
  • Terraform will now reject excessively-complex regular expression patterns passed to the regex, regexall, and replace functions, to avoid unbounded memory usage for maliciously-crafted patterns. This change should not affect any reasonable patterns intended for practical use. [Build with Go 1.19.3 #32135]
  • Terraform on Windows now rejects invalid environment variables whose values contain the NUL character when propagating environment variables to a child process such as a provider plugin. Previously Terraform would incorrectly treat that character as a separator between two separate environment variables. [Build with Go 1.19.3 #32135]

This includes a small selection of security-related fixes which do not
urgently impact Terraform's behavior but do close some potential avenues
for unbounded resource usage or misbehavior with malicious input:

 - golang/go#54853
 - golang/go#55949
 - golang/go#56284
@apparentlymart apparentlymart added bug cli 1.3-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged labels Nov 1, 2022
@apparentlymart apparentlymart requested a review from a team November 1, 2022 22:14
@apparentlymart apparentlymart self-assigned this Nov 1, 2022
@apparentlymart
Copy link
Member Author

This PR seems to have exposed a bug in the goimports consistency check script, which is running against any changed file whose name includes the characters .go. Of course the intention here was to match only files which have the suffix .go, but the grep pattern doesn't have any end-of-string anchoring and so it incorrectly matches .go-version and tries to parse it as a Go source file.

That consistency check failure is therefore just noise, and not something I intend to fix as part of this PR. Hopefully we will fix it in another follow-up PR later, although given how infrequently we change the .go-version file that doesn't strike me as particularly urgent. 🤷‍♂️

@github-actions
Copy link

github-actions bot commented Nov 2, 2022

Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch.

@github-actions
Copy link

github-actions bot commented Dec 3, 2022

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
1.3-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged bug cli
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants