Prevent .tsh/environment values from overloading prior set values#34277
Prevent .tsh/environment values from overloading prior set values#34277
.tsh/environment values from overloading prior set values#34277Conversation
There was a problem hiding this comment.
Are environment variables case sensitive?
There was a problem hiding this comment.
They are in linux and macOS, but they are not in windows.
There was a problem hiding this comment.
Aren't we breaking windows by doing a case insensitive comparison here then? foo != FOO != Foo but since we are converting to upercase all three of those keys would be equivalent and only the first one defined would be allowed.
There was a problem hiding this comment.
Actually this check is mostly to protect Windows, arguably we could be breaking Linux / MacOS. In Windows regardless of the casing used for lookup you would find a value. However in Linux and MacOS the lookup must match the casing exactly. So, if some tooling needs to use the key Foo and FOO as two separate and distinct values, then we would not allow that distinction.
That said, I feel like this condition is extremely corner case. And I think it's reasonable for Teleport to generally not allow any duplication regardless of casing. But if necessary we can put OS specific logic in to handle the way different operating systems consider environment key conflicts.
There was a problem hiding this comment.
Isn't this a breaking change then? What if my pam config defines foo and FOO?
There was a problem hiding this comment.
I would be extremely surprised if people are passing different values through a variation in casing. For that reason, I feel that considering duplicates generally to be case insensitive is fine for simplicity of code and generally being defensive. However if we need to do OS specific logic I can add that.
There was a problem hiding this comment.
I think it would be least disruptive if we could do the following:
- Still allow both
fooandFooin the PAM config, maintaining existing behavior. - Prevent overriding any variation of an existing variable in
.tsh/environment. This means ifFoois already set, then any variation (foo,FoO,FOO, etc) of that name in.tsh/environmentis ignored.
Seem reasonble?
There was a problem hiding this comment.
Sounds good to me, I made this change in dbc5896
jentfoo
force-pushed
the
jent/env_overload_protection
branch
from
81fd410 to
968c0d0
Compare
It's not possible to have duplicate environment values within an environment. And in fact the last value in the string slice will be preserved. Prior to this change that allows users to possibly change any environment values through the use of the `.tsh/environment` file. This is within user level control, where other environment value sources originate from a more protected location (for example the PAM configuration). Prior to this change that allows users to possibly change any environment passed configuration through the use of the `.tsh/environment` file. This change makes it so that the administrative set values will be preferred, and any duplicate records will be ignored.
This change updates `SafeEnv` to be allow the caller to select if the value should be checked for duplicates. We then leverage this to avoid this check when sourced from a trusted source. But then exclude potential duplicates when sourced from .tsh/environment file or the local environment.
jentfoo
force-pushed
the
jent/env_overload_protection
branch
from
ef81e33 to
b495731
Compare
rosstimothy
left a comment
rosstimothy
left a comment
There was a problem hiding this comment.
Thanks for this @jentfoo!
public-teleport-github-review-bot
Bot
removed request for
codingllama,
jimbishopp,
r0mant and
reedloden
3 participants
It's not possible to have duplicate environment values within an environment. And in fact the last value in the string slice will be preserved. Prior to this change that allows users to possibly change any environment values through the use of the
.tsh/environmentfile. This is a user level control, where other environment value originate from a more protected location (for example the PAM configuration).I believe this user level override is not expected, and may be dangerous if users can control where temporary files are written or other system level administration attributes.
This PR makes it so that the administrative set values will be preferred, and any duplicate records will be ignored.
This is a follow up from #34177. We pulled this change into a separate PR so that we can discuss if this behavior change can be safely backported to our current major versions. I can not conceive of a valid use case of this behavior, and I believe that our customers would likely be concerned that a user could influence the system level configuration set by an administrator.
However I have not been able to find a specific exploit case where this manipulation could lead to privilege escalation or other more serious impacts.
changelog: Environment values can not be overridden from the
.tsh/environmentfile, only unique keys will be inserted into the environment.