microsoft / sarif-sdk

.NET code and supporting files for working with the 'Static Analysis Results Interchange Format' (SARIF, see https://github.com/oasis-tcs/sarif-spec)
Other
193 stars 93 forks source link

Handling Platform Specific Skimmers #751

Open Evmaus-MS opened 6 years ago

Evmaus-MS commented 6 years ago

@michaelcfanning, @lgolding for comments on approach, as I'm new to the Sarif-SDK internals.

Background: Porting BinSkim to run on Linux involves changing some skimmers to flag as "Only supported on Windows" since they rely on native interop. I've discussed this a bit offline with Michael--it seems like there's two options for marking the checks as not supported on a particular platform.

One option is changing the meaning of "CanAnalyze()" to also cover "Can't be analyzed on this platform"--however, the return of CanAnalyze seems to answer to "Should this binary be analyzed by this rule?", which is a different question from "Is this rule supported on this platform?", so this doesn't seem like the right approach. It also would be nice to give an actual error message (rather than just "can't analyze").

One good option is having every Skimmer that can't run on non-Windows platforms throw a PlatformNotSupportedException or similar, and disable the rule after the first time the exception is thrown. This has the nice effect of not spamming the user with "We can't run this rule" when analyzing large numbers of binaries, while still surfacing it as an error. However, this currently leads to a fatal exit code--so if we go this route, we'd want to also add a new non-fatal RunTimeError and treat that exception as a different case when Skimmer.Analyze is called in the AnalyzeCommandBase. (I've got what I believe are the appropriate changes in https://github.com/Microsoft/sarif-sdk/compare/master...Evmaus-MS:skimmer-platform-unsupported and can submit a PR for them if this is the correct approach.)

ghost commented 6 years ago

I like the idea of platform-specific skimmers. Here's another way to implement it:

  1. Define an enumeration:

    [Flags]
    public enum SupportedPlatforms
    {
        Windows = 0x01,
        Linux   = 0x02,
        All = Windows | Linux
    }
  2. Define a new property onSkimmerBase:

    public virtual SupportedPlatforms SupportedPlatforms => SupportedPlatforms.All;

    Specific skimmers can override it to omit platforms they don't support, for example:

    public class WindowsSpecificSkimmer : SkimmerBase
    {
        public override SupportedPlatforms SupportedPlatforms
            => SupportedPlatforms.All & ~SupportedPlatforms.Linux;
    }
  3. Modify the private method AnalyzeCommandBase.CreateSkimmers as follows:

    skimmers = DriverUtilities.GetExports<ISkimmer<TContext>>(DefaultPlugInAssemblies);
    
    SupportedPlatform currentPlatform = GetCurrentPlatform(); // I don't know how to do this.
    foreach (ISkimmer<TContext> skimmer in skimmers)
    {
        if (skimmer.SupportedPlatforms.Contains(currentPlatform))
        {
            result.Add(skimmer);
        }
        else
        {
            // Define a new warning-level notification, and add a new method to the Warnings class:
            Warnings.LogUnsupportedPlatform(skimmer.Id, currentPlatform);
        }
    }

This has the nice design characteristics:

  1. It doesn't overload the meaning of CanAnalyze; instead if makes platform support a first-class concept in the SDK.
  2. It only reports the notification once.
  3. It's a warning-level notification.