Skip to content

The -Stream parameter now works with directories - #13941

Merged
Rob Holt (rjmholt) merged 3 commits into
PowerShell:masterfrom
kyanha:issue10570v3
Dec 8, 2020
Merged

The -Stream parameter now works with directories#13941
Rob Holt (rjmholt) merged 3 commits into
PowerShell:masterfrom
kyanha:issue10570v3

Conversation

@kyanha

Copy link
Copy Markdown
Contributor

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

@kyanha

Kyle H (kyanha) commented Oct 30, 2020

Copy link
Copy Markdown
Contributor Author

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 Kyle H (kyanha) left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a few notes for the reviewers.

// If we want to retrieve the file streams, retrieve them.
if (retrieveStreams)
{
if (!isContainer)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread 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);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

internal static class NativeMethods
{
internal const int ERROR_HANDLE_EOF = 38;
internal const int ERROR_INVALID_PARAMETER = 87;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

$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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@kyanha

Copy link
Copy Markdown
Contributor Author

Closing and reopening to rerun CI.

@kyanha

Copy link
Copy Markdown
Contributor Author

Reopening to rerun CI.

@kyanha

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown

sweet, that attribute looks good now!

@kyanha

Copy link
Copy Markdown
Contributor Author

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.

@rjmholt Rob Holt (rjmholt) added the Review - Needed The PR is being reviewed label Nov 10, 2020
Comment thread src/System.Management.Automation/namespaces/FileSystemProvider.cs Outdated
Comment thread src/System.Management.Automation/namespaces/FileSystemProvider.cs Outdated
Comment thread src/System.Management.Automation/namespaces/FileSystemProvider.cs
Comment thread src/System.Management.Automation/namespaces/FileSystemProvider.cs Outdated
Comment thread src/System.Management.Automation/namespaces/FileSystemProvider.cs
@kyanha

Copy link
Copy Markdown
Contributor Author

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

@n3rdopolis

Copy link
Copy Markdown

Are they going to merge this? it worked for me

@rjmholt

Copy link
Copy Markdown
Collaborator

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?

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

@rjmholt Rob Holt (rjmholt) left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM, with the suggestion from earlier

@kyanha

Copy link
Copy Markdown
Contributor Author

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

@kyanha

Kyle H (kyanha) commented Dec 3, 2020

Copy link
Copy Markdown
Contributor Author

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.

@kyanha

Copy link
Copy Markdown
Contributor Author

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

@rjmholt
Rob Holt (rjmholt) merged commit 909ca89 into PowerShell:master Dec 8, 2020
@iSazonov Ilya (iSazonov) added this to the 7.2.0-preview.2 milestone Dec 8, 2020
@iSazonov

Copy link
Copy Markdown
Collaborator

Kyle H (@kyanha) Thanks for your contribution!

@ghost

Copy link
Copy Markdown

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

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Get-Content, Set-Content, and Clear-Content should work with datastreams set on directories Get-Item P:\ath\To\A\Directory -Streams * does not work

4 participants