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

Filter dangerous environment variables before reexec #34177

Merged
merged 4 commits into from
Nov 6, 2023

Conversation

jentfoo
Copy link
Contributor

@jentfoo jentfoo commented Nov 2, 2023

This PR fixes https://github.com/gravitational/teleport-private/issues/1056 by filtering potentially dangerous environment variables that could result in code execution.

This seemed safest to integrate as a new struct within the environment.go in utils. This struct allows us to validate variables as they are built, making sure that regardless of the source we have validated them against our filter list.

The environment specific logic was significant enough that the current and new logic was refactored into a new package envutils.

changelog: A medium severity security fix now prevents LD_PRELOAD and other dangerous environment variables from being forwarded during re-exec

@jentfoo
Copy link
Contributor Author

jentfoo commented Nov 2, 2023

I will investigate the unit test failure tomorrow. It's not failing locally, but it seems to fail reliably in CI.

@jentfoo jentfoo force-pushed the jent/exec_env_filter branch 3 times, most recently from 90436e1 to 2af79b8 Compare November 3, 2023 20:35
@jentfoo
Copy link
Contributor Author

jentfoo commented Nov 3, 2023

@r0mant and @rosstimothy, can you please re-review this fix? After finding failures in CI I had to make some additional changes. They are encompassed in the second commit on this PR

lib/utils/envutils/environment.go Outdated Show resolved Hide resolved
lib/utils/envutils/environment.go Show resolved Hide resolved
lib/utils/envutils/environment_test.go Outdated Show resolved Hide resolved
lib/utils/envutils/environment_test.go Outdated Show resolved Hide resolved
lib/utils/envutils/environment_test.go Outdated Show resolved Hide resolved
lib/utils/envutils/environment_test.go Outdated Show resolved Hide resolved
@rosstimothy rosstimothy self-requested a review November 6, 2023 14:00
This change filters potentially dangerous environment variables that could result in code execution.

This seemed safest to integrate as a new struct within the `environment.go` in utils.  This struct allows us to validate variables as they are built, making sure that regardless of the source we have validated them against our filter list.

The environment specific logic was significant enough that the current and new logic was refactored into a new package `envutils`.
In addition this commit adds in a check to look for duplicate keys which may be attempting to overload our set values.
@jentfoo jentfoo force-pushed the jent/exec_env_filter branch 2 times, most recently from 1c05137 to a44d9d0 Compare November 6, 2023 17:13
@jentfoo
Copy link
Contributor Author

jentfoo commented Nov 6, 2023

I have removed the duplicate variable handling after discussing with @rosstimothy. There is still a security risk around this behavior, but he has convinced me to follow it up with a second PR to make sure we can land this one soon.

Copy link
Contributor

@rosstimothy rosstimothy left a comment

Choose a reason for hiding this comment

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

Thanks @jentfoo!

lib/utils/envutils/environment.go Show resolved Hide resolved
// create a temp file with an environment in it
f, err := os.CreateTemp(t.TempDir(), "teleport-environment-")
require.NoError(t, err)
defer os.Remove(f.Name())
Copy link
Contributor

Choose a reason for hiding this comment

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

Note this isn't strictly necessary since the entire directory is removed at the conclusion of the test

lib/utils/envutils/environment.go Outdated Show resolved Hide resolved
lib/utils/envutils/environment_test.go Show resolved Hide resolved
@public-teleport-github-review-bot public-teleport-github-review-bot bot removed the request for review from zmb3 November 6, 2023 21:51
@jentfoo jentfoo added this pull request to the merge queue Nov 6, 2023
Merged via the queue into master with commit a9055bc Nov 6, 2023
32 checks passed
@jentfoo jentfoo deleted the jent/exec_env_filter branch November 6, 2023 22:43
@public-teleport-github-review-bot

@jentfoo See the table below for backport results.

Branch Result
branch/v12 Failed
branch/v13 Failed
branch/v14 Create PR

jentfoo added a commit that referenced this pull request Nov 6, 2023
* Filter dangerous environment variables before reexec

This change filters potentially dangerous environment variables that could result in code execution.

This seemed safest to integrate as a new struct within the `environment.go` in utils.  This struct allows us to validate variables as they are built, making sure that regardless of the source we have validated them against our filter list.

The environment specific logic was significant enough that the current and new logic was refactored into a new package `envutils`.

* Allow the easy addition of execution environment into SafeEnv

In addition this commit adds in a check to look for duplicate keys which may be attempting to overload our set values.

* Apply PR Feedback and remove env duplicate handling

* Apply additional PR feedback
jentfoo added a commit that referenced this pull request Nov 6, 2023
* Filter dangerous environment variables before reexec

This change filters potentially dangerous environment variables that could result in code execution.

This seemed safest to integrate as a new struct within the `environment.go` in utils.  This struct allows us to validate variables as they are built, making sure that regardless of the source we have validated them against our filter list.

The environment specific logic was significant enough that the current and new logic was refactored into a new package `envutils`.

* Allow the easy addition of execution environment into SafeEnv

In addition this commit adds in a check to look for duplicate keys which may be attempting to overload our set values.

* Apply PR Feedback and remove env duplicate handling

