Remove BA2015 false positive for exes that we expect to run as 32-bit

[michaelcfanning]

@JayOfManyTrades, FYI

I have ported your excellent fix to the CanAnalyze method. This call already filtered out EXEs, so we simply needed to add your check for Bit32Machine. Honestly a little surprised this false positive has lingered in the tool so long!

I have added you as a contributor to this repo. In the future, please feel free to branch off main to propose fixes, new rules, etc.

I will take a note to update our onboarding guidance. Currently, new contributors propose a change from their fork that a current contributor sanity checks. Then we add them as contributors and request a new branch/PR to main. All this hoops-jumping is intended to provide a guarantee that our various workflows don't execute against arbitrary changes contributed by new users. A supply-chain security protection. There may be a better way to onboard, if you have any suggestions, let me know.

I'd appreciate your review and testing here (particularly against any of your own files you have that are appropriate for testing). Note that I did not directly port your test file changes. Instead, reapplied your code fix, then rebaselined. From this, we have a reasonable guarantee that this new version accomplishes what your original change did (because the same test file edits were generated as yours).

Thanks again for the contribution.

[JayOfManyTrades]

@michaelcfanning, I completed my initial testing, which consisted of repeating the detailed tests―which I previously performed for the changes I "originally" proposed―with your changes. The results were identical. (In other words, "so far, so good!")

I want to complete additional testing before I officially "sign off" on your changes. I may not finish the tests today but should do so by the end of Wednesday of next week.

[michaelcfanning]

@HulonJenkins @shaopeng-gh can you look at this?

[shaopeng-gh]

Looking...

---

In reply to: 1287439364

[shaopeng-gh]

Tried to create file to test,
VS2022 DotnetCore with manually set <Prefer32Bit>true</Prefer32Bit>
using the branch it shows:

And the code will return as Not Applicable.

But when I run it I believe it will run as 64bit by default?
Unless specify the 32bit dotnet.

The code I used is:

using System.Diagnostics;
using System.Runtime.InteropServices;

namespace Managed_x64_VS2022_NetCore_CSharp
{
    internal class Program
    {
        [DllImport("kernel32.dll", SetLastError = true, CallingConvention = CallingConvention.Winapi)]
        [return: MarshalAs(UnmanagedType.Bool)]
        public static extern bool IsWow64Process(
    [In] IntPtr hProcess,
    [Out] out bool wow64Process
);

        static void Main(string[] args)
        {
            Console.WriteLine("Environment.Is64BitProcess: " + Environment.Is64BitProcess);

            IsWow64Process(Process.GetCurrentProcess().Handle, out bool ret_val);
            Console.WriteLine("IsWow64Process: " + ret_val);

            if (IntPtr.Size == 8)
            {
                Console.WriteLine("IntPtr.Size: 64 bit machine");
            }
            else if (IntPtr.Size == 4)
            {
                Console.WriteLine("IntPtr.Size: 32 bit machine");
            }
        }
    }
}

If I ignore the code and force it to analyze, by default it does have it on and passed the rule:

If I disable it by <HighEntropyVA>False</HighEntropyVA>, it will fail the rule:

---

In reply to: 1287639820

[shaopeng-gh]

The other minor comments I have is
2. I see we only support exe, but seems dll should also support in the document
https://learn.microsoft.com/en-us/cpp/build/reference/highentropyva-support-64-bit-aslr?view=msvc-170

/HIGHENTROPYVA modifies the header of an executable image file (for example, a .dll or .exe file), to indicate whether ASLR can use the entire 64-bit address space. To have an effect, set the option on both the executable and all modules that it depends on.

reasonForNotAnalyzing = MetadataConditions.ImageIsNotExe;
if (!portableExecutable.PEHeaders.IsExe) { return result; }

3. fix by add /HIGHENTROPYVA to the C or C++ linker command line. see if we want to add or <HighEntropyVA>True</HighEntropyVA> to .csproj file for C#.

---

In reply to: 1287641845

[shaopeng-gh]

        // that will always be configured for high entropy va, if available.

per my two test files added, I think this is wrong as well.

---

In reply to: 1294325595

---

Refers to: src/BinSkim.Rules/PERules/BA2015.EnableHighEntropyVirtualAddresses.cs:75 in 6e17d94. [](commit_id = 6e17d94, deletion_comment = False)

[shaopeng-gh]

Update: I have checked in the 2 test files corresponding to the screenshots I added above for reviewer's convenience.

I put them in the folder that I think they should be at, not per the current code logic.

To debug, set BinSkim.Driver as startup project and use analyze [filepath] point to the local file path
e.g.
analyze C:\GitHub\binskim\src\Test.FunctionalTests.BinSkim.Rules\FunctionalTestData\BA2015.EnableHighEntropyVirtualAddresses\Fail\HIGHENTROPYVA\Managed_x64_VS2022_NetCore6_CSharp_HighEntropyVA_False.dll
and
analyze C:\GitHub\binskim\src\Test.FunctionalTests.BinSkim.Rules\FunctionalTestData\BA2015.EnableHighEntropyVirtualAddresses\Pass\Managed_x64_VS2022_NetCore6_CSharp_HighEntropyVA_True.dll

---

In reply to: 1294342020

[shaopeng-gh]

🕐

[JayOfManyTrades]

@michaelcfanning, thanks for your patience! I plan to complete my hopefully final round of testing this weekend.

[JayOfManyTrades]

I did not get to this over the weekend.

[michaelcfanning]

Managed_x64_VS2022_NetCore6_CSharp_HighEntropyVA_

Take a look at changes I made in the rule. See if you agree.

For this test file, our code says that any dotnet core entry point dll which is loaded into a 64-bit process will be forced into high entropy va. Which makes this setting effectively meaningless for that scenario.

Now, if users can set it reliably, we would recommend it.

Thoughts? #Closed

[shaopeng-gh]

1. I see in the if statement the Requires32Bit and Prefers32Bit is removed, agree. My understanding per the test file I created, even a file with both true can be running in 64 by default, so there is no point checking them.
2. We also removed !portableExecutable.IsManaged, I think it is by mistake?
3. We now have a second check if (!coffHeader.Characteristics.HasFlag(Characteristics.LargeAddressAware)) and return not applicable, I think we don't need it because we have that case in the Analyze(), and in this case currently we ask people to enable it together with HIGHENTROPYVA.
4. I think I may mistake your question here but let me first explain my thoughts: I see we have a comment in the code "A dotnet core entry point dll is itself loaded within a process that will always be configured for high entropy va, if available."
   I am wondering if this is true and have a source of it, because the 2 test files I created, currently will get caught by this check right under this comment, and directly return not applicable.
   However if I delete this comment with the check, my 2 test files can get analyzed and pass and fail, according to the setting I set in <HighEntropyVA>True/False</HighEntropyVA>.
   So I brought this question for discussion.
   If my assumption is wrong, my current two test files should be deleted because they will block the PR checks with current code.

[shaopeng-gh]

Thanks for fixing 2 and 3 and explaining the 4 does not matter what HighEntropyVA user set because will be taken care of by runtime.

[shaopeng-gh]

I have fixed the test and the dotnet format.
The current test does not work in Linux is not related to this PR, happening in main as well, I am looking to that.
I have approved the pr, ready for merge.

[shaopeng-gh]