Skip to content

C#: New query VulnerablePackage#335

Merged
hvitved merged 7 commits into
github:masterfrom
calumgrant:cs/cwe-937
Nov 12, 2018
Merged

C#: New query VulnerablePackage#335
hvitved merged 7 commits into
github:masterfrom
calumgrant:cs/cwe-937

Conversation

@calumgrant
Copy link
Copy Markdown
Contributor

This query finds vulnerable packages imported in project or config files.

Vulnerabilities are described in QL, and the design is extensible to make it straightforward to add new CVEs.

Autobuilder has been changed to also index .csproj and .props files as XML.

An initial version of this query attempted to use the paths of imported assemblies, but this had the disadvantage that it was hard to report the location of the error and it is often hard to track down the root cause of the import. This new version reports the XML element that caused the import, which is more actionable and easier to test.

@calumgrant calumgrant added the C# label Oct 18, 2018
@calumgrant calumgrant requested review from a team October 18, 2018 09:40
@jf205
Copy link
Copy Markdown
Contributor

jf205 commented Oct 18, 2018

@calumgrant I'll have a look at this. Shall I wait for the technical review to be completed first?

@calumgrant
Copy link
Copy Markdown
Contributor Author

@jf205 That would probably be best.

Copy link
Copy Markdown
Contributor

@hvitved hvitved left a comment

Choose a reason for hiding this comment

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

Very nice. My only concern is that we have to keep Vulnerabilities.qll up-to-date manually.

Note: I did not check all vulnerability version numbers in detail.

Comment thread csharp/extractor/Semmle.Autobuild/XmlBuildRule.cs Outdated
Comment thread csharp/ql/src/Security Features/CWE-937/Vulnerabilities.qll Outdated
Comment thread csharp/ql/src/Security Features/CWE-937/Vulnerability.qll Outdated
Comment thread csharp/ql/src/Security Features/CWE-937/Vulnerability.qll Outdated
Comment thread csharp/ql/src/Security Features/CWE-937/Vulnerability.qll Outdated
Comment thread csharp/ql/src/Security Features/CWE-937/Vulnerability.qll Outdated
Comment thread csharp/ql/src/Security Features/CWE-937/Vulnerabilities.qll
<overview>
<p>
Using a package with a known vulnerability is a security risk that could leave the
software vulnerable to attack.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

an attack or attacks?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think you could use any of the above suggestions–my preference would be to leave as it is though.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think you could use any of the above suggestions–my preference would be to leave as it is though.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Any of the above would work I think–my preference would be to leave as is.


from Vulnerability vuln, VulnerablePackage package
where vuln = package.getVulnerability()
select package, "Package " + package + " has vulnerability $@, and should be upgraded to version " + package.getFixedVersion() + ".",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

add ' around package

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Have you checked that the links to vuln.getUrl() render correctly in both QL4E and on LGTM.com?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@calumgrant : Did you check this?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If the above does not work, I think something like this should:

"Package " + package + " has vulnerability [[\"" + vuln + "\"|\"" +vuln.getUrl()+ "\"]], and should be upgraded to version " + package.getFixedVersion() + "."

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If the above does not work, I think something like this should:

"Package " + package + " has vulnerability [[\"" + vuln + "\"|\"" +vuln.getUrl()+ "\"]], and should be upgraded to version " + package.getFixedVersion() + "."

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If the above does not work, I think something like this should:

"Package " + package + " has vulnerability [[\"" + vuln + "\"|\"" +vuln.getUrl()+ "\"]], and should be upgraded to version " + package.getFixedVersion() + "."

Copy link
Copy Markdown
Contributor

@jf205 jf205 left a comment

Choose a reason for hiding this comment

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

One very minor suggestion, otherwise LGTM.

One question: does this query need to be added to a standard suite?

<overview>
<p>
Using a package with a known vulnerability is a security risk that could leave the
software vulnerable to attack.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think you could use any of the above suggestions–my preference would be to leave as it is though.

<overview>
<p>
Using a package with a known vulnerability is a security risk that could leave the
software vulnerable to attack.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think you could use any of the above suggestions–my preference would be to leave as it is though.

<recommendation>
<p>
Upgrade the package to the recommended version, for example using the NuGet package manager,
or by editing the project files directly.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggest very minor rewording here:
Upgrade the package to the recommended version, using, for example, the NuGet package manager, or by editing the project files directly.

<recommendation>
<p>
Upgrade the package to the recommended version, for example using the NuGet package manager,
or by editing the project files directly.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggest very minor rewording here:
Upgrade the package to the recommended version, using, for example, the NuGet package manager, or by editing the project files directly.

Copy link
Copy Markdown
Contributor

@jf205 jf205 left a comment

Choose a reason for hiding this comment

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

One very minor suggestion, otherwise LGTM.

One question: does this query need to be added to a standard suite?

<overview>
<p>
Using a package with a known vulnerability is a security risk that could leave the
software vulnerable to attack.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Any of the above would work I think–my preference would be to leave as is.

<recommendation>
<p>
Upgrade the package to the recommended version, for example using the NuGet package manager,
or by editing the project files directly.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggest a very minor re-wording here:
Upgrade the package to the recommended version using, for example, the NuGet package manager, or by editing the project files directly.

<recommendation>
<p>
Upgrade the package to the recommended version, for example using the NuGet package manager,
or by editing the project files directly.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggest a very minor re-wording here:
Upgrade the package to the recommended version using, for example, the NuGet package manager, or by editing the project files directly.

@hvitved hvitved merged commit dd6fd40 into github:master Nov 12, 2018
aibaars pushed a commit that referenced this pull request Oct 22, 2021
smowton pushed a commit to smowton/codeql that referenced this pull request Apr 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants