env: properly handle nulls in REG_SZ strings #16190

lhecker · 2023-10-18T13:31:47Z

eb871bf fails to properly handle REG_SZ strings, which are documented as
being null-terminated and length restricted.
wcsnlen is the perfect fit for handling this situation as it returns
the position of the first \0, or the given length parameter.

As a drive by improvement, this also drops some redundant code:

to_environment_strings_w which is the same as to_string
Retrieving USERNAME/USERDOMAIN via LookupAccountSidW and
COMPUTERNAME via GetComputerNameW is not necessary as the
variables are "volatile" and I believe there's generally no
expectation that they change unless you log in again.

Closes #16051

Validation Steps Performed

Run this in PowerShell to insert a env value with \0:

$hklm = [Microsoft.Win32.RegistryKey]::OpenBaseKey(
  [Microsoft.Win32.RegistryHive]::LocalMachine,
  0
)
$key = $hklm.OpenSubKey(
  'SYSTEM\CurrentControlSet\Control\Session Manager\Environment',
  $true
)
$key.SetValue('test', "foo`0bar")

All EnvTests still pass ✅
(Don't forget to remove the above value again!)

lhecker · 2023-10-18T13:35:20Z

src/inc/til/env.h

-                    strip_trailing_null(accountName);
-                    strip_trailing_null(userDomain);


My intention was to just remove these two lines here, but then I realized that it doesn't actually seem to do anything, because HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Control\Session Manager\Environment contains a USERNAME value (with the value-value of SYSTEM) which immediately overwrites this. Reading more into volatile environment variables, I've realized that this code can be replaced with one that simply copies them from the stale (process) environment block, because they are immutable across a login.

clever. this means it is no longer a clone of RegenerateUserEnvironment, but that's OK.

KalleOlaviNiemitalo · 2023-10-18T13:51:20Z

src/inc/til/env.h

+            // The difference is that we don't
+            // * handle autoexec.bat (intentionally unsupported).
+            // * call LookupAccountSidW to get the USERNAME/USERDOMAIN variables
+            //   These are available as standard environment variables and they're "volatile"
+            //   (= computed dynamically at each login and then constant until logout).
+            //   We don't expect our process to impersonate another user so we can simplify this.
+            // * call GetComputerNameW to get the COMPUTERNAME variable, for the same reason.


This description of "volatile" seems a bit off, if it covers CLIENTNAME, which I'd expect to change if the user reconnects to the same Remote Desktop session from a different client computer.

I have assumed that the "HKEY_CURRENT_USER\Volatile Environment" key is created with RegCreateKeyExW REG_OPTION_VOLATILE or the NT API equivalent, so that it is never saved to the user profile, but I haven't verified whether this is actually true.

That's basically what I meant, but I tried writing it in a way that doesn't assume the reader knows what any of this means.

I think CLIENTNAME is a bit special in this regard, because it's not even stored in the Volatile Environment key directly, but rather in it's child key where the key name is your session ID, starting from 1. To me it seems like it is still a constant across an entire session, because every time you login or connect remotely the session ID changes and a new child key is created.

How would you express the comment?

I tried it just now: disconnected from the session without logging out, and reconnected to the same session from a different client. In this case, "Volatile Environment" still had a subkey with the same session ID as its name, but the data of the "CLIENTNAME" value was updated. And Registry Editor was still running so it had to be the same session.

How would you express the comment?

I would not try to explain what "volatile" means.

// * call LookupAccountSidW to get the USERNAME/USERDOMAIN variables // Windows sets these as values of the "Volatile Environment" key at each login. // We don't expect our process to impersonate another user so we can simplify this.

Thanks for testing this! I guess in hindsight this is kind of obvious, since RDP allows reconnects... I've never really thought about how that would work. I hope it properly emits a WM_SETTINGCHANGE event, whenever this happens, so that it still works if we don't regenerate the environment every time.

zadjii-msft

Sure let's give it a shot. I think I follow

DHowett · 2023-10-18T18:06:10Z

This is a very long change for what it purports to be. Can you point out the specific part that is the fix, and what is a drive-by?

Should we have a smaller, targeted version for 1.18 and 1.19?

lhecker · 2023-10-18T18:49:32Z

This is a very long change for what it purports to be. Can you point out the specific part that is the fix, and what is a drive-by?

The only drive-by is the removal of get_computer_name(), all the other changes are necessary. I removed get_computer_name() because its functionality was very similar to how get_user_name_and_domain() worked. Can you spot what get_user_name_and_domain() did wrong? 😄

Solution: ZFQA qbrfa'g ernyyl zragvba vg fcrpvsvpnyyl, ohg YbbxhcNppbhagFvqJ gerngf gur nppbhagAnzrFvmr naq hfreQbznvaFvmr cnenzrgref nf va-bhg barf rira vs gur gjb jpune cbvagref nera'g ahyycge. Gung vf, nsgre gur frpbaq pnyy gb YbbxhcNppbhagFvqJ, jr unq gb pnyy erfvmr() ba gur gjb jfgevat'f gb cebcreyl gevz bs genvyvat ahyyf naq fgevc_genvyvat_ahyy bayl fgevcf 1 genvyvat ahyy.

DHowett · 2023-10-18T19:07:04Z

src/cascadia/TerminalConnection/ConptyConnection.cpp

-
-        til::env environment;
-        auto zeroEnvMap = wil::scope_exit([&]() noexcept {
-            environment.clear();


Why did we used to do this, anyway?

I'm not sure! Maybe security? But that wouldn't have worked, since the creation of the many std::wstring instances during regenerate() would leave tons of "unsafely" freed memory locations around anyways.

src/cascadia/TerminalConnection/ConptyConnection.cpp

DHowett · 2023-10-18T19:08:45Z

src/inc/til/env.h

-                    strip_trailing_null(accountName);
-                    strip_trailing_null(userDomain);


clever. this means it is no longer a clone of RegenerateUserEnvironment, but that's OK.

DHowett · 2023-10-18T19:10:10Z

src/inc/til/env.h

                                    data = expand_environment_strings(data.data());
                                }

-                                // Because Registry data may or may not be null terminated... check if we've managed
-                                // to store an extra null in the wstring by telling it to create itself from pointer and size.


this seems like a willy way to handle that!

DHowett · 2023-10-18T19:12:35Z

src/til/ut_til/EnvTests.cpp

+        auto expectedIt = expected.as_map().begin();
+        auto actualIt = actual.as_map().begin();
+
+        for (; expectedIt != expectedEnd; ++expectedIt, ++actualIt)


Careful! The original check is order-independent. This check is not. It was explicitly written this way because the test was failing.

It's just test code, it's allowed to be inefficient.

Did it use an unordered_map before? Right now it uses a map and the iteration order of the two instances should match 1:1.

I made this change because I found the old code very confusing while testing my changes. This doesn't work:

const auto actualValue = actual.as_map()[expectedKey]; VERIFY_IS_TRUE(actual.as_map().find(expectedKey) != actual.as_map().end());

The first line ensures that the second line always succeeds. This code is also somewhat confusing to me:

VERIFY_ARE_EQUAL(expectedValue, actual.as_map()[expectedKey]);

because it doesn't properly work in case either of the two values is an empty string.

In short, my intention was to remove the use of operator[] to increase my confidence that I didn't break anything.

AH. Thanks.

eb871bf fails to properly handle REG_SZ strings, which are documented as being null-terminated _and_ length restricted. `wcsnlen` is the perfect fit for handling this situation as it returns the position of the first \0, or the given length parameter. As a drive by improvement, this also drops some redundant code: * `to_environment_strings_w` which is the same as `to_string` * Retrieving `USERNAME`/`USERDOMAIN` via `LookupAccountSidW` and `COMPUTERNAME` via `GetComputerNameW` is not necessary as the variables are "volatile" and I believe there's generally no expectation that they change unless you log in again. Closes #16051 ## Validation Steps Performed * Run this in PowerShell to insert a env value with \0: ```pwsh $hklm = [Microsoft.Win32.RegistryKey]::OpenBaseKey( [Microsoft.Win32.RegistryHive]::LocalMachine, 0 ) $key = $hklm.OpenSubKey( 'SYSTEM\CurrentControlSet\Control\Session Manager\Environment', $true ) $key.SetValue('test', "foo`0bar") ``` * All `EnvTests` still pass ✅ * (Don't forget to remove the above value again!) (cherry picked from commit 64b5b28) Service-Card-Id: 90879164 Service-Version: 1.19

eb871bf fails to properly handle REG_SZ strings, which are documented as being null-terminated _and_ length restricted. `wcsnlen` is the perfect fit for handling this situation as it returns the position of the first \0, or the given length parameter. As a drive by improvement, this also drops some redundant code: * `to_environment_strings_w` which is the same as `to_string` * Retrieving `USERNAME`/`USERDOMAIN` via `LookupAccountSidW` and `COMPUTERNAME` via `GetComputerNameW` is not necessary as the variables are "volatile" and I believe there's generally no expectation that they change unless you log in again. Closes #16051 ## Validation Steps Performed * Run this in PowerShell to insert a env value with \0: ```pwsh $hklm = [Microsoft.Win32.RegistryKey]::OpenBaseKey( [Microsoft.Win32.RegistryHive]::LocalMachine, 0 ) $key = $hklm.OpenSubKey( 'SYSTEM\CurrentControlSet\Control\Session Manager\Environment', $true ) $key.SetValue('test', "foo`0bar") ``` * All `EnvTests` still pass ✅ * (Don't forget to remove the above value again!) (cherry picked from commit 64b5b28) Service-Card-Id: 90879163 Service-Version: 1.18

Properly handle nulls in REG_SZ strings

23f3e41

microsoft-github-policy-service bot added Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-0 Bugs that we consider release-blocking/recall-class (P0) Product-Terminal The new Windows Terminal. labels Oct 18, 2023

lhecker commented Oct 18, 2023

View reviewed changes

KalleOlaviNiemitalo reviewed Oct 18, 2023

View reviewed changes

zadjii-msft mentioned this pull request Oct 18, 2023

v1.18.2681.0 breaks Powershell/WSL2/Commandline Shell (only tested on 23H2 (10.0.22631.2361)) (error 2147942487 (0x80070057)) #16051

Closed

Forgot to commit a file

47d361e

zadjii-msft approved these changes Oct 18, 2023

View reviewed changes

lhecker added 3 commits October 18, 2023 17:08

Fix compilation

7334832

Address feedback

f5b1a0c

This warning does not exist in VS 17.8

ca13965

DHowett requested changes Oct 18, 2023

View reviewed changes

microsoft-github-policy-service bot added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Oct 18, 2023

DHowett approved these changes Oct 19, 2023

View reviewed changes

DHowett changed the title ~~Properly handle nulls in REG_SZ strings~~ env: properly handle nulls in REG_SZ strings Oct 19, 2023

DHowett merged commit 64b5b28 into main Oct 19, 2023
17 checks passed

