Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Combability with .NET ReadyToRun and Self-Contained #970

Open
shaopeng-gh opened this issue Nov 3, 2023 · 6 comments · May be fixed by #1013
Open

Combability with .NET ReadyToRun and Self-Contained #970

shaopeng-gh opened this issue Nov 3, 2023 · 6 comments · May be fixed by #1013

Comments

@shaopeng-gh
Copy link
Collaborator

This just is to note that Binskim combability with .NET ReadyToRun and self-contained could be looked into.
with p:PublishReadyToRun=true and --self-contained

With PublishReadyToRun the section alignment changed to 200 from 2000.
Binskim will fail with PublishReadyToRun while pass without.
Not sure if it is a security issue or by design.

image
image

full command
"C:\Program Files\dotnet\dotnet.exe" publish C:\Sources\Repos\aaa\aaa.csproj --configuration Debug --output C:\Sources\Repos\aaa\linux-x64\aaa -r linux-x64 p:PublishReadyToRun=true --self-contained --framework net6.0

##[error]7. BinSkim Error BA2021 - File: build/linux-x64/aaa/aaa.dll.
Tool: BinSkim: Rule: BA2021 (DoNotMarkWritableSectionsAsExecutable). https://github.com/microsoft/binskim/blob/main/docs/BinSkimRules.md#rule-BA2021DoNotMarkWritableSectionsAsExecutable
'aaa.dll' has a section alignment (0x200) that is smaller than its page size (0x1000).

@shaopeng-gh
Copy link
Collaborator Author

User report that BinSkim BA2021 could have compatibility issue with R2R Linux binaries

This issue is created to investigate if below check should be removed/revised

// TODO: do we really require this check? What is the proposed fix to this issue? 
if (peHeader.SectionAlignment < PAGE_SIZE)
{
    // '{0}' has a section alignment ({1}) that is less than its page size ({2}).
    context.Logger.Log(this,
        RuleUtilities.BuildResult(FailureLevel.Error, context, null,
            nameof(RuleResources.BA2021_Error_UnexpectedSectionAligment),
            context.CurrentTarget.Uri.GetFileName(),
            "0x" + peHeader.SectionAlignment.ToString("x"),
            "0x" + PAGE_SIZE.ToString("x")));
    return;
}

To repro, download official C# extension Linux x64 version from
https://marketplace.visualstudio.com/items?itemName=ms-dotnettools.csharp
image

@GrumpyCytokine
Copy link

GrumpyCytokine commented Mar 3, 2024

We also encountered this issue, in binaries compiled for Linux only.
Binaries compiled for Windows do not raise error BA2021.

@shaopeng-gh
Copy link
Collaborator Author

Thanks for adding the feedback.

@steveisok
Copy link

The layout of R2R assemblies on non-windows is by design. The main issue appears to be that BinSkim rules are Windows specific. Ideally, you want to relax or ignore this rule for non-windows platforms.

steveisok added a commit to steveisok/binskim that referenced this issue Oct 4, 2024
This change skips analysis when it finds non-Windows .NET R2R & NativeAOT PE's. The reason for this is the DoNotMarkWritableSectionsAsExecutable
is Windows specific and R2R/NativeAOT do not follow Windows layout rules on non-Windows platforms.

Fixes microsoft#970
@AllDwarf
Copy link
Collaborator

Hey @steveisok, I'm just looking into the PR. Can you also share some additional for MetadataCondition_ImageIsNonWindowsDotNetAssembly Where this value can be found is there any docs page?

@steveisok
Copy link

Hey @steveisok, I'm just looking into the PR. Can you also share some additional for MetadataCondition_ImageIsNonWindowsDotNetAssembly Where this value can be found is there any docs page?

I made it up to fit the change as it's going to skip scanning when it finds one of these. There wasn't a good already existing message, so I created one. Open to suggestions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants