This repository has been archived by the owner on Nov 15, 2023. It is now read-only.
PVF worker: Prevent access to env vars #7330
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mrcnski
added
A0-please_review
bkchr
reviewed
Jun 4, 2023
node/core/pvf/common/src/worker.rs
Outdated
Comment on lines
104
to
107
| // Delete env vars to prevent malicious code from accessing them. | ||
| for (key, _) in std::env::vars() { | ||
| std::env::remove_var(key); | ||
| } |
There was a problem hiding this comment.
We should just not inherit any env variables when starting the worker: https://doc.rust-lang.org/std/process/struct.Command.html#method.env_clear
(But we should still forward RUST_LOG)
There was a problem hiding this comment.
Nice, I didn't think of RUST_LOG. There's still an env var present with this method:
Jun 04 16:34:24.259 WARN parachain::pvf-prepare-worker: Unexpected env var found. key="__CF_USER_TEXT_ENCODING"
Not a blocker, just weird. Your method is still better.
There was a problem hiding this comment.
Hmm I don't know what that env var is but maybe we should still clear it on the child-side as it can be a source of randomness for attackers. 🤷♂️
There was a problem hiding this comment.
__CF_USER_TEXT_ENCODING this is some macos env variable, not sure why it still appears..
|
I think it's required:Mac: What's ~/.CFUserTextEncoding for?superuser.comI don't know, I would clear the env vars child-side to make sure we get everything. Sent from my iPhoneOn 4 Jun 2023, at 17:00, Bastian Köcher ***@***.***> wrote:
@bkchr commented on this pull request.
In node/core/pvf/common/src/worker.rs:
+ // Delete env vars to prevent malicious code from accessing them.
+ for (key, _) in std::env::vars() {
+ std::env::remove_var(key);
+ }
…__CF_USER_TEXT_ENCODING this is some macos env variable, not sure why it still appears..
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were assigned.Message ID: ***@***.***>
|
Clearing env vars with the `std::process::Command` API didn't get everything on Mac, namely `__CF_USER_TEXT_ENCODING` was still present. While we don't support Mac itself as a secure system, the same issue could exist on some Linux systems either now or in the future. So it is better to just clear it on the child-side and not worry about it. We may not use the `Command` API in the future, anyway: https://github.com/paritytech/polkadot/issues/4721
alindima
approved these changes
Aug 21, 2023
tdimitrov
approved these changes
Aug 21, 2023
vstakhov
reviewed
Aug 21, 2023
| @@ -128,6 +128,16 @@ pub fn worker_event_loop<F, Fut>( | |||
| } | |||
| } | |||
|
|
|||
| // Delete all env vars to prevent malicious code from accessing them. | |||
| for (key, _) in std::env::vars() { | |||
There was a problem hiding this comment.
Suggested change
| for (key, _) in std::env::vars() { | |
| for (key, _) in std::env::vars_os() { |
It should be safer to use vars_os iterator as vars may panic on invalid UTF8.
| // TODO: *theoretically* the value (or mere presence) of `RUST_LOG` can be a source of | ||
| // randomness for malicious code. In the future we can remove it also and log in the host; | ||
| // see <https://github.com/paritytech/polkadot/issues/7117>. | ||
| if key != "RUST_LOG" { |
There was a problem hiding this comment.
If key would be OsStr then this comparision should also be adjusted.
| // see <https://github.com/paritytech/polkadot/issues/7117>. | ||
| if key != "RUST_LOG" { | ||
| std::env::remove_var(key); | ||
| } |
There was a problem hiding this comment.
One more thing: should we remove PATH or maybe it is better to set it to some standard value?
mrcnski
added a commit
that referenced
this pull request
Aug 21, 2023
- Fixes possible panic due to non-UTF-8 env vars (#7330 (comment)) - Very small refactor of some duplicated code
paritytech-processbot bot
pushed a commit
that referenced
this pull request
Aug 21, 2023
* PVF worker: random fixes - Fixes possible panic due to non-UTF-8 env vars (#7330 (comment)) - Very small refactor of some duplicated code * Don't need `to_str()` for comparison between OsString and str * Check edge cases that can cause env::remove_var to panic In case of a key or value that would cause env::remove_var to panic, we first log a warning and then proceed to attempt to remove the env var. * Make warning message clearer for end users * Backslash was unescaped, but can just remove it from error messages
1 task
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Removes any env vars accessible from the spawned worker process.
TODO
Related
Closes #7326.