Use DefaultDllImportSearchPaths attribute #2658

LLLXXXCCC · 2019-07-08T10:31:04Z

…hub.com/LLLXXXCCC/roslyn-analyzers into UseDefaultDllImportSearchPathsAttribute

dotpaul · 2019-07-17T00:18:43Z

src/Microsoft.NetCore.Analyzers/Core/Security/SystemSecurityCryptographyResources.resx

+    <value>Use DefaultDllImportSearchPaths Attribute For P/Invokes</value>
+  </data>
+  <data name="UseDefaultDllImportSearchPathsAttributeDescription" xml:space="preserve">
+    <value>By default, P/Invokes using DllImportAttribute probe a number of directories, including the current working directory for the library to load. This can be a security issue for certain applications, leading to dll hijacking.</value>


dll [](start = 220, length = 3)

DLL

dotpaul · 2019-07-17T00:21:10Z

src/Microsoft.NetCore.Analyzers/Core/Security/UseDefaultDllImportSearchPathsAttribute.cs

+                        var dllPath = constructorArguments[0].Value.ToString();
+
+                        if ((Path.IsPathRooted(dllPath) &&
+                            dllPath.EndsWith(".dll", StringComparison.Ordinal)) ||


Ordinal [](start = 70, length = 7)

OrdinalIgnoreCase

dotpaul · 2019-07-17T00:29:04Z

src/Microsoft.NetCore.Analyzers/Core/Security/UseDefaultDllImportSearchPathsAttribute.cs

+                        var dllPath = constructorArguments[0].Value.ToString();
+
+                        if ((Path.IsPathRooted(dllPath) &&
+                            dllPath.EndsWith(".dll", StringComparison.Ordinal)) ||


Does it matter whether or not it ends in ".dll"?

dotpaul · 2019-07-17T00:35:41Z

src/Microsoft.NetCore.Analyzers/Core/Security/UseDefaultDllImportSearchPathsAttribute.cs

+                if (compilation.Assembly.GetAttributes().Select(o => o.AttributeClass).Contains(defaultDllImportSearchPathsAttributeTypeSymbol))
+                {
+                    hasDefaultDllImportSearchPathsAttribute = true;
+                }


If there's an assembly level DefaultDllImportSearchPaths, do we need to do anything further?

DllImportSearchPath.AssemblyDirectory, DllImportSearchPath.LegacyBehavior, DllImportSearchPath.UseDllDirectoryForDependencies.
If these 3 flag were used in DefaultDllImportSearchPaths, it will still search in the default order when it can't find the DLL in the specified path.
I don't know if we should take this into consideration.

dotpaul · 2019-07-17T00:43:15Z

...crosoft.NetCore.Analyzers/UnitTests/Security/UseDefaultDllImportSearchPathsAttributeTests.cs

+        //  3. The system directory, usually C:\\Windows\\System32\\ (The GetSystemDirectory function is called to obtain this directory.).
+        //  4. The 16-bit system directory – There is no dedicated function to retrieve the path of this directory, but it is searched as well.
+        //  5. The Windows directory. The GetWindowsDirectory function is called to obtain this directory.
+        //  6. The directories that are listed in the PATH environment variable


I don't think we should try to document behavior here, since it varies a lot, we're unlikely to be accurate.

dotpaul · 2019-07-17T00:50:54Z

...crosoft.NetCore.Analyzers/UnitTests/Security/UseDefaultDllImportSearchPathsAttributeTests.cs

+
+        // [DllImport] is set with an absolute path, which will let the [DefaultDllImportSearchPaths] be ignored.
+        // So user32.dll will also be searched in the default order.
+        [Fact]


It doesn't just load from the absolute path?

You are right, wrong comment.

…dencies.

…hub.com/LLLXXXCCC/roslyn-analyzers into UseDefaultDllImportSearchPathsAttribute

…hub.com/LLLXXXCCC/roslyn-analyzers into LLLXXXCCC-UseDefaultDllImportSearchPathsAttribute

… UseDefaultDllImportSearchPathsAttribute

dotpaul · 2019-07-30T00:33:56Z

src/Microsoft.NetCore.Analyzers/Core/Security/UseDefaultDllImportSearchPathsAttribute.cs

+            MicrosoftNetCoreAnalyzersResources.ResourceManager,
+            typeof(MicrosoftNetCoreAnalyzersResources));
+
+        private const int UnsafeBits = 2 | 256;


2 | 256; [](start = 39, length = 8)

Add a comment for what these values are.

Given that we're treating AssemblyDirectory as potentially dangerous, should do the same for ApplicationDirectory.

When using ApplicationDirectory, it won't search by the default order after it didn't find the target DLL in the application directory. When using AssemblyDirectory, it does, that's why we treat it as potentially dangerous.

I think the attack scenario of executing an application with elevated privileges from a user's "Downloads" directory, which contains a malicious DLL, is something we should worry about. In that case, I think ApplicationDirectory is also a problem, because it's going to include the "Downloads" directory.

src/Microsoft.NetCore.Analyzers/Core/Security/UseDefaultDllImportSearchPathsAttribute.cs

docs/Analyzer Configuration.md

dotpaul · 2019-07-30T00:58:07Z

src/Microsoft.NetCore.Analyzers/Core/MicrosoftNetCoreAnalyzersResources.resx

    <value>Do not create tasks without passing a TaskScheduler</value>
  </data>
+  <data name="UseDefaultDllImportSearchPathsAttribute" xml:space="preserve">
+    <value>Use DefaultDllImportSearchPaths attribute for P/Invokes</value>


Use DefaultDllImportSearchPaths attribute for P/Invokes [](start = 11, length = 55)

Trying to think of a title/message/description kinda makes me think we should have a separate rule for checking the value. What do you think?

If you thumbs up the comment, does that mean you're going to add another rule? 🙂

Is that a "yes let's have a separate diagnostic if the value has unsafe bits", or a "no let's use the same diagnostic"? 🙂

...crosoft.NetCore.Analyzers/UnitTests/Security/UseDefaultDllImportSearchPathsAttributeTests.cs

src/Utilities/Compiler/Options/EditorConfigOptionNames.cs

Sync.

dotpaul · 2019-07-31T21:42:33Z

...crosoft.NetCore.Analyzers/UnitTests/Security/UseDefaultDllImportSearchPathsAttributeTests.cs

+
+        [Theory]
+        [InlineData("")]
+        [InlineData("dotnet_code_quality.CA5392.unsafe_DllImportSearchPath_bits = 2 | 256")]


2 | 256 [](start = 82, length = 7)

This case doesn't prove that the parsing works. Without me looking at the code, it could be just using the default value. It could just be parsing the 2 and ignoring the rest.

Add a unit test for say "1 | 1024", and see if [DefaultDllImportSearchPaths(DillImportSearchPath.UserDirectories)] will produce a diagnostic.

dotpaul · 2019-08-02T02:31:03Z

...crosoft.NetCore.Analyzers/UnitTests/Security/UseDefaultDllImportSearchPathsAttributeTests.cs

+}",
+            GetEditorConfigAdditionalFile(editorConfigText),
+            Array.Empty<DiagnosticResult>());
+        }


