Add a file for storing elevated-only state #11222

zadjii-msft · 2021-09-14T10:56:23Z

Summary of the Pull Request

This creates an elevated-state.json that lives in %LOCALAPPDATA% next to state.json, that's only writable when elevated. It doesn't use this file for anything, it just puts the framework down for use later.

It's just like ApplicationState. We'll use it the same way.

It's readable when unelevated, which is nice, but not writable. If you're dumb and try to write to the file when unelevated, it'll just silently do nothing.

If we try opening the file and find out the permissions are different, we'll blow the file away entirely. This is to prevent someone from renaming the original file (which they can do unelevated), then slapping a new file that's writable by them down in it's place.

References

We're going to use this in Prompt before launching commandlines when elevated ; cache answers in elevated state.json #11096, but these PRs need to be broken up.

PR Checklist

Closes nothing
I work here
Tests added/passed
Requires documentation to be updated - maybe? not sure we have docs on state.json at all yet

Validation Steps Performed

I've played with this much more in dev/migrie/f/non-terminal-content-elevation-warning

followed by #11308, #11310

…arning for specifically ElevatedState (cherry picked from commit 97b0f06)

(cherry picked from commit 00c7647)

(cherry picked from commit 306ad30)

carlos-zamora

LGTM. @lhecker may want to take a look

src/cascadia/TerminalSettingsModel/ElevatedState.h

src/cascadia/TerminalSettingsModel/ElevatedState.cpp

src/cascadia/TerminalApp/AppLogic.cpp

src/cascadia/TerminalSettingsModel/ApplicationState.cpp

lhecker · 2021-09-15T14:00:16Z

src/cascadia/TerminalApp/AppLogic.cpp

+            [this, settingsPath](wil::FolderChangeEvent, PCWSTR fileModified) {
+                static const std::filesystem::path statePath{ std::wstring_view{ ApplicationState::SharedInstance().FilePath() } };
+                static const std::filesystem::path elevatedStatePath{ std::wstring_view{ ElevatedState::SharedInstance().FilePath() } };


I personally prefer the previous code, which didn't call .filename() repeatedly in the closure. The .filename() function could possibly be a very heavy operation after all (for instance by checking the file system in future STL versions)...
However we can shorten the existing code by adding some helper lambdas:

static constexpr auto intoPath = [](const std::wstring_view& path) -> std::filesystem::path { return path; }; static constexpr auto intoFilename = [](const std::wstring_view& path) -> std::filesystem::path { return intoPath(path).filename(); };

Afterwards we can shorten the capture group down to a single character:

void AppLogic::_RegisterSettingsChange() { // ... lambdas here ... const auto settingsPath = intoPath(CascadiaSettings::SettingsPath()); const auto settingsBasename = settingsPath.filename(); const auto stateBasename = intoFilename(ApplicationState::SharedInstance().FilePath()); const auto elevatedStateBasename = intoFilename(ElevatedState::SharedInstance().FilePath()); _reader.create( // ... [=](wil::FolderChangeEvent, PCWSTR fileModified) { // ... }); }

See, I tried just capturing them all, but the compiler gave me some error about having too much stuff being captured or something. I forget the error exactly. It was unhappy with me capturing the fourth thing. Hence moving the stuff inside the lambda itself.

wistd::function only allows holding 12 pointers and a std::filesystem::path is 4 pointers large.
I wonder what's so bad about std::function that it wasn't used here. I guess the reason is that they wanted something that's noexcept constructible... (but why?) Certainly very annoying now though.

In that case however you could still write this (outside of the closure - it's automatically captured):

// wistd::function only stores closures as large as 12 pointers and each path is 4 pointers large. // --> They need to be static, otherwise the code doesn't compile. static const auto settingsBasename = settingsPath.filename(); static const auto stateBasename = intoFilename(ApplicationState::SharedInstance().FilePath()); static const auto elevatedStateBasename = intoFilename(ElevatedState::SharedInstance().FilePath());

BTW I wonder if we're doing it all wrong either way (if we're thinking outside of the scope of this PR)...
Shouldn't the the caller - in this case the AppLogic class - decide which file to load and not CascadiaSettings? I guess LoadAll should have a path string argument. That way we could safely hardcode the correct filenames into AppLogic.cpp.

src/cascadia/TerminalSettingsModel/FileUtils.cpp

lhecker · 2021-09-15T14:14:29Z

src/cascadia/TerminalSettingsModel/FileUtils.cpp

+            if (!hadExpectedPermissions)
+            {
+                // delete the file. It's been compromised.
+                LOG_LAST_ERROR_IF(!DeleteFile(path.c_str()));


I don't think we should delete a user's files.
Refusing to load it should be enough, shouldn't it?

This is a good point for discussion - I'm not sure if we just ignore reading the file, then try writing it with the correct permissions set, if it'll overwrite the permissions. I don't think it will. Either way, we're going to end up blowing away the contents currently in the file with whatever the first state we add to the file is.

Please keep the deletion. Even if it's not strictly necessary at this point in time, better to harden further when possible. Deleting the file removes any chance of anything bad happening because if the deletion fails, then the user has got much bigger problems with their Windows host than simply what's happening with Terminal.

Yeah -- i'm also of the mind that state is not the user's property. It just happens to live in the user's house. They can change it, own it, do whatever they want... but if Terminal ignores it or deletes it: Oh well.

src/cascadia/TerminalSettingsModel/FileUtils.cpp

…vated-state-2

* [x] delete all state, open terminal elevated - generated profiles go to the correct place, approved commandlines go to the elevated one. * [x] delete some dynamic profiles, open the terminal - they don't resurrect in either IL * [ ] GAH saving a window layout unelevated, then trying to save one elevated will blow away the unelevated one

This fixes the issue I had in the last commit. It's a little weird, but gets rid of the muckiness of layering. Things that are local to one elevation level won't pollute the other, and we don't need to worry about layering or where they came from. Just write shared state to `state.json`, and window state to `elevated-state`/`user-state`

…vated-state-2

…ptor

…vated-state-2

src/cascadia/TerminalSettingsModel/FileUtils.cpp

DHowett · 2021-11-10T20:35:54Z

src/cascadia/TerminalSettingsModel/ApplicationState.h

+    // If a property is Shared, then it'll be stored in `state.json`, and used
+    // in both elevated and unelevated instances of the Terminal. If a property
+    // is marked Local, then it will have separate values for elevated and
+    // unelevated instances.
+    enum FileSource : int
+    {
+        Shared = 0x1,
+        Local = 0x2
+    };
+    DEFINE_ENUM_FLAG_OPERATORS(FileSource);


Any interest in doing this? @lhecker this something you would require?

DHowett · 2021-11-10T20:41:54Z

src/cascadia/TerminalSettingsModel/FileUtils.cpp

+            if (!hadExpectedPermissions)
+            {
+                // delete the file. It's been compromised.
+                LOG_LAST_ERROR_IF(!DeleteFile(path.c_str()));


Yeah -- i'm also of the mind that state is not the user's property. It just happens to live in the user's house. They can change it, own it, do whatever they want... but if Terminal ignores it or deletes it: Oh well.

src/cascadia/TerminalSettingsModel/FileUtils.cpp

src/cascadia/TerminalSettingsModel/ApplicationState.h

src/cascadia/TerminalSettingsModel/ApplicationState.cpp

eryksun · 2021-11-11T01:37:28Z

src/cascadia/TerminalSettingsModel/FileUtils.cpp

+        wil::unique_hfile file{ CreateFileW(path.c_str(),
+                                            GENERIC_WRITE,
+                                            FILE_SHARE_READ | FILE_SHARE_DELETE,
+                                            elevatedOnly ? &sa : nullptr,
+                                            CREATE_ALWAYS,
+                                            FILE_ATTRIBUTE_NORMAL,
+                                            nullptr) };


The existing security is retained when the file is overwritten with CREATE_ALWAYS disposition. That's okay only if the file is owned by the administrators group. To check this, maybe change _isOwnedByAdministrators() to take a file handle and call GetSecurityInfo() instead of GetNamedSecurityInfoW(). This entails also updating ReadUTF8File() to move the check after CreateFileW(). (Related: I think ReadUTF8File should be able to do the file create outside of the loop, and just reset the file pointer on each pass.)

In WriteUTF8File(), if elevatedOnly is true and the file isn't owned by the administrators group, or opening fails with ERROR_ACCESS_DENIED, I suggest doing an atomic replace. Create a new file beside the existing file, with a unique name and CREATE_NEW disposition. Then rename the new file to the target name.

An alternative to an atomic replace would be to rewrite the security of the existing file, but that's more complicated. The existing permissions may not allow opening with WRITE_OWNER and WRITE_DAC access, or even GENERIC_WRITE access. SeRestorePrivilege would have to be enabled for the current thread, and the open would have to use FILE_FLAG_BACKUP_SEMANTICS. The security info to set would be OWNER_SECURITY_INFORMATION | LABEL_SECURITY_INFORMATION | DACL_SECURITY_INFORMATION | UNPROTECTED_DACL_SECURITY_INFORMATION. Setting "unprotected" DACL information allows using an empty DACL (not NULL; just empty), since the permissions will be inherited from the parent directory. This avoids having to create a custom DACL for the current user.

This problem with WriteUTF8File() should be avoided, else the write is just a waste of time since ReadUTF8File() will ignore it. Personally, I'd wrap CreateFileW() with the addition of the elevatedOnly parameter. Replace an existing file with a new empty file if the existing file isn't owned by the administrators. An administrator should be free to change anything about the file security except for the owner. There's no way to know whether to trust the file if it's not owned by the administrators group.

I've read your comments just now, but I'm not sure I understand them entirely...
First of all: Is there a security issue with the current code?

The way I understand this, we don't check the security info during writing which can cause us to create a file with the wrong security info despite elevatedOnly being true, right? But is that an issue if we delete that file during ReadUTF8File anyways? I personally don't mind this being a "waste of time", because this would be a situation that can only be created artificially, for instance by a user creating the file themselves... As such it's unlikely to occur in practice, isn't it?

Is there a security issue with the current code?

I don't think so because the user always has read access anyway. There's just not any point in writing data that will be thrown away.

this would be a situation that can only be created artificially, for instance by a user
creating the file themselves

Yes, for example it would occur if the user edits the file using an editor that deletes the original before saving. (Overwriting a file just deletes its data, alternate data streams, and extended attributes, but not its security descriptor.) This will succeed in a standard security context, since the file is in a directory that grants the user FILE_DELETE_CHILD access. The new file will be owned by the user instead of the administrators group and inherit its permissions from the parent directory.

…vated-state-2

eryksun · 2021-11-11T19:42:34Z

src/cascadia/TerminalSettingsModel/FileUtils.cpp

+            // Open the file _first_, then check if it has the right
+            // permissions. This prevents a "Time-of-check to time-of-use"
+            // vulnerability where a malicious exe could delete the file and
+            // replace it between us checking the permissions, and reading the
+            // contents. We've got a handle to the file now, which means we're


CreateFileW() and checking the owner are expensive and don't need to be in the loop. When bytesRead != fileSize, the loop can reset the file pointer via SetFilePointerEx(file.get(), zero, NULL, FILE_BEGIN) and continue.

Filed- #11753
Thanks!

(We don't expect to hit this codepath that many times in rapid succession, but we should still be wary of doing it the right way.)

miniksa

Nothing blocking really. Just a few odds and ends.

miniksa · 2021-11-12T18:21:57Z

src/cascadia/TerminalSettingsModel/ApplicationState.h

        // General getters/setters
-        winrt::hstring FilePath() const noexcept;
+        bool IsStatePath(const winrt::hstring& filename);


Is it really necessary to do winrt::hstring&? I thought it internally had some sort of reference copy magic. @lhecker do you know?

It's not necessary, but if we can save an AddRef/Release, why not?

With the reference in place, MSVC can properly inline this call into the generated WinRT classes.

miniksa · 2021-11-12T18:25:32Z

src/cascadia/TerminalSettingsModel/FileUtils.cpp

-            wil::unique_hfile file{ CreateFileW(path.c_str(), GENERIC_READ, FILE_SHARE_READ | FILE_SHARE_WRITE, nullptr, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, nullptr) };
+            wil::unique_hfile file{ CreateFileW(path.c_str(),
+                                                GENERIC_READ,
+                                                FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,


If we weren't sharing... we wouldn't need to be concerned with someone else editing it while we're reading. Why didn't we briefly take exclusive use? Does it mess up how editors work with it, I'm guessing?

Yeah exclusive reads/writes fail because editors hold shared locks already.

miniksa · 2021-11-12T18:27:55Z

src/cascadia/TerminalSettingsModel/FileUtils.cpp

+            // The required security descriptor can be created easily from the
+            // SDDL string: "S:(ML;;NW;;;HI)"
+            // (i.e. SACL:mandatory label;;no write up;;;high integrity level)
+            unsigned long cb;


Shouldn't you check something about cb?

This parameter is actually optional and can be dropped here.

miniksa · 2021-11-12T18:28:50Z

src/cascadia/TerminalSettingsModel/FileUtils.cpp

+            THROW_IF_WIN32_BOOL_FALSE(
+                ConvertStringSecurityDescriptorToSecurityDescriptor(L"S:(ML;;NW;;;HI)",
+                                                                    SDDL_REVISION_1,
+                                                                    wil::out_param_ptr<PSECURITY_DESCRIPTOR*>(sd),


First time I'm seeing wil::out_param_ptr... I presume it's better somehow than like &sd or &sd.get() or something?

I believe this is a copy-paste from the wil docs and is not required here. It's basically equivalent to:

static_cast<PSECURITY_DESCRIPTOR*>(sd.put())

This is required because wil::hlocal_* wrappers return HLOCAL pointers for put().

But PSECURITY_DESCRIPTOR is of type void* with which the return value of .addressof() (and put() - HLOCAL) is compatible with.

miniksa · 2021-11-12T18:29:25Z

src/cascadia/TerminalSettingsModel/FileUtils.cpp

-            wil::unique_hfile file{ CreateFileW(path.c_str(), GENERIC_READ, FILE_SHARE_READ | FILE_SHARE_WRITE, nullptr, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, nullptr) };
+            wil::unique_hfile file{ CreateFileW(path.c_str(),
+                                                GENERIC_READ,
+                                                FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,


Why is delete OK now?

This just broadens the situations in which we can open the file. Widest sharing perms = fewer opportunities for applications to stop us from doing something useful with the user's settings.

Most editors overwrite a file when they save, which requires write sharing. Some (vim?) apparently delete an existing file first [1]. In Windows 10+, DeleteFileW() has been updated to try a POSIX delete. If the filesystem supports it, already open files can continue accessing the original file, even though it's unlinked from the directory. (NTFS renames the deleted file to the reserved directory "\$Extend\$Deleted".) If a POSIX delete isn't supported, the file is marked as deleted, but not unlinked, and the name is inaccessible for new opens until all handles for existing opens have been closed. In either case, existing opens have to share delete access. Otherwise DeleteFileW() will fail with a sharing violation.

[1] IMO, deleting the file is wrong. It discards all of the metadata -- not only create timestamp, attributes, extended attributes, alternate data streams, but also the security descriptor, including the owner, mandatory and discretionary permissions, integrity level, security attributes, and access auditing.

miniksa · 2021-11-12T18:31:05Z

src/types/utils.cpp

+bool Utils::IsElevated()
+{
+    static bool isElevated = []() {
+        try
+        {
+            wil::unique_handle processToken{ GetCurrentProcessToken() };
+            const auto elevationType = wil::get_token_information<TOKEN_ELEVATION_TYPE>(processToken.get());
+            const auto elevationState = wil::get_token_information<TOKEN_ELEVATION>(processToken.get());
+            if (elevationType == TokenElevationTypeDefault && elevationState.TokenIsElevated)
+            {
+                // In this case, the user has UAC entirely disabled. This is sort of
+                // weird, we treat this like the user isn't an admin at all. There's no
+                // separation of powers, so the things we normally want to gate on
+                // "having special powers" doesn't apply.
+                //
+                // See GH#7754, GH#11096
+                return false;
+            }
+
+            return wil::test_token_membership(nullptr, SECURITY_NT_AUTHORITY, SECURITY_BUILTIN_DOMAIN_RID, DOMAIN_ALIAS_RID_ADMINS);
+        }
+        catch (...)
+        {
+            LOG_CAUGHT_EXCEPTION();
+            return false;
+        }
+    }();
+    return isElevated;
+}


Why here and not in til? I thought we were moving as much as we could to til and getting rid of old school utils.

Eh. I'm 50/50 on putting things into til that are just functions. Since it doesn't have a static lib, it's "annoying" (requires the forceinline/noinline specifier for OPT:REF and OPT:ICF and COMDAT folding to work)

All concerns addressed -- I don't have the time at the moment to review so i trust michael and leonard

DHowett · 2021-11-13T00:11:54Z

@msftbot make sure @DHowett-MSFT signs off on this

ghost · 2021-11-13T00:11:58Z

Hello @DHowett!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

I'll only merge this pull request if it's approved by @DHowett-MSFT

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

DHowett · 2021-11-13T00:12:47Z

This PR needs to be merged using the github UI, not the bot's automerge feature. Automerge breaks stacked PRs. @lhecker please copy the PR body into the commit body when you're done

lhecker · 2021-11-13T00:51:07Z

src/cascadia/TerminalSettingsModel/ApplicationState.h

        // General getters/setters
-        winrt::hstring FilePath() const noexcept;
+        bool IsStatePath(const winrt::hstring& filename);


With the reference in place, MSVC can properly inline this call into the generated WinRT classes.

lhecker

Some unimportant nits.

lhecker · 2021-11-13T01:14:10Z

src/cascadia/TerminalApp/AppLogic.cpp

+                // for why. ApplicationState::SharedInstance already caches it's
+                // own static ref.
+
+                const winrt::hstring modifiedBasename{ std::filesystem::path{ fileModified }.filename().c_str() };


We should use .native() more liberally on std::filesystem::paths. It returns a constant reference to the underlying wide string for free.

lhecker · 2021-11-13T01:14:42Z

src/cascadia/TerminalApp/AppLogic.cpp


                if (modifiedBasename == settingsBasename)
                {
                    _reloadSettings->Run();
                }
-                else if (modifiedBasename == stateBasename)
+                else if (ApplicationState::SharedInstance().IsStatePath(modifiedBasename))


You don't need to construct a hstring to pass it to a COM call. All std strings implicitly convert to winrt::param::hstring.

lhecker · 2021-11-13T01:17:09Z

src/cascadia/TerminalSettingsModel/FileUtils.cpp

+                    LOG_LAST_ERROR_IF(!DeleteFile(path.c_str()));
+
+                    // Exit early, because obviously there's nothing to read from the deleted file.
+                    return "";


Prefer return {} over return "". Those aren't actually the same.

ghost · 2022-02-03T18:27:06Z

🎉Windows Terminal Preview v1.13.10336.0 has been released which incorporates this pull request.:tada:

Handy links:

zadjii-msft added 3 commits September 14, 2021 05:48

This is everything from dev/migrie/f/non-terminal-content-elevation-w…

6be6972

…arning for specifically ElevatedState (cherry picked from commit 97b0f06)

🤦

c106f64

(cherry picked from commit 00c7647)

minor nits

51e0473

(cherry picked from commit 306ad30)

This comment has been minimized.

Sign in to view

zadjii-msft added 3 commits September 14, 2021 05:59

todo! in the code

da0cc7b

bunch of new allowed words

4e69a32

fix the tests

6757452

carlos-zamora previously approved these changes Sep 14, 2021

View reviewed changes

src/cascadia/TerminalSettingsModel/ElevatedState.h Outdated Show resolved Hide resolved

carlos-zamora requested a review from lhecker September 14, 2021 22:34

Rosefield reviewed Sep 15, 2021

View reviewed changes

src/cascadia/TerminalSettingsModel/ElevatedState.cpp Outdated Show resolved Hide resolved

zadjii-msft marked this pull request as draft September 15, 2021 13:30

lhecker reviewed Sep 15, 2021

View reviewed changes

src/cascadia/TerminalApp/AppLogic.cpp Outdated Show resolved Hide resolved

lhecker reviewed Sep 15, 2021

View reviewed changes

zadjii-msft added the Needs-Discussion Something that requires a team discussion before we can proceed label Sep 15, 2021

zadjii-msft mentioned this pull request Sep 16, 2021

Add shield to tab row when elevated #11224

Merged

3 tasks

zadjii-msft added 3 commits September 20, 2021 11:34

blindly remove ElevatedState

a4acdeb

Merge remote-tracking branch 'origin/main' into dev/migrie/f/just-ele…

5685063

…vated-state-2

remove baseapplicationstate and just merge it back in

9b3b9e0

zadjii-msft removed the Needs-Discussion Something that requires a team discussion before we can proceed label Sep 20, 2021

zadjii-msft added 7 commits September 22, 2021 08:27

Merge remote-tracking branch 'origin/main' into dev/migrie/f/just-ele…

507a48e

…vated-state-2

this is a crazy idea that I hate but gotta start somewhere

b1b1bef

pr nits

a6e044d

fix hot reloading for this file

eee657b

Merge remote-tracking branch 'origin/main' into dev/migrie/f/just-ele…

02e9871

…vated-state-2

This comment has been minimized.

Sign in to view

okay so I guess that's not a word

5ff9a24

zadjii-msft mentioned this pull request Sep 22, 2021

Warn before the user runs a new commandline elevated #11308

Closed

4 tasks

zadjii-msft marked this pull request as ready for review September 22, 2021 22:03

zadjii-msft added 4 commits November 9, 2021 07:47

this is the right way to initialize the unique_hlocal_security_descri…

b212871

…ptor

many comments

4f16dfb

I believe this is the rest of the comments

a93d17e

Merge remote-tracking branch 'origin/main' into dev/migrie/f/just-ele…

ce6a9c5

…vated-state-2

zadjii-msft dismissed lhecker’s stale review via ce6a9c5 November 9, 2021 14:24

This comment has been minimized.

Sign in to view

eryksun reviewed Nov 9, 2021

View reviewed changes

src/cascadia/TerminalSettingsModel/FileUtils.cpp Show resolved Hide resolved

zadjii-msft added 2 commits November 10, 2021 10:16

spell

db9cbf3

the last of it?

08cbd16

DHowett previously requested changes Nov 10, 2021

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Nov 10, 2021

DHowett reviewed Nov 10, 2021

View reviewed changes

src/cascadia/TerminalSettingsModel/ApplicationState.h Outdated Show resolved Hide resolved

src/cascadia/TerminalSettingsModel/ApplicationState.cpp Show resolved Hide resolved

src/cascadia/TerminalSettingsModel/ApplicationState.cpp Show resolved Hide resolved

eryksun reviewed Nov 11, 2021

View reviewed changes

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Nov 11, 2021

zadjii-msft added 3 commits November 11, 2021 11:55

Merge remote-tracking branch 'origin/main' into dev/migrie/f/just-ele…

999f21f

…vated-state-2

mitigate a TOCTOU

33e96e7

dustins nits

7f03d4d

eryksun reviewed Nov 11, 2021

View reviewed changes

miniksa approved these changes Nov 12, 2021

View reviewed changes

ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Nov 13, 2021

lhecker approved these changes Nov 13, 2021

View reviewed changes

lhecker merged commit c79334f into main Nov 13, 2021

lhecker deleted the dev/migrie/f/just-elevated-state-2 branch November 13, 2021 00:58

ghost removed the Needs-Second It's a PR that needs another sign-off label Nov 13, 2021

lhecker reviewed Nov 13, 2021

View reviewed changes

Add a file for storing elevated-only state #11222

Add a file for storing elevated-only state #11222

Conversation

zadjii-msft commented Sep 14, 2021 • edited Loading

Summary of the Pull Request

References

PR Checklist

Validation Steps Performed

followed by #11308, #11310

This comment has been minimized.

carlos-zamora left a comment

Choose a reason for hiding this comment

lhecker Sep 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhecker Sep 15, 2021 • edited Loading

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

This comment has been minimized.

This comment has been minimized.

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

Choose a reason for hiding this comment

miniksa left a 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

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

eryksun Nov 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett commented Nov 13, 2021

ghost commented Nov 13, 2021

DHowett commented Nov 13, 2021

Choose a reason for hiding this comment

lhecker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhecker Nov 13, 2021 • edited Loading

Choose a reason for hiding this comment

ghost commented Feb 3, 2022

zadjii-msft commented Sep 14, 2021 •

edited

Loading

lhecker Sep 15, 2021 •

edited

Loading

lhecker Sep 15, 2021 •

edited

Loading

eryksun Nov 13, 2021 •

edited

Loading

lhecker Nov 13, 2021 •

edited

Loading