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

Add leniancy to the version matching for debian to account for versio… #328

Conversation

jsulinski
Copy link
Contributor

…ns without the "+" when package maintainers aren't using them.

#327

@jsulinski jsulinski force-pushed the leniant_changelog_parsing_for_debian branch 2 times, most recently from 26e9d11 to 9dbd270 Compare February 10, 2017 07:08
scan/debian.go Outdated
@@ -592,19 +592,28 @@ func (o *debian) parseChangelog(changelog string,
cveRe := regexp.MustCompile(`(CVE-\d{4}-\d{4,})`)
stopRe := regexp.MustCompile(fmt.Sprintf(`\(%s\)`, regexp.QuoteMeta(versionOrLater)))
stopLineFound := false
leniantStopeLineFound := false
Copy link

Choose a reason for hiding this comment

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

Should be leniantStopLineFound := false?

…ns without the "+" when package maintainers aren't using them.
@jsulinski jsulinski force-pushed the leniant_changelog_parsing_for_debian branch from 9dbd270 to 9816315 Compare February 10, 2017 19:39
@jsulinski
Copy link
Contributor Author

Thanks, fixed.

@kotakanbe kotakanbe added the bug label Feb 14, 2017
@kotakanbe
Copy link
Member

kotakanbe commented Feb 14, 2017

Applying this P / R may report a false positive of the detected vulnerability.

https://readme.phys.ethz.ch/documentation/debian_version_numbers/

+nmu: Non-maintainer upload (NMU) of a native package

    123+nmu3 = Third non-maintainer upload based on package version 123

Note: The versions of dpkg and changelog do not match

Example:

  • dpkg
admin@ip-172-31-7-15:~$ dpkg-query -W tar
tar     1.27.1-2+b1
  • changelog
$ aptitude changelog tar | grep '\(urgency\|CVE\)'
tar (1.27.1-2+deb8u1) jessie-security; urgency=high
  * CVE-2016-6321: Bypassing the extract path name.
tar (1.27.1-2) unstable; urgency=low

In the current implementation, since it is detected by version complete match in changelog, Vuls does not detect it when the version of dpkg is not match the version in changelog.

For this reason, there is no false positive in this case.

After applying this P / R, since it matches the string before +, there is a possibility that false positive may occur.

@jsulinski
Copy link
Contributor Author

That is correct, which is why adding a configuration options for lenient version matching, and adding information to the output about lenient version matching are important.

Keep in mind that this is only checked when version matching fails -- so the options are to either report no vulnerabilities when infact vulnerabilities may exist, or report potential false positives. I'd rather opt for the latter, personally.

Alternatively, we can work with the package maintainers to resolve this issue directly and not add complexity to the vuls code. This may be a better option.

@kotakanbe kotakanbe removed the Doing label Feb 15, 2017
@kotakanbe
Copy link
Member

kotakanbe commented Feb 15, 2017

I think that it is better to indicate in the JSON and Reporting that this vulnerability is not detected by accurate version matching.
I will implement this function. I will Merge this P/R after implementation.

@kotakanbe
Copy link
Member

I will merge this P/R since additional implementation is complete.
#350

@kotakanbe kotakanbe merged commit 1d3ee6a into future-architect:master Feb 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants