diff --git a/.github/actions/spelling/allow/allow.txt b/.github/actions/spelling/allow/allow.txt index e7ffdca52..eb9b2e277 100644 --- a/.github/actions/spelling/allow/allow.txt +++ b/.github/actions/spelling/allow/allow.txt @@ -1,6 +1,7 @@ apc calt ccmp +cybersecurity Apc clickable clig diff --git a/.github/actions/spelling/expect/web.txt b/.github/actions/spelling/expect/web.txt index f50d31a6e..3072b0075 100644 --- a/.github/actions/spelling/expect/web.txt +++ b/.github/actions/spelling/expect/web.txt @@ -11,6 +11,7 @@ leonerd fixterms winui appshellintegration +mdtauk cppreference gfycat Guake diff --git a/build/Helix/OutputTestResults.ps1 b/build/Helix/OutputTestResults.ps1 index 19aa8441b..eee4c5bae 100644 --- a/build/Helix/OutputTestResults.ps1 +++ b/build/Helix/OutputTestResults.ps1 @@ -21,7 +21,7 @@ Write-Host "Checking test results..." $queryUri = GetQueryTestRunsUri -CollectionUri $CollectionUri -TeamProject $TeamProject -BuildUri $BuildUri -IncludeRunDetails Write-Host "queryUri = $queryUri" -$testRuns = Invoke-RestMethod -Uri $queryUri -Method Get -Headers $azureDevOpsRestApiHeaders +$testRuns = Invoke-RestMethodWithRetries $queryUri -Headers $azureDevOpsRestApiHeaders [System.Collections.Generic.List[string]]$failingTests = @() [System.Collections.Generic.List[string]]$unreliableTests = @() [System.Collections.Generic.List[string]]$unexpectedResultTest = @() @@ -50,7 +50,7 @@ foreach ($testRun in ($testRuns.value | Sort-Object -Property "completedDate" -D $totalTestsExecutedCount += $testRun.totalTests $testRunResultsUri = "$($testRun.url)/results?api-version=5.0" - $testResults = Invoke-RestMethod -Uri "$($testRun.url)/results?api-version=5.0" -Method Get -Headers $azureDevOpsRestApiHeaders + $testResults = Invoke-RestMethodWithRetries "$($testRun.url)/results?api-version=5.0" -Headers $azureDevOpsRestApiHeaders foreach ($testResult in $testResults.value) { diff --git a/build/Helix/ProcessHelixFiles.ps1 b/build/Helix/ProcessHelixFiles.ps1 index f74187219..dcd3608a5 100644 --- a/build/Helix/ProcessHelixFiles.ps1 +++ b/build/Helix/ProcessHelixFiles.ps1 @@ -20,13 +20,31 @@ function Generate-File-Links Out-File -FilePath $helixLinkFile -Append -InputObject "
Accessibility | ++ +The set of changes proposed here are not expected to introduce any new +accessibility issues. Users can already create elevated Terminal windows. Making +it easier to create these windows doesn't really change our accessibility story. + + | +
Security | +
+
+We won't be doing anything especially unique, so there aren't expected to be any
+substantial security risks associated with these changes. Users can already
+create elevated Terminal windows, so we're not really introducing any new
+functionality, from a security perspective.
+
+We're relying on the inherent security of the `runas` verb of `ShellExecute` to
+prevent any sort of unexpected escalation-of-privilege.
+
+ + +One security concern is the fact that the `settings.json` file is currently a +totally unsecured file. It's completely writable by any medium-IL process. That +means it's totally possible for a malicious program to change the file. The +malicious program could find a user's "Elevated PowerShell" profile, and change +the commandline to `malicious.exe`. The user might then think that their +"Elevated PowerShell" will run `powershell.exe` elevated, but will actually +auto-elevate this attacker. + +If all we expose to the user is the name of the profile in the UAC dialog, then +there's no way for the user to be sure that the program that's about to be +launched is actually what they expect. + +To help mitigate this, we should _always_ pass the evaluated `commandline` as a +part of the call to `ShellExecute`. the arguments that are passed to +`ShellExecute` are visible to the user, though they need to click the "More +Details" dropdown to reveal them. + +We will need to mitigate this vulnerability regardless of adding support for the +auto-elevation of individual terminal tabs/panes. If a user is launching the +Terminal elevated (i.e. from the Win+X menu in Windows 11), then it's possible +for a malicious program to overwrite the `commandline` of their default profile. +The user may now unknowingly invoke this malicious program while thinking they +are simply launching the Terminal. + +To deal with this more broadly, we will display a dialog within the Terminal +window before creating **any** elevated terminal instance. In that dialog, we'll +display the commandline that will be executed, so the user can very easily +confirm the commandline. + +This will need to happen for all elevated terminal instances. For an elevated +Windows Terminal window, this means _all_ connections made by the Terminal. +Every time the user opens a new profile or a new commandline in a pane, we'll +need to prompt them first to confirm the commandline. This dialog within the +elevated window will also prevent an attacker from editing the `settings.json` +file while the user already has an elevated Terminal window open and hijacking a +profile. + +The dialog options will certainly be annoying to users who don't want to be +taken out of their flow to confirm the commandline that they wish to launch. +There's precedent for a similar warning being implemented by VSCode, with their +[Workspace Trust] feature. They too faced a similar backlash when the feature +first shipped. However, in light of recent global cybersecurity attacks, this is +seen as an acceptable UX degradation in the name of application trust. We don't +want to provide an avenue that's too easy to abuse. + +When the user confirms the commandline of this profile as something safe to run, +we'll add it to an elevated-only version of `state.json`. (see [#7972] for more +details). This elevated version of the file will only be accessible by the +elevated Terminal, so an attacker cannot hijack the contents of the file. This +will help mitigate the UX discomfort caused by prompting on every commandline +launched. This should mean that the discomfort is only limited to the first +elevated launch of a particular profile. Subsequent launches (without modifying +the `commandline`) will work as they always have. + +The dialog for confirming these commandlines should have a link to the docs for +"Learn more...". Transparency in the face of this dialog should +mitigate some dissatisfaction. + +The dialog will _not_ appear if the user does not have a split token - if the +user's PC does not have UAC enabled, then they're _already_ running as an +Administrator. Everything they do is elevated, so they shouldn't be prompted in +this way. + +The Settings UI should also expose a way of viewing and removing these cached +entries. This page should only be populated in the elevated version of the +Terminal. + + |
+
Reliability | ++ +No changes to our reliability are expected as a part of this change. + + | +
Compatibility | ++ +There are no serious compatibility concerns expected with this changelist. The +new `elevate` property will be unset by default, so users will heed to opt-in +to the new auto-elevating behavior. + +There is one minor concern regarding introducing the UAC shield on the window. +We're planning on using themes to configure the appearance of the shield. That +means we'll need to ship themes before the user will be able to hide the shield +again. + + | +
Performance, Power, and Efficiency | ++ +No changes to our performance are expected as a part of this change. + + | +