Skip to content
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
merged 5 commits into from
Aug 21, 2023
Merged

PVF worker: Prevent access to env vars #7330

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a041b0c
PVF worker: Prevent access to env vars
mrcnski Jun 4, 2023
7d60c2e
Clear env vars at time of spawning process
mrcnski Jun 4, 2023
e0cfab7
Move env var clearage back to child...! Exception for RUST_LOG
mrcnski Aug 6, 2023
555bc81
Merge branch 'master' into mrcnski/pvf-worker-prevent-access-to-env-vars
mrcnski Aug 6, 2023
2df9755
Update impl guide
mrcnski Aug 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions node/core/pvf/common/src/worker.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,11 @@ pub fn worker_event_loop<F, Fut>(
}
}

// Delete env vars to prevent malicious code from accessing them.
for (key, _) in std::env::vars() {
std::env::remove_var(key);
}
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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. 🤷‍♂️

Copy link
Member

Choose a reason for hiding this comment

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

__CF_USER_TEXT_ENCODING this is some macos env variable, not sure why it still appears..


// Run the main worker loop.
let rt = Runtime::new().expect("Creates tokio runtime. If this panics the worker will die and the host will detect that and deal with it.");
let err = rt
Expand Down