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

Basic rules transformation (except for v1 <-> v2 conversion) #1223

Merged
merged 3 commits into from
Jan 20, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
"results": [
{
"ruleId": "31D4607A-A3FF-447C-908A-CA2BBE4CE4B7",
"ruleIndex": 0,
"message": {
"text": "Sample abstract text. SQL injection vulnerability."
},
Expand Down Expand Up @@ -127,8 +128,8 @@
}
],
"resources": {
"rules": {
"31D4607A-A3FF-447C-908A-CA2BBE4CE4B7": {
"rules": [
{
"id": "31D4607A-A3FF-447C-908A-CA2BBE4CE4B7",
"shortDescription": {
"text": "Sample abstract text. SQL injection vulnerability."
Expand All @@ -137,7 +138,7 @@
"text": "The quick brown fox jumps over the lazy dog.\nThis section explains the rule in detail."
}
}
}
]
}
}
]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
"results": [
{
"ruleId": "97A5F38F-524B-4A83-94FA-9387B5265825",
"ruleIndex": 0,
"message": {
"text": "The function getVSTSDetailsForICTO() in [DBHelper.cs](1) sometimes fails to release a system resource allocated by ExecuteReader() on line 146.\r\nThe program can potentially fail to release a system resource."
},
Expand Down Expand Up @@ -385,8 +386,8 @@
}
],
"resources": {
"rules": {
"97A5F38F-524B-4A83-94FA-9387B5265825": {
"rules": [
{
"id": "97A5F38F-524B-4A83-94FA-9387B5265825",
"shortDescription": {
"text": "The function <Replace key=\"EnclosingFunction.name\"/> in <Replace key=\"PrimaryLocation.file\"/> sometimes fails to release a system resource allocated by <Replace key=\"FirstTransitionFunction\"/> on line <Replace key=\"FirstTraceLocation.line\"/>.\r\nThe program can potentially fail to release a system resource."
Expand All @@ -395,7 +396,7 @@
"text": "The program can potentially fail to release a system resource.\r\n\r\nResource leaks have at least two common causes:\r\n\r\n- Error conditions and other exceptional circumstances.\r\n\r\n- Confusion over which part of the program is responsible for releasing the resource.\r\n\r\nIn this case, there are program paths on which the resource allocated in <Replace key=\"FirstTraceLocation.file\"/> at line <Replace key=\"FirstTraceLocation.line\"/> is not released.\r\n\r\nMost unreleased resource issues result in general software reliability problems, but if an attacker can intentionally trigger a resource leak, the attacker may be able to launch a denial of service attack by depleting the resource pool.\r\n\r\n**Example:** Under normal conditions the following code executes a database query, processes the results returned by the database, and closes the allocated `SqlConnection` object. But if an exception occurs while executing the SQL or processing the results, the `SqlConnection` object will not be closed. If this happens often enough, the database will run out of available cursors and not be able to execute any more SQL queries.\r\n\r\n`\n ...\n SqlConnection conn = new SqlConnection(connString);\n SqlCommand cmd = new SqlCommand(queryString);\n cmd.Connection = conn;\n conn.Open();\n SqlDataReader rdr = cmd.ExecuteReader();\n HarvestResults(rdr);\n conn.Connection.Close();\n ...\n`"
}
}
}
]
}
}
]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
"results": [
{
"ruleId": "31D4607A-A3FF-447C-908A-CA2BBE4CE4B7",
"ruleIndex": 0,
"message": {
"text": "Sample abstract text. SQL injection vulnerability."
},
Expand Down Expand Up @@ -115,6 +116,7 @@
},
{
"ruleId": "F7A2423A-1927-40A3-920E-17ADB5430412",
"ruleIndex": 1,
"message": {
"text": "The function WriteLog() in [Logger.cs](1) reveals system data or debugging information by calling WriteLine() on line [65](1). The information revealed by WriteLine() could help an adversary form a plan of attack.\r\nRevealing system data or debugging information helps an adversary learn about the system and form a plan of attack."
},
Expand Down Expand Up @@ -374,8 +376,8 @@
}
],
"resources": {
"rules": {
"31D4607A-A3FF-447C-908A-CA2BBE4CE4B7": {
"rules": [
{
"id": "31D4607A-A3FF-447C-908A-CA2BBE4CE4B7",
"shortDescription": {
"text": "Sample abstract text. SQL injection vulnerability."
Expand All @@ -384,7 +386,7 @@
"text": "The quick brown fox jumps over the lazy dog.\nThis section explains the rule in detail."
}
},
"F7A2423A-1927-40A3-920E-17ADB5430412": {
{
"id": "F7A2423A-1927-40A3-920E-17ADB5430412",
"shortDescription": {
"text": "The function <Replace key=\"EnclosingFunction.name\"/> in <Replace key=\"PrimaryLocation.file\"/> reveals system data or debugging information by calling <Replace key=\"PrimaryCall.name\"/> on line <Replace key=\"PrimaryLocation.line\"/>. The information revealed by <Replace key=\"PrimaryCall.name\"/> could help an adversary form a plan of attack.\r\nRevealing system data or debugging information helps an adversary learn about the system and form a plan of attack."
Expand All @@ -393,7 +395,7 @@
"text": "An information leak occurs when system data or debugging information leaves the program through an output stream or logging function.\r\n\r\nIn this case the data from <Replace key=\"SourceFunction\" link=\"SourceLocation\"/> in <Replace key=\"SourceLocation.file\"/> at line <Replace key=\"SourceLocation.line\"/> leaves the program through <Replace key=\"SinkFunction\" link=\"SinkLocation\"/> in <Replace key=\"SinkLocation.file\"/> at line <Replace key=\"SinkLocation.line\"/>.\r\n\r\n**Example:** The following code constructs a database connection string, uses it to create a new connection to the database, and prints it to the console.\r\n\r\n`\nstring cs=\"database=northwind;server=mySQLServer...\";\nSqlConnection conn=new SqlConnection(cs);\n...\nConsole.Writeline(cs);\n`\r\n\r\nDepending on the system configuration, this information can be dumped to a console, written to a log file, or exposed to a remote user. For example, with scripting mechanisms it is trivial to redirect output information from &quot;Standard error&quot; or &quot;Standard output&quot; into a file or another program. Alternatively the system that the program runs on could have a remote logging mechanism such as a &quot;syslog&quot; server that will send the logs to a remote device. During development you will have no way of knowing where this information may end up being displayed.\r\n\r\nIn some cases the error message tells the attacker precisely what sort of an attack the system is vulnerable to. For example, a database error message can reveal that the application is vulnerable to a SQL injection attack. Other error messages can reveal more oblique clues about the system. In the example above, the leaked information could imply information about the type of operating system, the applications installed on the system, and the amount of care that the administrators have put into configuring the program."
}
}
}
]
}
}
]
Expand Down
68 changes: 38 additions & 30 deletions src/Sarif.Converters/FortifyFprConverter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@ internal class FortifyFprConverter : ToolFileConverterBase
private string _originalUriBasePath;
private List<Result> _results = new List<Result>();
private HashSet<FileData> _files;
private Dictionary<string, Rule> _ruleDictionary;
private List<Rule> _rules;
private Dictionary<string, int> _ruleIdToIndexMap;
Copy link

