Skip to content

Commit

Permalink
Fix #1593: FortifyFprConverter produces invalid SARIF (#1641)
Browse files Browse the repository at this point in the history 
There were several bugs in the converter that led to invalid SARIF:

- The trailing 12-digit component of the GUID identifying the `toolComponent` for the CWE taxonomy had only 11 digits.

    This was due to an invalid GUID in `FortifyFprConverter.CweToolComponent.Guid`. Presumably this was a copy/paste error from when the author generated the GUID. I generated a new, valid GUID.

    Also, the invalid GUID had been copy/pasted to another location in the source code, rather than referring to it by its name `FortifyFprConverter.CweToolComponent.Guid`.

- Some `threadFlowLocation.kinds` arrays contained duplicate (non-unique) values.

    This was due to an incorrect initialization of one of the elements of the `ActionTypeToLocationKindMap` (which I also renamed to `ActionTypeToLocationKinds`).

- The array `run.threadFlowLocations` contained duplicate (non-unique) elements.

    This was due to the assumption that the `ThreadFlowLocation` objects we constructed from the Fortify `UnifiedNodePool` were all distinct. Although it is true that all the `Node` sub-elements of the `UnifiedNodePool` element are distinct, we do not use all of the properties of the `Node` element in constructing a `ThreadFlowLocation`. As a result, some of the `ThreadFlowLocation`s were identical.

- `rule.id` was missing. Fortify doesn't have anything other than the GUID to serve as `rule.id` -- which, per the spec, needs to be a "stable, opaque" identifier. So I assigned the GUID to both the `id` and `guid` properties.

Also:

- Upgrade FortifyTest.fpr.sarif (which had been generated by a pre-release version of the SDK) to the final version of the SARIF 2.1.0 format. This is necessary because if this file is down-level, the `FortifyFprConverter`'s call to the `PrereleaseCompatibilityTransformer` produces valid SARIF, masking the bugs in the converter.
- We do a little code cleanup (removing unnecessary parentheses in object initializers, capitalizing "SARIF" in comments, _etc._).
  • Loading branch information
Larry Golding committed Aug 14, 2019
1 parent 6e194ec commit 8bc1e45
Show file tree
Hide file tree
Showing 7 changed files with 1,428 additions and 1,640 deletions.
5 changes: 3 additions & 2 deletions src/ReleaseHistory.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -436,7 +436,7 @@
* BUGFIX: Improve handling of `null` values in property bags. https://github.com/microsoft/sarif-sdk/issues/1581

## **v2.1.13** [Sdk](https://www.nuget.org/packages/Sarif.Sdk/2.1.13) | [Driver](https://www.nuget.org/packages/Sarif.Driver/2.1.13) | [Converters](https://www.nuget.org/packages/Sarif.Converters/2.1.13) | [Multitool](https://www.nuget.org/packages/Sarif.Multitool/2.1.13)
* BUGFIX: Respect the --force option in Sarif.Multitool rather than overwriting the output file. https://github.com/microsoft/sarif-sdk/issues/1304
* BUGFIX: Respect the --force option in Sarif.Multitool rather than overwriting the output file. https://github.com/microsoft/sarif-sdk/issues/1340
* BUGFIX: Accept URI-valued properties whose value is the empty string. https://github.com/microsoft/sarif-sdk/issues/1632

## **v2.1.14** [Sdk](https://www.nuget.org/packages/Sarif.Sdk/2.1.14) | [Driver](https://www.nuget.org/packages/Sarif.Driver/2.1.14) | [Converters](https://www.nuget.org/packages/Sarif.Converters/2.1.14) | [Multitool](https://www.nuget.org/packages/Sarif.Multitool/2.1.14)
Expand All @@ -445,4 +445,5 @@
* FEATURE: Add validation rule to ensure that all array-index-valued properties are consistent with their respective arrays.