So doesn't this case show that the | syntax in analyzer config integer values doesn't work? We shouldn't use | as an example in the Analyzer Configuration.md in the documentation.

docs/Analyzer Configuration.md

src/Microsoft.NetCore.Analyzers/Core/MicrosoftNetCoreAnalyzersResources.resx

src/Microsoft.NetCore.Analyzers/Core/Security/UseDefaultDllImportSearchPathsAttribute.cs

dotpaul · 2019-08-02T15:18:36Z

src/Microsoft.NetCore.Analyzers/Core/Security/UseDefaultDllImportSearchPathsAttribute.cs

+
+                                rule = DoNotUseUnsafeDllImportSearchPathRule;
+                                ruleArgument = ((DllImportSearchPath)validBits).ToString();
+                            }


Not a huge deal, but may want to consider refactoring so you don't have to repeat this code and the code below.

src/Microsoft.NetCore.Analyzers/Core/MicrosoftNetCoreAnalyzersResources.resx

...crosoft.NetCore.Analyzers/UnitTests/Security/UseDefaultDllImportSearchPathsAttributeTests.cs

dotpaul

🚢

Zenexer · 2019-09-17T20:51:08Z

Can you clarify the relevance of this on platforms other than Windows?

LingxiaChen and others added 8 commits July 8, 2019 18:29

UseDefaultDllImportSearchPathsAttribute

bcaaa2a

Update rule title.

e09d6ca

Merge branch 'master' into UseDefaultDllImportSearchPathsAttribute

20ec3e0

Update rule message.

2a7eecc

Update.

3207b5f

Merge branch 'UseDefaultDllImportSearchPathsAttribute' of https://git…

3543d04

…hub.com/LLLXXXCCC/roslyn-analyzers into UseDefaultDllImportSearchPathsAttribute

Add comment to test cases.

c9e6108

Add comment about the Known Dlls.

314ec65

dotpaul reviewed Jul 17, 2019

View reviewed changes

LingxiaChen and others added 12 commits July 18, 2019 12:44

Remove the dll extension limitaion.

dd1ff21

Add a test case using nonexistent absolute path.

03a4151

