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
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 10 additions & 0 deletions node/core/pvf/common/src/worker/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Copy link
Contributor

Choose a reason for hiding this comment

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

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" {
Copy link
Contributor

Choose a reason for hiding this comment

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

If key would be OsStr then this comparision should also be adjusted.

std::env::remove_var(key);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

One more thing: should we remove PATH or maybe it is better to set it to some standard value?

}

// 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
Original file line number Diff line number Diff line change
Expand Up @@ -125,3 +125,10 @@ A basic security mechanism is to make sure that any thread directly interfacing
with untrusted code does not have access to the file-system. This provides some
protection against attackers accessing sensitive data or modifying data on the
host machine.

### Clearing env vars

We clear environment variables before handling untrusted code, because why give
attackers potentially sensitive data unnecessarily? And even if everything else
is locked down, env vars can potentially provide a source of randomness (see
point 1, "Consensus faults" above).