@ghost ghost Jan 18, 2019

Choose a reason for hiding this comment

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

_ruleIdToIndexMap [](start = 40, length = 17)

Note to self: This only works if Fortify doesn't have rule collisions (if it did, a single rule id could map to multiple indices). #Closed

private Dictionary<ThreadFlowLocation, string> _tflToNodeIdDictionary;
private Dictionary<ThreadFlowLocation, string> _tflToSnippetIdDictionary;
private Dictionary<Location, string> _locationToSnippetIdDictionary;
Expand All @@ -50,7 +51,8 @@ public FortifyFprConverter()

_results = new List<Result>();
_files = new HashSet<FileData>(FileData.ValueComparer);
_ruleDictionary = new Dictionary<string, Rule>();
_rules = new List<Rule>();
_ruleIdToIndexMap = new Dictionary<string, int>();
_tflToNodeIdDictionary = new Dictionary<ThreadFlowLocation, string>();
_tflToSnippetIdDictionary = new Dictionary<ThreadFlowLocation, string>();
_locationToSnippetIdDictionary = new Dictionary<Location, string>();
Expand Down Expand Up @@ -90,7 +92,8 @@ public override void Convert(Stream input, IResultLogWriter output, OptionallyEm
_invocation.ToolNotifications = new List<Notification>();
_results.Clear();
_files.Clear();
_ruleDictionary.Clear();
_rules.Clear();
_ruleIdToIndexMap.Clear();
_tflToNodeIdDictionary.Clear();
_tflToSnippetIdDictionary.Clear();
_locationToSnippetIdDictionary.Clear();
Expand All @@ -116,7 +119,7 @@ public override void Convert(Stream input, IResultLogWriter output, OptionallyEm
Files = new List<FileData>(_files),
Tool = tool,
Invocations = new[] { _invocation },
Resources = new Resources { Rules = _ruleDictionary}
Resources = new Resources { Rules = _rules }
};

if (!string.IsNullOrWhiteSpace(_originalUriBasePath))
Expand Down Expand Up @@ -342,6 +345,11 @@ private void ParseVulnerability()
if (AtStartOfNonEmpty(_strings.ClassId))
{
result.RuleId = _reader.ReadElementContentAsString();

if (_ruleIdToIndexMap.TryGetValue(result.RuleId, out int ruleIndex))
{
result.RuleIndex = ruleIndex;
}
}
else if (AtStartOfNonEmpty(_strings.ReplacementDefinitions))
{
Expand Down Expand Up @@ -661,7 +669,8 @@ private void ParseRuleFromDescription()
}
}

_ruleDictionary.Add(rule.Id, rule);
_ruleIdToIndexMap[rule.Id] = _ruleIdToIndexMap.Count;
_rules.Add(rule);
}

private void ParseNodes()
Expand Down Expand Up @@ -915,38 +924,37 @@ private void AddMessagesToResults()
{
foreach (Result result in _results)
{
Rule rule;
if (_ruleDictionary.TryGetValue(result.RuleId, out rule))
{
Message message = rule.ShortDescription ?? rule.FullDescription;
Dictionary<string, string> replacements = null;
int ruleIndex = _ruleIdToIndexMap[result.RuleId];
Copy link

@ghost ghost Jan 18, 2019

Choose a reason for hiding this comment

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

RuleId [](start = 57, length = 6)

Don't you need a test similar to the old code, where you do TryGetValue on _ruleIdToIndexMap? #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

don't believe so. i think the previous test was unnecessary, as the fortify report consistently contains rules metadata. will verify.


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

Copy link
Member Author

Choose a reason for hiding this comment

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

verified this data s.be present, no need for check


In reply to: 249147574 [](ancestors = 249147574,249143447)

result.RuleIndex = ruleIndex;

Rule rule = _rules[ruleIndex];
Message message = rule.ShortDescription ?? rule.FullDescription;
Dictionary<string, string> replacements = null;

if (_resultToReplacementDefinitionDictionary.TryGetValue(result, out replacements))
if (_resultToReplacementDefinitionDictionary.TryGetValue(result, out replacements))
{
string messageText = message?.Text;
foreach (string key in replacements.Keys)
{
string messageText = message?.Text;
foreach (string key in replacements.Keys)
{
string value = replacements[key];
string value = replacements[key];

if (SupportedReplacementTokens.Contains(key))
{
// Replace the token with an embedded hyperlink
messageText = messageText.Replace(string.Format(ReplacementTokenFormat, key),
string.Format(EmbeddedLinkFormat, value));
}
else
{
// Replace the token with plain text
messageText = messageText.Replace(string.Format(ReplacementTokenFormat, key), value);
}
if (SupportedReplacementTokens.Contains(key))
{
// Replace the token with an embedded hyperlink
messageText = messageText.Replace(string.Format(ReplacementTokenFormat, key),
string.Format(EmbeddedLinkFormat, value));
}
else
{
// Replace the token with plain text
messageText = messageText.Replace(string.Format(ReplacementTokenFormat, key), value);
}

message = message.DeepClone();
message.Text = messageText;
}

result.Message = message;
message = message.DeepClone();
message.Text = messageText;
}
result.Message = message;
}
}

Expand Down
9 changes: 1 addition & 8 deletions src/Sarif.Converters/FxCopConverter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -59,16 +59,9 @@ public override void Convert(Stream input, IResultLogWriter output, OptionallyEm

if (rules.Count > 0)
{
IDictionary<string, Rule> rulesDictionary = new Dictionary<string, Rule>();

foreach (Rule rule in rules)
{
rulesDictionary[rule.Id] = rule;
}

run.Resources = new Resources
{
Rules = rulesDictionary
Rules = rules
Copy link

@ghost ghost Jan 18, 2019

Choose a reason for hiding this comment

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

rules [](start = 28, length = 5)

Well, that's a nice simple change! #ByDesign

Copy link
Member Author

Choose a reason for hiding this comment

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

:)


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

};
}

Expand Down
7 changes: 2 additions & 5 deletions src/Sarif.Driver/Sdk/ExportRulesMetadataCommandBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ private void OutputSarifRulesMetada(string outputFilePath, ImmutableArray<IRule>
log.Runs.Add(run);
run.Resources = new Resources
{
Rules = new Dictionary<string, Rule>()
Rules = new List<Rule>()
};

SortedDictionary<int, Rule> sortedRules = new SortedDictionary<int, Rule>();
Expand Down Expand Up @@ -147,10 +147,7 @@ private void OutputSarifRulesMetada(string outputFilePath, ImmutableArray<IRule>
sortedRules[numericId] = newRule;
}

foreach (Rule rule in sortedRules.Values)
{
run.Resources.Rules[rule.Id] = rule;
}
run.Resources.Rules = new List<Rule>(sortedRules.Values);

var settings = new JsonSerializerSettings()
{
Expand Down
Loading