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

Enable analysis for managed binary #335

Merged
merged 11 commits into from
Jan 25, 2021
46 changes: 20 additions & 26 deletions src/BinSkim.Rules/PERules/BA2004.EnableSecureSourceCodeHashing.cs
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,15 @@ public class EnableSecureSourceCodeHashing : WindowsBinaryAndPdbSkimmerBase, IOp

public override AnalysisApplicability CanAnalyzePE(PEBinary target, Sarif.PropertiesDictionary policy, out string reasonForNotAnalyzing)
{
PE portableExecutable = target.PE;
AnalysisApplicability result = AnalysisApplicability.NotApplicableToSpecifiedTarget;
Pdb di = target.Pdb;
if (target.PE.IsManaged && (di?.FileType == PdbFileType.Windows))
{
reasonForNotAnalyzing = MetadataConditions.CouldNotLoadPdb;
Copy link
Member

Choose a reason for hiding this comment

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

reasonForNotAnalyzing = MetadataConditions.CouldNotLoadPdb [](start = 16, length = 58)

in other checks, the absence of a PDB results in an error level notification, not a 'not applicable' message. 'Not applicable' is not the right return value here, as this return value means 'the binary isn't a valid scan target'. that's not true here, managed code is a valid scan target, the problem is that we can't analyze it due to a missing pdb. you should go look and see how the native pdb reading errors handle a missing pdb.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can you validate again? pushed new changes. Thank you


In reply to: 563214549 [](ancestors = 563214549)

return AnalysisApplicability.NotApplicableToSpecifiedTarget;
}

reasonForNotAnalyzing = MetadataConditions.ImageIsILOnlyAssembly;
if (portableExecutable.IsILOnly) { return result; }
//reasonForNotAnalyzing = MetadataConditions.ImageIsILOnlyAssembly;
//if (portableExecutable.IsILOnly) { return result; }

// TODO: currently our test binary for this check is a dll that does not
// compile against any external library. BinSkim regards this as a resource
Expand All @@ -55,23 +59,10 @@ public override AnalysisApplicability CanAnalyzePE(PEBinary target, Sarif.Proper

public override void AnalyzePortableExecutableAndPdb(BinaryAnalyzerContext context)
{
if (context.PEBinary().PE.IsManaged)
{
AnalyzeManagedAssemblyAndPdb(context);
return;
}

AnalyzeNativeBinaryAndPdb(context);
}

#pragma warning disable IDE0060 // Remove unused parameter
private void AnalyzeManagedAssemblyAndPdb(BinaryAnalyzerContext context)
#pragma warning restore IDE0060 // Remove unused parameter
{
// TODO: use DiaSymReader?
AnalyzeBinaryAndPdb(context);
}

public void AnalyzeNativeBinaryAndPdb(BinaryAnalyzerContext context)
public void AnalyzeBinaryAndPdb(BinaryAnalyzerContext context)
{
PEBinary target = context.PEBinary();
Pdb di = target.Pdb;
Expand All @@ -84,15 +75,18 @@ public void AnalyzeNativeBinaryAndPdb(BinaryAnalyzerContext context)
Symbol om = omView.Value;
ObjectModuleDetails omDetails = om.GetObjectModuleDetails();

if (omDetails.Language != Language.C &&
omDetails.Language != Language.Cxx)
if (!target.PE.IsManaged)
{
continue;
}
if (omDetails.Language != Language.C &&
omDetails.Language != Language.Cxx)
{
continue;
}

if (!omDetails.HasDebugInfo)
{
continue;
if (!omDetails.HasDebugInfo)
{
continue;
}
}

CompilandRecord record = om.CreateCompilandRecord();
Expand Down
2 changes: 1 addition & 1 deletion src/BinaryParsers/PEBinary/PEBinary.cs
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ private Pdb LoadPdb()
// We should never be required to load a PDB for a managed assembly that does
// not incorporate native code, as no managed-relevant rule currently crawls
// PDBs for its analysis.
Copy link
Member

@michaelcfanning michaelcfanning Jan 18, 2021

Choose a reason for hiding this comment

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

delete all this eventually. :) #Closed

Debug.Assert(!this.PE.IsManaged || this.PE.IsMixedMode);
// Debug.Assert(!this.PE.IsManaged || this.PE.IsMixedMode);

Pdb pdb = null;
try
Expand Down
47 changes: 47 additions & 0 deletions src/BinaryParsers/PEBinary/ProgramDatabase/Pdb.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@

using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.IO;
using System.Runtime.InteropServices;
using System.Text;
using System.Threading;
Expand All @@ -22,6 +24,11 @@ public sealed class Pdb : IDisposable, IDiaLoadCallback2
private StringBuilder loadTrace;
private readonly Lazy<Symbol> globalScope;
private bool restrictReferenceAndOriginalPathAccess;
private PdbFileType pdbFileType;
private const string s_windowsPdbSignature = "Microsoft C/C++ MSF 7.00\r\n\x001ADS\x0000\x0000\x0000";
private const string s_portablePdbSignature = "BSJB";
public static readonly ImmutableArray<byte> WindowsPdbSignature = ImmutableArray.Create(Encoding.ASCII.GetBytes(s_windowsPdbSignature));
public static readonly ImmutableArray<byte> PortablePdbSignature = ImmutableArray.Create(Encoding.ASCII.GetBytes(s_portablePdbSignature));

/// <summary>
/// Load debug info from PE or PDB, using symbolPath to help find symbols
Expand Down Expand Up @@ -134,6 +141,46 @@ private void WindowsNativeLoadPdbUsingDia(string pePath, string symbolPath, stri

public bool IsStripped => this.GlobalScope.IsStripped;

public PdbFileType FileType
Copy link
Member

@michaelcfanning michaelcfanning Jan 23, 2021

Choose a reason for hiding this comment

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

You should cache this property value rather than computing it every time. #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are caching unless it's unknown


In reply to: 563214200 [](ancestors = 563214200)

{
get
{
if (this.pdbFileType != PdbFileType.Unknown)
{
return this.pdbFileType;
}

if (Directory.Exists(PdbLocation))
{
return PdbFileType.Unknown;
}

int max = Math.Max(WindowsPdbSignature.Length, PortablePdbSignature.Length);

byte[] b = new byte[max];

using (FileStream fs = File.OpenRead(PdbLocation))
{
if (fs.Read(b, 0, b.Length) != b.Length)
{
return this.pdbFileType;
}
}

Span<byte> span = b.AsSpan();

if (WindowsPdbSignature.AsSpan().SequenceEqual(span.Slice(0, WindowsPdbSignature.Length)))
{
this.pdbFileType = PdbFileType.Windows;
}
else if (PortablePdbSignature.AsSpan().SequenceEqual(span.Slice(0, PortablePdbSignature.Length)))
{
this.pdbFileType = PdbFileType.Portable;
}

return this.pdbFileType;
}
}

/// <summary>
/// Get the list of modules in this executable
Expand Down
26 changes: 26 additions & 0 deletions src/BinaryParsers/PdbFileType.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// Copyright (c) Microsoft. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

namespace Microsoft.CodeAnalysis.BinaryParsers
{
/// <summary>
/// What is the format of a file.
/// </summary>
public enum PdbFileType
{
/// <summary>
/// Unknown format.
/// </summary>
Unknown,

/// <summary>
/// Windows specific format.
/// </summary>
Windows,

/// <summary>
/// Portable OS format.
/// </summary>
Portable,
}
}
Loading