DHowett deleted the dev/lhecker/16051-env-nulls branch October 19, 2023 00:46

lhecker mentioned this pull request Oct 24, 2023

RunAs Administrator returning "SYSTEM" as $env:USERNAME in PowerShell #16200

Open

adonis-jimenez mentioned this pull request Nov 18, 2023

Windows Terminal Preview v1.19.3172.0 #16337

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

env: properly handle nulls in REG_SZ strings #16190

env: properly handle nulls in REG_SZ strings #16190

lhecker commented Oct 18, 2023 •

edited

Loading

lhecker Oct 18, 2023 •

edited

Loading

DHowett Oct 18, 2023

KalleOlaviNiemitalo Oct 18, 2023

lhecker Oct 18, 2023 •

edited

Loading

KalleOlaviNiemitalo Oct 18, 2023 •

edited

Loading

lhecker Oct 18, 2023

zadjii-msft left a comment

DHowett commented Oct 18, 2023

lhecker commented Oct 18, 2023

DHowett Oct 18, 2023

lhecker Oct 18, 2023

DHowett Oct 18, 2023

DHowett Oct 18, 2023

DHowett Oct 18, 2023

lhecker Oct 18, 2023

DHowett Oct 19, 2023

		strip_trailing_null(accountName);
		strip_trailing_null(userDomain);

env: properly handle nulls in REG_SZ strings #16190

env: properly handle nulls in REG_SZ strings #16190

Conversation

lhecker commented Oct 18, 2023 • edited Loading

Validation Steps Performed

lhecker Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhecker Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

KalleOlaviNiemitalo Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zadjii-msft left a comment

Choose a reason for hiding this comment

DHowett commented Oct 18, 2023

lhecker commented Oct 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhecker commented Oct 18, 2023 •

edited

Loading

lhecker Oct 18, 2023 •

edited

Loading

lhecker Oct 18, 2023 •

edited

Loading

KalleOlaviNiemitalo Oct 18, 2023 •

edited

Loading