** **v2.1.15** [Sdk](https://www.nuget.org/packages/Sarif.Sdk/2.1.15) | [Driver](https://www.nuget.org/packages/Sarif.Driver/2.1.15) | [Converters](https://www.nuget.org/packages/Sarif.Converters/2.1.15) | [Multitool](https://www.nuget.org/packages/Sarif.Multitool/2.1.15)
* BUGFIX: FxCop converter produced empty `result.message` objects . https://github.com/microsoft/sarif-sdk/issues/1594
* BUGFIX: FortifyFpr converter produced invalid SARIF. https://github.com/microsoft/sarif-sdk/issues/1593
* BUGFIX: FxCop converter produced empty `result.message` objects. https://github.com/microsoft/sarif-sdk/issues/1594
87 changes: 50 additions & 37 deletions src/Sarif.Converters/FortifyFprConverter.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,10 @@ internal class FortifyFprConverter : ToolFileConverterBase
private readonly NameTable _nameTable;
private readonly FortifyFprStrings _strings;
private readonly string[] SupportedReplacementTokens = new[] { "PrimaryLocation.file", "PrimaryLocation.line" };
private readonly Dictionary<string, List<string>> ActionTypeToLocationKindMap = new Dictionary<string, List<string>>
private readonly Dictionary<string, List<string>> ActionTypeToLocationKindsMap = new Dictionary<string, List<string>>
{
{ "InCall", new List<string> { "call", "function" } },
{ "InOutCall", new List<string> { "call", "function", "return" , "function"} },
{ "InOutCall", new List<string> { "call", "function", "return" } },
{ "BranchTaken", new List<string> { "branch", "true" } },
{ "BranchNotTaken", new List<string> { "branch", "false" } },
{ "Return", new List<string> { "return", "function" } },
Expand All @@ -38,7 +38,7 @@ internal class FortifyFprConverter : ToolFileConverterBase
private readonly ToolComponent CweToolComponent = new ToolComponent
{
Name = "CWE",
Guid = "2B841697-D0DE-45DD-9F19-1EEE1312429",
Guid = "25F72D7E-8A92-459D-AD67-64853F788765",
Organization = "MITRE",
ShortDescription = new MultiformatMessageString
{
Expand All @@ -63,6 +63,7 @@ internal class FortifyFprConverter : ToolFileConverterBase
private List<ReportingDescriptor> _rules;
private Dictionary<string, int> _ruleIdToIndexMap;
private Dictionary<string, Node> _nodeDictionary;
private IDictionary<ThreadFlowLocation, int> _threadFlowLocationToIndexDictionary;
private Dictionary<string, Snippet> _snippetDictionary;

// Output Configurability
Expand All @@ -82,14 +83,15 @@ public FortifyFprConverter()
_ruleIdToIndexMap = new Dictionary<string, int>();
_cweIds = new HashSet<string>();
_nodeDictionary = new Dictionary<string, Node>();
_threadFlowLocationToIndexDictionary = new Dictionary<ThreadFlowLocation, int>(ThreadFlowLocation.ValueComparer);
_snippetDictionary = new Dictionary<string, Snippet>();

IncludeContextRegions = true;
IncludeSnippets = true;
IncludeThreadFlowLocations = true;
}

public override string ToolName => "HP Fortify Static Code Analyzer";
public override string ToolName => "Micro Focus Fortify Static Code Analyzer";

/// <summary>
/// Interface implementation for converting a stream in Fortify FPR format to a stream in
Expand Down Expand Up @@ -119,6 +121,7 @@ public override void Convert(Stream input, IResultLogWriter output, OptionallyEm
_ruleIdToIndexMap.Clear();
_cweIds.Clear();
_nodeDictionary.Clear();
_threadFlowLocationToIndexDictionary.Clear();
_snippetDictionary.Clear();

// Uncomment following Line to fetch FVDL content from any FPR file. This is not needed for conversion.
Expand All @@ -131,7 +134,11 @@ public override void Convert(Stream input, IResultLogWriter output, OptionallyEm
// Add Snippets to NodePool Nodes which referenced them (Snippets appear after the NodePool in Fortify files)
AddSnippetsToNodes();

// Create the Sarif Run itself to report
// Now that the ThreadFlowLocation objects are fully constructed (including their code
// snippets), we can identify the set of distinct ThreadFlowLocations and associate
// each one with its array index in run.threadFlowLocations.
AssociateIndicesWithThreadFlowLocations();

Run run = CreateSarifRun();

// Write the Run itself
Expand All @@ -147,14 +154,12 @@ public override void Convert(Stream input, IResultLogWriter output, OptionallyEm

private string ExtractFvdl(XmlReader reader)
{
string output;
using (reader)
{
// Moves the reader to the root element.
reader.MoveToContent();
output = reader.ReadOuterXml();
return reader.ReadOuterXml();
}
return output;
}

private Run CreateSarifRun()
Expand Down Expand Up @@ -182,7 +187,7 @@ private Run CreateSarifRun()
{
Name = "CWE",
Index = 0,
Guid = "2B841697-D0DE-45DD-9F19-1EEE1312429"
Guid = CweToolComponent.Guid
}
}
}
Expand All @@ -194,15 +199,12 @@ private Run CreateSarifRun()
Invocations = new[] { _invocation },
};

// Note: Serialize ThreadFlowLocations from the 'UnifiedNodePool' to maintain same reuse as Fortify log
if (_nodeDictionary?.Count > 0 && IncludeThreadFlowLocations)
{
run.ThreadFlowLocations = new List<ThreadFlowLocation>();
foreach (Node node in _nodeDictionary.Values)
{
node.Index = run.ThreadFlowLocations.Count;
run.ThreadFlowLocations.Add(node.ThreadFlowLocation);
}
run.ThreadFlowLocations = _threadFlowLocationToIndexDictionary
.OrderBy(entry => entry.Value)
.Select(entry => entry.Key)
.ToList();
}

if (_cweIds.Count > 0)
Expand Down Expand Up @@ -531,7 +533,6 @@ internal static FailureLevel GetFailureLevelFromRuleMetadata(ReportingDescriptor

private void ParseLocationsFromTraces(Result result)
{
CodeFlow codeFlow = null;
string nodeLabel = null;
string lastNodeId = null;
bool? isDefault = null;
Expand All @@ -545,7 +546,7 @@ private void ParseLocationsFromTraces(Result result)
result.CodeFlows = new List<CodeFlow>();
}

codeFlow = SarifUtilities.CreateSingleThreadedCodeFlow();
CodeFlow codeFlow = SarifUtilities.CreateSingleThreadedCodeFlow();
result.CodeFlows.Add(codeFlow);

while (!AtEndOf(_strings.Trace))
Expand All @@ -558,8 +559,8 @@ private void ParseLocationsFromTraces(Result result)
{
if (_nodeDictionary.TryGetValue(nodeId, out Node node))
{
// Add a ThreadFlowLocation referencing the global set, mimicing the reuse within the Fortify format
codeFlow.ThreadFlows[0].Locations.Add(new ThreadFlowLocation() { Index = node.Index });
int index = _threadFlowLocationToIndexDictionary[node.ThreadFlowLocation];
codeFlow.ThreadFlows[0].Locations.Add(new ThreadFlowLocation { Index = index });
}
}

Expand Down Expand Up @@ -724,19 +725,19 @@ private PhysicalLocation ParsePhysicalLocationFromSourceInfo()
{
string path = _reader.GetAttribute(_strings.PathAttribute);

PhysicalLocation location = new PhysicalLocation()
PhysicalLocation location = new PhysicalLocation
{
Region = ParseRegion()
};

var uri = new Uri(path, UriKind.RelativeOrAbsolute);
if (_files.TryGetValue(uri, out var entry))
{
location.ArtifactLocation = new ArtifactLocation() { Index = entry.Item2 };
location.ArtifactLocation = new ArtifactLocation { Index = entry.Item2 };
}
else
{
location.ArtifactLocation = new ArtifactLocation() { Uri = uri };
location.ArtifactLocation = new ArtifactLocation { Uri = uri };
}

return location;
Expand Down Expand Up @@ -924,14 +925,14 @@ private void ParseNode()
string actionType = _reader.GetAttribute(_strings.TypeAttribute);
string nodeLabel = _reader.ReadElementContentAsString();

// Convert to Sarif types
// Convert to SARIF types.
Node node = new Node(
new ThreadFlowLocation()
new ThreadFlowLocation
{
Kinds = ConvertActionTypeToLocationKinds(actionType),
Location = new Location()
Location = new Location
{
Message = new Message() { Text = nodeLabel },
Message = new Message { Text = nodeLabel },
PhysicalLocation = physicalLocation
}
},
Expand Down Expand Up @@ -969,13 +970,11 @@ private void ParseSnippet()
string snippetId = _reader.GetAttribute(_strings.IdAttribute);
int snippetStartLine = 0;
int snippetEndLine = 0;
int regionStartLine = 0;
int regionEndLine = 0;

string[] parts = snippetId.Split(':');

int.TryParse(parts[parts.Length - 2], out regionStartLine);
int.TryParse(parts[parts.Length - 1], out regionEndLine);
int.TryParse(parts[parts.Length - 2], out int regionStartLine);
int.TryParse(parts[parts.Length - 1], out int regionEndLine);
string text = null;

_reader.Read();
Expand Down Expand Up @@ -1135,7 +1134,11 @@ private ReportingDescriptor FindOrCreateRule(string ruleId, out int ruleIndex)
}
else
{
ReportingDescriptor rule = new ReportingDescriptor() { Guid = ruleId };
ReportingDescriptor rule = new ReportingDescriptor
{
Id = ruleId,
Guid = ruleId
};

ruleIndex = _rules.Count;
_rules.Add(rule);
Expand All @@ -1149,7 +1152,7 @@ private List<string> ConvertActionTypeToLocationKinds(string actionType)
{
List<string> kinds;

if (actionType == null || !ActionTypeToLocationKindMap.TryGetValue(actionType, out kinds))
if (actionType == null || !ActionTypeToLocationKindsMap.TryGetValue(actionType, out kinds))
{
kinds = new List<string> { "unknown" };
}
Expand Down Expand Up @@ -1233,15 +1236,26 @@ private void AddSnippetsToNodes()

foreach (Node node in _nodeDictionary.Values)
{
if (!String.IsNullOrEmpty(node.SnippetId) && _snippetDictionary.TryGetValue(node.SnippetId, out Snippet snippet))
if (!string.IsNullOrEmpty(node.SnippetId) && _snippetDictionary.TryGetValue(node.SnippetId, out Snippet snippet))
{
snippet.ApplyTo(node.ThreadFlowLocation.Location.PhysicalLocation);
}
}
}

private void AssociateIndicesWithThreadFlowLocations()
{
foreach (Node node in _nodeDictionary.Values)
{
if (!_threadFlowLocationToIndexDictionary.TryGetValue(node.ThreadFlowLocation, out _))
{
_threadFlowLocationToIndexDictionary.Add(node.ThreadFlowLocation, _threadFlowLocationToIndexDictionary.Count);
}
}
}

/// <summary>
/// Represents a Snippet in the Fortify format converted to Sarif types.
/// Represents a Snippet in the Fortify format converted to SARIF types.
/// </summary>
private class Snippet
{
Expand All @@ -1262,13 +1276,12 @@ public void ApplyTo(PhysicalLocation physicalLocation)
}

/// <summary>
/// Represents a Node in the Fortify format converted to Sarif types
/// Represents a Node in the Fortify format converted to SARIF types.
/// </summary>
private class Node
{
public ThreadFlowLocation ThreadFlowLocation { get; set; }
public string SnippetId { get; set; }
public int Index { get; set; }

public Node(ThreadFlowLocation tfl, string snippetId)
{
Expand Down
Loading

0 comments on commit 8bc1e45

Please sign in to comment.