The -Stream parameter now works with directories #13941

kyanha · 2020-10-30T07:17:19Z

Take 3, final version.

PR Summary

This PR allows Get-Item -Stream, Get-Content -Stream, Clear-Content -Stream, Set-Content -Stream, Add-Content -Stream, and Remove-Item -Stream to see and address alternate data streams on directories, not merely on files.

Fixes #10570. Fixes #13656.

Supersedes #13650 (squashes all intermediary commits).
Supersedes #13795 (apparently some pull I did wanted to update the workflow, and my OAUTH token doesn't have workflow scope, and I can't figure out how to fix it)

Addresses addtional comments in #13795 regarding $PSIsContainer should be false on alternate data streams on directories because ADSes cannot contain other items.

PR Context

Issue #10570 has been open for a year. NTFS supports what are called "Alternate Data Streams" on both files and directories (multiple named discrete blobs of data which are associated with a single directory entry). PowerShell currently supports enumeration of these Alternate Data Streams on files, using the '-Stream' parameter to 'Get-Item'. It also supports manipulation of these alternate data streams on files, using the '-Stream' parameter to Set-Content, Add-Content, Clear-Content, and Remove-Item.

Unfortunately, the initial implementation of PowerShell only supported alternate data streams on files, not on directories. This makes an entire facility of the OS's file system invisible, and if an administration team is relying on PowerShell it makes an attractive place for a red team to store data to exfiltrate. (This is not an invitation to destroy the capability to store alternate data streams on directories, as they are useful for many purposes. It is merely a rationale for making their existence visible through PowerShell.)

To create and see an alternate data stream on a directory, use cmd.exe to run the following commands:

> mkdir 10570demo
> cd 1057demo
> echo "This is a file." > 10570demo.txt
> echo "This is an alternate data stream on the file." > 10570demo.txt:datastream
> mkdir bug10570
> echo "This is an alternate data stream on the directory." > bug10570:datastream
> dir /r

The output is something like:

D:\10570demo>dir /r
 Volume in drive D is DATA
 Volume Serial Number is 8FD3-BD69

 Directory of D:\10570demo

09/17/2020  02:59 PM    <DIR>          .
09/17/2020  02:59 PM    <DIR>          ..
09/17/2020  02:58 PM                20 10570demo.txt
                                    50 10570demo.txt:datastream:$DATA
09/17/2020  02:59 PM    <DIR>          bug10570
                                    55 bug10570:datastream:$DATA
               1 File(s)             20 bytes
               3 Dir(s)  88,185,401,344 bytes free

To see the failure of PowerShell being able to see the stream on the file, but not the directory:

> pwsh
PS > Get-Item *


    Directory: D:\10570demo

Mode                 LastWriteTime         Length Name
----                 -------------         ------ ----
da---           9/17/2020  2:59 PM                bug10570
-a---           9/17/2020  2:58 PM             20 10570demo.txt

PS > Get-Item * -stream *

PSPath        : Microsoft.PowerShell.Core\FileSystem::D:\10570demo\10570demo.txt::$DATA
PSParentPath  : Microsoft.PowerShell.Core\FileSystem::D:\10570demo
PSChildName   : 10570demo.txt::$DATA
PSDrive       : D
PSProvider    : Microsoft.PowerShell.Core\FileSystem
PSIsContainer : False
FileName      : D:\10570demo\10570demo.txt
Stream        : :$DATA
Length        : 20

PSPath        : Microsoft.PowerShell.Core\FileSystem::D:\10570demo\10570demo.txt:datastream
PSParentPath  : Microsoft.PowerShell.Core\FileSystem::D:\10570demo
PSChildName   : 10570demo.txt:datastream
PSDrive       : D
PSProvider    : Microsoft.PowerShell.Core\FileSystem
PSIsContainer : False
FileName      : D:\10570demo\10570demo.txt
Stream        : datastream
Length        : 50

PS > Get-Item bug10570 -stream *
PS >

Writing the tests revealed that Set-Content internally calls Clear-Content, which is hardcoded to not check for streams on directories. This was raised as an issue in Issue #13656, but I decided to put that into this PR as well, because it made it easier to write the tests.

PR Checklist

PR has a meaningful title
Summarized changes
Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
This PR is ready to merge and is not Work in Progress.
- If the PR is work in progress, please add the prefix WIP: or [ WIP ] to the beginning of the title (the WIP bot will keep its status check at Pending while the prefix is present) and remove the prefix when the PR is ready.
Breaking changes
- I'm assuming bucket 3: current behavior of Get-Item -stream when confronted with a directory was to silently do nothing, and this PR continues to do so in the event a directory doesn't have any alternate data streams. I would like someone to doublecheck, though.
User-facing changes
- Documentation needed
  - Issue filed: Get-Item -stream now works on directories. (PS #13941) MicrosoftDocs/PowerShell-Docs#6648
Testing - New and feature
- Make sure you've added a new test if existing tests do not effectively test the code changed
Tooling
- I have considered the user experience from a tooling perspective and don't believe tooling will be impacted.

kyanha · 2020-10-30T07:33:27Z

Note: the Powershell-CI-static-analysis tests are failing because powershellgallery.com is down and the CI system cannot install Pester. (See PowerShell/PowerShellGallery#135)

kyanha

Adding a few notes for the reviewers.

kyanha · 2020-10-30T10:54:01Z

src/System.Management.Automation/namespaces/FileSystemProvider.cs

@@ -1311,35 +1311,34 @@ protected override void GetItem(string path)
                    // If we want to retrieve the file streams, retrieve them.
                    if (retrieveStreams)
                    {
-                        if (!isContainer)


Removed isContainer check, because streams can be retrieved from containers as well.

kyanha · 2020-10-30T10:57:11Z

src/System.Management.Automation/namespaces/FileSystemProvider.cs

@@ -6666,7 +6665,14 @@ public IContentReader GetContentReader(string path)

            try
            {
-                if (Directory.Exists(path))
+                // Get-Content will write a non-terminating error if the target is a directory.


Added a few comments here, per Ilya in the V1 PR. Just needed to make sure to explain why the streamName check was here in a !UNIX block.

src/System.Management.Automation/namespaces/FileSystemProvider.cs

kyanha · 2020-10-30T11:01:48Z

src/System.Management.Automation/namespaces/FileSystemProvider.cs

+
+                // An unexpected error was returned, that we don't know how to interpret. The most helpful
+                // thing we can do at this point is simply throw the raw Win32 exception.
+                throw new Win32Exception(error);


I know this isn't a a friendly error message, but I'd rather get functionality in and then gussy it up than fight with it in this PR. (again, an Ilya suggestion.)

kyanha · 2020-10-30T11:02:16Z

src/System.Management.Automation/namespaces/FileSystemProvider.cs

@@ -8757,6 +8797,7 @@ internal static void SetZoneOfOrigin(string path, SecurityZone securityZone)
        internal static class NativeMethods
        {
            internal const int ERROR_HANDLE_EOF = 38;
+            internal const int ERROR_INVALID_PARAMETER = 87;


This is returned when a filesystem doesn't support alternate data streams.

kyanha · 2020-10-30T19:57:35Z

test/powershell/Modules/Microsoft.PowerShell.Management/Get-Item.Tests.ps1

+            $result = Get-Item $altStreamDirectory -Stream $streamName
+            $result.Length | Should -Be ($stringData.Length + [Environment]::NewLine.Length )
+            $result.Stream | Should -Be $streamName
+            $result.PSIsContainer | Should -BeExactly $false


This particular line is to ensure that alternate data streams on containers are not themselves reported as being containers.

kyanha · 2020-10-30T20:19:08Z

Closing and reopening to rerun CI.

kyanha · 2020-10-30T20:19:18Z

Reopening to rerun CI.

kyanha · 2020-11-06T05:03:07Z

@n3rdopolis Tagging you to test the compatibility, particularly that alternate data streams on directories should not report as containers, if you would like to do so.

n3rdopolis · 2020-11-06T20:04:22Z

sweet, that attribute looks good now!

kyanha · 2020-11-07T18:16:34Z

Also, I request that this be merged, even with the CodeFactor analysis failing. I touched none of the functions that CodeFactor is complaining about, and for me to fix them in this PR would muddy the intent and scope of the actual changes.

src/System.Management.Automation/namespaces/FileSystemProvider.cs

test/powershell/Modules/Microsoft.PowerShell.Management/Add-Content.Tests.ps1

kyanha · 2020-11-11T08:53:01Z

Current CI failures appear to be completely unrelated to this PR.

n3rdopolis · 2020-12-03T03:02:58Z

Are they going to merge this? it worked for me

rjmholt · 2020-12-03T16:48:15Z

Are they going to merge this? it worked for me

We need reviews to merge a PR.

Are you saying you've run this branch and it works? Would you be able to contribute a review of the PR as well?

rjmholt

This LGTM, with the suggestion from earlier

kyanha · 2020-12-03T20:19:56Z

There. @rjmholt I think I got all the suggestions addressed (including properly hanging the if block, naming the parameter, and all the newlines).

kyanha · 2020-12-03T20:58:05Z

PowerShell-CI-static-analysis is failing because someone on the docs team removed
https://docs.microsoft.com/dotnet/csharp/programming-guide/xmldoc/xml-documentation-comments, as specified by the Azure Pipeline.

PowerShell-CI-static-analysis (Markdown and Common Tests) is failing for a reason I can't discern, but I assume it's related to the prior issue.

404 - Content Not Found

kyanha · 2020-12-07T07:55:28Z

@rjmholt This is still saying merging is blocked, even after your approval. What else needs to be done?

iSazonov · 2020-12-08T03:43:55Z

@kyanha Thanks for your contribution!

ghost · 2020-12-15T21:37:47Z

🎉v7.2.0-preview.2 has been released which incorporates this pull request.:tada:

Handy links:

Release Notes

The -Stream parameter now works with directories

2751030

kyanha requested a review from anmenaga as a code owner October 30, 2020 07:17

ghost assigned rjmholt Oct 30, 2020

kyanha mentioned this pull request Oct 30, 2020

WIP: Windows: Alternate Data Steams on directories are now accessible. #13795

Closed

9 tasks

kyanha commented Oct 30, 2020

View reviewed changes

kyanha closed this Oct 30, 2020

kyanha reopened this Oct 30, 2020

rjmholt requested review from SteveL-MSFT and JamesWTruher October 30, 2020 22:13

kyanha closed this Oct 31, 2020

kyanha reopened this Oct 31, 2020

kyanha mentioned this pull request Oct 31, 2020

Get-Item -stream now works on directories. (PS #13941) MicrosoftDocs/PowerShell-Docs#6648

Closed

23 tasks

iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Oct 31, 2020

rjmholt added the Review - Needed The PR is being reviewed label Nov 10, 2020

rjmholt reviewed Nov 10, 2020

View reviewed changes

kyanha force-pushed the issue10570v3 branch from 05ec186 to 2751030 Compare November 10, 2020 21:06

Properly hang the if block.

86b8a6b

kyanha requested a review from rjmholt November 11, 2020 08:53

ghost removed the Review - Needed The PR is being reviewed label Dec 3, 2020

rjmholt approved these changes Dec 3, 2020

View reviewed changes

Applying suggestions that github's interface didn't

d2e58ce

kyanha force-pushed the issue10570v3 branch from 9d6a9e4 to d2e58ce Compare December 3, 2020 20:17

rjmholt merged commit 909ca89 into PowerShell:master Dec 8, 2020

iSazonov added this to the 7.2.0-preview.2 milestone Dec 8, 2020

This was referenced Dec 15, 2020

Get-Content, Set-Content, and Clear-Content should work with datastreams set on directories #13656

Closed

Get-Item P:\ath\To\A\Directory -Streams * does not work #10570

Closed

The -Stream parameter now works with directories #13941

The -Stream parameter now works with directories #13941

Uh oh!

Conversation

kyanha commented Oct 30, 2020

PR Summary

PR Context

PR Checklist

Uh oh!

kyanha commented Oct 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kyanha left a comment

Choose a reason for hiding this comment

Uh oh!

kyanha Oct 30, 2020

Choose a reason for hiding this comment

Uh oh!

kyanha Oct 30, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kyanha Oct 30, 2020

Choose a reason for hiding this comment

Uh oh!

kyanha Oct 30, 2020

Choose a reason for hiding this comment

Uh oh!

kyanha Oct 30, 2020

Choose a reason for hiding this comment

Uh oh!

kyanha commented Oct 30, 2020

Uh oh!

kyanha commented Oct 30, 2020

Uh oh!

kyanha commented Nov 6, 2020

Uh oh!

n3rdopolis commented Nov 6, 2020

Uh oh!

kyanha commented Nov 7, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kyanha commented Nov 11, 2020

Uh oh!

n3rdopolis commented Dec 3, 2020

Uh oh!

rjmholt commented Dec 3, 2020

Uh oh!

rjmholt left a comment

Choose a reason for hiding this comment

Uh oh!

kyanha commented Dec 3, 2020

Uh oh!

kyanha commented Dec 3, 2020 • edited by unfurl-links bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kyanha commented Dec 7, 2020

Uh oh!

iSazonov commented Dec 8, 2020

Uh oh!

ghost commented Dec 15, 2020

Uh oh!

Uh oh!

kyanha commented Oct 30, 2020 •

edited

Loading

kyanha commented Dec 3, 2020 •

edited by unfurl-links bot

Loading