* Apply additional PR feedback
github-merge-queue bot pushed a commit that referenced this pull request Nov 7, 2023
* Filter dangerous environment variables before reexec

This change filters potentially dangerous environment variables that could result in code execution.

This seemed safest to integrate as a new struct within the `environment.go` in utils.  This struct allows us to validate variables as they are built, making sure that regardless of the source we have validated them against our filter list.

The environment specific logic was significant enough that the current and new logic was refactored into a new package `envutils`.

* Allow the easy addition of execution environment into SafeEnv

In addition this commit adds in a check to look for duplicate keys which may be attempting to overload our set values.

* Apply PR Feedback and remove env duplicate handling

* Apply additional PR feedback
github-merge-queue bot pushed a commit that referenced this pull request Nov 7, 2023
* Filter dangerous environment variables before reexec

This change filters potentially dangerous environment variables that could result in code execution.

This seemed safest to integrate as a new struct within the `environment.go` in utils.  This struct allows us to validate variables as they are built, making sure that regardless of the source we have validated them against our filter list.

The environment specific logic was significant enough that the current and new logic was refactored into a new package `envutils`.

* Allow the easy addition of execution environment into SafeEnv

In addition this commit adds in a check to look for duplicate keys which may be attempting to overload our set values.

* Apply PR Feedback and remove env duplicate handling

* Apply additional PR feedback
zmb3 added a commit that referenced this pull request Feb 9, 2024
This test writes sample data to a temporary file and then tries to
parse it. In #34177 we disallowed reading the environment file from
a symlink, but the Go utilities we use to create temp files end up
using symlinks on macOS.

Fix this by breaking out the core functionality such that it only
requires an io.Reader instead of an os.File.
github-merge-queue bot pushed a commit that referenced this pull request Feb 14, 2024
* Fix TestReadEnvironmentFile on macOS

This test writes sample data to a temporary file and then tries to
parse it. In #34177 we disallowed reading the environment file from
a symlink, but the Go utilities we use to create temp files end up
using symlinks on macOS.

Fix this by breaking out the core functionality such that it only
requires an io.Reader instead of an os.File.

* Use consistent language in log output

Co-authored-by: rosstimothy <39066650+rosstimothy@users.noreply.github.com>

---------

Co-authored-by: rosstimothy <39066650+rosstimothy@users.noreply.github.com>
github-actions bot pushed a commit that referenced this pull request Feb 14, 2024
This test writes sample data to a temporary file and then tries to
parse it. In #34177 we disallowed reading the environment file from
a symlink, but the Go utilities we use to create temp files end up
using symlinks on macOS.

Fix this by breaking out the core functionality such that it only
requires an io.Reader instead of an os.File.
github-actions bot pushed a commit that referenced this pull request Feb 14, 2024
This test writes sample data to a temporary file and then tries to
parse it. In #34177 we disallowed reading the environment file from
a symlink, but the Go utilities we use to create temp files end up
using symlinks on macOS.

Fix this by breaking out the core functionality such that it only
requires an io.Reader instead of an os.File.
github-actions bot pushed a commit that referenced this pull request Feb 14, 2024
This test writes sample data to a temporary file and then tries to
parse it. In #34177 we disallowed reading the environment file from
a symlink, but the Go utilities we use to create temp files end up
using symlinks on macOS.

Fix this by breaking out the core functionality such that it only
requires an io.Reader instead of an os.File.
github-merge-queue bot pushed a commit that referenced this pull request Feb 15, 2024
* Fix TestReadEnvironmentFile on macOS

This test writes sample data to a temporary file and then tries to
parse it. In #34177 we disallowed reading the environment file from
a symlink, but the Go utilities we use to create temp files end up
using symlinks on macOS.

Fix this by breaking out the core functionality such that it only
requires an io.Reader instead of an os.File.

* Use consistent language in log output

Co-authored-by: rosstimothy <39066650+rosstimothy@users.noreply.github.com>

---------

Co-authored-by: rosstimothy <39066650+rosstimothy@users.noreply.github.com>
github-merge-queue bot pushed a commit that referenced this pull request Feb 15, 2024
* Fix TestReadEnvironmentFile on macOS

This test writes sample data to a temporary file and then tries to
parse it. In #34177 we disallowed reading the environment file from
a symlink, but the Go utilities we use to create temp files end up
using symlinks on macOS.

Fix this by breaking out the core functionality such that it only
requires an io.Reader instead of an os.File.

* Use consistent language in log output

Co-authored-by: rosstimothy <39066650+rosstimothy@users.noreply.github.com>

---------

Co-authored-by: rosstimothy <39066650+rosstimothy@users.noreply.github.com>
github-merge-queue bot pushed a commit that referenced this pull request Feb 15, 2024
* Fix TestReadEnvironmentFile on macOS

This test writes sample data to a temporary file and then tries to
parse it. In #34177 we disallowed reading the environment file from
a symlink, but the Go utilities we use to create temp files end up
using symlinks on macOS.

Fix this by breaking out the core functionality such that it only
requires an io.Reader instead of an os.File.

* Use consistent language in log output

Co-authored-by: rosstimothy <39066650+rosstimothy@users.noreply.github.com>

---------

Co-authored-by: rosstimothy <39066650+rosstimothy@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants