Skip to content

C#: New query VulnerablePackage #335

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

Merged
merged 7 commits into from
Nov 12, 2018
Merged

Conversation

calumgrant
Copy link
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
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
Contributor Author

@jf205 That would probably be best.

Copy link
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.

<overview>
<p>
Using a package with a known vulnerability is a security risk that could leave the
software vulnerable to attack.
Copy link
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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