Ability to stop child process from Inheriting Handles

[michaelvanstraten]

API Change Proposal

Problem Statement

Currently, there is no mechanism in the Rust standard library to create a child process on Windows that does not inherit handles from the calling process.

Motivating Examples or Use Cases

Handle inheritance can be problematic in multi-threaded programs, as different command-spawning actions may require passing different files to the child process. In addition, improving security by preventing the child process from acquiring certain handles is essential.

Disabling handle inheritance when unnecessary is important for several reasons:

1. Inheriting unnecessary handles is inefficient and prevents kernel objects from being cleaned up.
2. In some cases, it might allow the child process to interfere with the inherited handles, although it would need to locate them first.

Solution Sketch

To address this issue, we propose adding a new flag to the CommandExt trait in Rust's standard library. This flag will determine whether the child process should inherit handles from the calling process.

/// If this flag is set to `true`, each inheritable handle in the calling process is inherited by the new process.
/// If the flag is `false`, the handles are not inherited.
///
/// The default value for this flag is `true`.
///
/// **Note** that inherited handles have the same value and access rights as the original handles. For additional discussion of inheritable handles, see [Remarks][1].
///
/// [1]: <https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessw#remarks>
#[unstable(feature = "windows_process_extensions_inherit_handles", issue = "")]
fn inherit_handles(&mut self, inherit_handles: bool) -> &mut process::Command;

Additionally, the proposed change will affect the underlying CreateProcessW function, as shown below:

cvt(c::CreateProcessW(
    program.as_ptr(),
    cmd_str.as_mut_ptr(),
    ptr::null_mut(),
    ptr::null_mut(),
    inherit_handles,
    flags,
    envp,
    dirp,
    si_ptr,
    &mut pi,
))

Alternatives

• currently non

Links and Related Work

For further reference, please consult the following resources:

• Microsoft CreateProcessW Documentation
• Handle Inheritance in Windows
• Stack Overflow Discussion on Preventing Handle Inheritance in Rust

[ChrisDenton]

This was discussed in the libs-api meeting. It was suggested that perhaps a better API would be to give an array of handles to inherit, with an empty array automatically setting inheritance to false. It was also mentioned that however this bool is set, it does effectively override Stdio. E.g. Stdio::inherit will be ignored.

Note: A motivating use case for this option is in the pseudo console example code: Creating the Hosted Process. The pseudo gets passed differently so inheritance is unnecessary. This works because if any of stdout, stdin or stderror are set to null then a child process will be given new handles derived from the console it's attached to (assuming it's not a gui process).

[michaelvanstraten]

Sorry for the late response, I recently had a lot on my hands with the new Semester at University and a new Job.

With this plan, it still eludes me how you could both pass some handles to a process while stopping it from inheriting other.

[jmillikin]

With this plan, it still eludes me how you could both pass some handles to a process while stopping it from inheriting other.

Windows versions >= Vista support specifying a list of handles to inherit when creating a process by setting the process attribute PROC_THREAD_ATTRIBUTE_HANDLE_LIST in the extended startup info passed to CreateProcessW().

The setup code is ... not straightforward. Raymond Chen has a brief writeup on it at https://devblogs.microsoft.com/oldnewthing/20111216-00/?p=8873, with sample code.

[michaelvanstraten]

If we want to allow specifying a slice of handles to be inherited, we should first merge rust-lang/rust#123604, so the interface can be added there.

[michaelvanstraten]

This was discussed in the libs-api meeting. It was suggested that perhaps a better API would be to give an array of handles to inherit, with an empty array automatically setting inheritance to false. It was also mentioned that however this bool is set, it does effectively override Stdio. E.g. Stdio::inherit will be ignored.

Since the list of handles must be specified in the process thread attributes via the PROC_THREAD_ATTRIBUTE_HANDLE_LIST attribute, it becomes difficult—if not impossible—to determine if the attribute is populated, as there seems to be no way (at least to my knowledge) to inspect the attributes set in a ProcThreadAttributeList.

Per the Windows documentation:

"If you use this attribute, pass in a value of TRUE for the bInheritHandles parameter of the CreateProcess function."

This could lead to unexpected behavior when bInheritHandles is set to false, but the PROC_THREAD_ATTRIBUTE_HANDLE_LIST attribute is still populated, causing potential confusion or bugs.

(Sorry for the links, text fragments are not working for some reason)

[pixmaip]

Hi, as mentioned in rust-lang/rust#115501 (comment), I believe that @michaelvanstraten's suggestion is relevant independently of what was proposed during the libs-api meeting regarding having an API to inherit specific handles.

Indeed, when a program is running as PPL, it can only spawn child processes as non-protected if (and only if) the bInheritHandles parameter is set to FALSE on the CreateProcess (see the Protected Process Light (PPL) processes section).
This is a protection put in place by Windows to prevent non-protected processes to tamper with PPL processes (by basically removing the PROCESS_DUP_HANDLE access right).

This means that current Rust programs running as PPL cannot use the standard library to spawn processes.

I tested the option proposed in this post and the CreateProcess API call will still fail. The only way to make it succeed is to set the bInheritHandles parameter to FALSE.

Note that setting bInheritHandles to FALSE will automatically prevent standard handles to work properly, meaning that stdin will not be passed through to the child process, and stdout/err will not output any data. The only possible "interaction" with the child process is through its exit status.

@ChrisDenton would it be possible to re-discuss this issue with the libs-api team?

[PaulDance]

Having this re-evaluated somewhat soon would be nice, please.

[PaulDance]

@ChrisDenton bump

This is preventing us from consuming the nightly release channel in order to fix issues ahead of stable-time for both Clippy warnings and the small fork of Rust we maintain for continued Windows 7 support, because we added the necessary API directly in our fork and don't locally publish nightly builds, only stable ones. Doing differently would most probably mean locally vendoring the whole process API for a small change, which would be a bit cumbersome. The simplest for us is therefore to upstream it, as its landing in nightly is sufficient in this case.

Would it therefore be possible to re-discuss this issue, please? Thanks in advance.

[ChrisDenton]

libs-api considers a random selection of old ACPs each weeks. However, there are a lot of ACPs to get through (in addition to other matters libs-api discusses) so this simply has yet to come up again.

But I think the result of the last meeting still stands. Having an API like:

fn inherit_handles(&mut self, handles: &[HANDLE]) -> &mut Command;

Would work for your use case if an empty slice causes bInheritHandles to be set to false.

[michaelvanstraten]

As mentioned in #264 (comment), there is no official way to determine what attributes are set in a PROC_THREAD_ATTRIBUTE_LIST. This means we would need to temporarily record the handles within the Command struct and, at spawn time, create a ProcThreadAttributeList, set the handles to inherit, and pass that to [spawn_with_attributes](https://doc.rust-lang.org/nightly/std/os/windows/process/trait.CommandExt.html#tymethod.spawn_with_attributes).

Since the spawn_with_attributes method takes a shared reference to a ProcThreadAttributeList and there is no known way to copy a ProcThreadAttributeList, any attempt to set both handles and attributes would result in the attributes being ignored. Changing the shared reference to a mutable one doesn’t make much sense to me, as it is unclear why spawning a command would modify the attribute list.

I propose adding a function to set the bInheritHandles flag for the CreateProcessW call on the Command struct, as well as an additional method on the ProcThreadAttributeListBuilder to set the handles to inherit.

[ChrisDenton]

Since the spawn_with_attributes method takes a shared reference to a ProcThreadAttributeList and there is no known way to copy a ProcThreadAttributeList, any attempt to set both handles and attributes would result in the attributes being ignored.

We could just say that using spawn_with_attributes overrides inherit_handles. If it's likely to be a footgun we could even make it an error.

[michaelvanstraten]

But I think the result of the last meeting still stands. Having an API like:

fn inherit_handles(&mut self, handles: &[HANDLE]) -> &mut Command;

Does this mean we have to add a lifetime to the command struct?

[PaulDance]

We could just say that using spawn_with_attributes overrides inherit_handles. If it's likely to be a footgun we could even make it an error.

If I understand things correctly -- correct me if I'm wrong, this would mean introducing a special case in spawn_with_attributes to somehow detect specifically if PROC_THREAD_ATTRIBUTE_HANDLE_LIST is set, or introduce a dedicated state. Also, inherit_handles would need to call spawn_with_attributes's inner workings that probably need to be split from it first since it would otherwise need to spawn the process as well because ProcThreadAttributeList is only taken by reference there, thus making it a feature dependency, which is not ideal for stabilization I would say, and could probably be a bit difficult, especially considering spawn was internally made a special case of spawn_with_attributes.

Overall, it seems a lot like things would end up not orthogonal at all and probably quite entangled. Currently, there is already a way to set a list of handles to inherit using what was introduced in rust-lang/rust#123604. Having a separate API only for bInheritHandles would not change that and using it would be compatible with spawn_with_attributes. Also, Command following a builder pattern that has orthogonality at its root I would say, introducing a new setting that is not compatible with existing ones would feel weird I think.

For low-level details such as these, it seems fine to me to expose them in a relatively-direct way instead of trying to wrap them up in a more complex operation: if used, it is probably for aspects that need to be set in specific ways that a higher-level API could hinder. Also, bInheritHandles is the only concrete parameter of CreateProcessW that is not currently customizable I think.

Wouldn't you therefore agree that keeping things separate would probably be better in the end?

[michaelvanstraten]

+1 for @PaulDance comment.

[michaelvanstraten]

@ChrisDenton maybe you could re-discuss this ACP in the next meeting?