Mark cases using absolute path to import dll as NoDiagnostic.

4eb57f0

Add a test case.

67a066e

Merge branch 'master' into UseDefaultDllImportSearchPathsAttribute

98f8ffc

Do not use AssemblyDirectory, LegacyBehavior, UseDllDirectoryForDepen…

b66bb54

…dencies.

Merge branch 'UseDefaultDllImportSearchPathsAttribute' of https://git…

80412bb

…hub.com/LLLXXXCCC/roslyn-analyzers into UseDefaultDllImportSearchPathsAttribute

Merge branch 'master' into UseDefaultDllImportSearchPathsAttribute

39324b8

Merge branch 'UseDefaultDllImportSearchPathsAttribute' of https://git…

b143627

…hub.com/LLLXXXCCC/roslyn-analyzers into LLLXXXCCC-UseDefaultDllImportSearchPathsAttribute

Merge branch 'LLLXXXCCC-UseDefaultDllImportSearchPathsAttribute' into…

c43e691

… UseDefaultDllImportSearchPathsAttribute

Sync.

a873385

Change title using sentence casing.

f620275

dotpaul reviewed Jul 30, 2019

View reviewed changes

src/Microsoft.NetCore.Analyzers/Core/Security/UseDefaultDllImportSearchPathsAttribute.cs Outdated Show resolved Hide resolved

dotpaul reviewed Jul 30, 2019

View reviewed changes

docs/Analyzer Configuration.md Outdated Show resolved Hide resolved

dotpaul reviewed Jul 30, 2019

View reviewed changes

docs/Analyzer Configuration.md Outdated Show resolved Hide resolved

dotpaul reviewed Jul 30, 2019

View reviewed changes

docs/Analyzer Configuration.md Outdated Show resolved Hide resolved

dotpaul reviewed Jul 30, 2019

View reviewed changes

docs/Analyzer Configuration.md Outdated Show resolved Hide resolved

dotpaul reviewed Jul 30, 2019

View reviewed changes

...crosoft.NetCore.Analyzers/UnitTests/Security/UseDefaultDllImportSearchPathsAttributeTests.cs Outdated Show resolved Hide resolved

dotpaul reviewed Jul 30, 2019

View reviewed changes

src/Utilities/Compiler/Options/EditorConfigOptionNames.cs Outdated Show resolved Hide resolved

LLLXXXCCC and others added 2 commits July 30, 2019 10:20

Merge pull request #19 from dotnet/master

6ca96e7

Sync.

Address review feedback.

d43709f

dotpaul reviewed Jul 31, 2019

View reviewed changes

LingxiaChen added 2 commits August 1, 2019 15:58

Add test cases for EditorConfigOption.

3704fb1

Mark DllImportSearchPath.ApplicationDirectory as a bad use.

d71044c

dotpaul reviewed Aug 2, 2019

View reviewed changes

Add another rule for using unsafe DllImportSearchPath value.

b61330e

dotpaul reviewed Aug 2, 2019

View reviewed changes

docs/Analyzer Configuration.md Outdated Show resolved Hide resolved

dotpaul reviewed Aug 2, 2019

View reviewed changes

src/Microsoft.NetCore.Analyzers/Core/MicrosoftNetCoreAnalyzersResources.resx Outdated Show resolved Hide resolved

dotpaul reviewed Aug 2, 2019

View reviewed changes

src/Microsoft.NetCore.Analyzers/Core/Security/UseDefaultDllImportSearchPathsAttribute.cs Outdated Show resolved Hide resolved

dotpaul reviewed Aug 2, 2019

View reviewed changes

src/Microsoft.NetCore.Analyzers/Core/MicrosoftNetCoreAnalyzersResources.resx Outdated Show resolved Hide resolved

dotpaul reviewed Aug 2, 2019

View reviewed changes

...crosoft.NetCore.Analyzers/UnitTests/Security/UseDefaultDllImportSearchPathsAttributeTests.cs Outdated Show resolved Hide resolved

Address review feedback.

6d53be4

dotpaul approved these changes Aug 5, 2019

View reviewed changes

LLLXXXCCC merged commit cb9f864 into dotnet:master Aug 7, 2019

Zenexer mentioned this pull request Sep 17, 2019

Incomplete DefaultDllImportSearchPaths docs dotnet/dotnet-api-docs#3205

Open

Use DefaultDllImportSearchPaths attribute #2658

Use DefaultDllImportSearchPaths attribute #2658

Uh oh!

Conversation

LLLXXXCCC commented Jul 8, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dotpaul Jul 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dotpaul left a comment

Choose a reason for hiding this comment

Uh oh!

Zenexer commented Sep 17, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dotpaul Jul 30, 2019 •

edited

Loading