improve ruby comparison version check. #552

rahul2393 · 2020-07-05T17:11:27Z

Issue

Improved version checking and added tests

pkg/scanner/utils/utils_test.go

pkg/scanner/utils/utils.go

pkg/scanner/utils/utils_test.go

pkg/scanner/utils/utils.go

knqyf263 · 2020-07-15T06:08:57Z

It looks like Gem has a version with 3 dot such as 1.6.7.1 and this library can't handle such a version. As you can see, it detects CVE-2015-5312 in nokogiri, but 1.10.9-java seems newer than 1.6.7.1.

$ ./trivy image --vuln-type library logstash:7.8.0
2020-07-15T09:05:38.332+0300    INFO    Detecting bundler vulnerabilities...
2020-07-15T09:05:38.334+0300    ERROR   NewConstrainterrorimproper constraint: >= 1.6.6.0
2020-07-15T09:05:38.334+0300    ERROR   NewConstrainterrorimproper constraint:  < 1.6.7.1
2020-07-15T09:05:38.334+0300    ERROR   NewConstrainterrorimproper constraint:  < 1.6.7.2
2020-07-15T09:05:38.335+0300    ERROR   NewConstrainterrorimproper constraint: ~> 1.6.6.4
2020-07-15T09:05:38.335+0300    ERROR   NewConstrainterrorimproper constraint: >= 1.6.7.1
2020-07-15T09:05:38.335+0300    ERROR   NewConstrainterrorimproper constraint: >= 1.6.7.2

usr/share/logstash/Gemfile.lock
===============================
Total: 6 (UNKNOWN: 0, LOW: 0, MEDIUM: 4, HIGH: 2, CRITICAL: 0)

+----------+------------------+----------+-------------------+---------------+--------------------------------+
| LIBRARY  | VULNERABILITY ID | SEVERITY | INSTALLED VERSION | FIXED VERSION |             TITLE              |
+----------+------------------+----------+-------------------+---------------+--------------------------------+
| json     | CVE-2020-10663   | MEDIUM   | 1.8.6-java        | >= 2.3.0      | rubygem-json: Unsafe Object    |
|          |                  |          |                   |               | Creation Vulnerability in JSON |
+----------+------------------+----------+-------------------+---------------+--------------------------------+
| nokogiri | CVE-2015-5312    | HIGH     | 1.10.9-java       | >= 1.6.7.1    | libxml2: CPU exhaustion when   |
|          |                  |          |                   |               | processing specially crafted   |
|          |                  |          |                   |               | XML input                      |
+          +------------------+----------+                   +---------------+--------------------------------+
|          | CVE-2015-7499    | MEDIUM   |                   | >= 1.6.7.2    | libxml2: Heap-based buffer     |
|          |                  |          |                   |               | overflow in xmlGROW            |
+----------+------------------+----------+-------------------+---------------+--------------------------------+
| puma     | CVE-2020-11076   | HIGH     | 4.3.3-java        | 4.3.4, 3.12.5 | rubygem-puma: HTTP             |
|          |                  |          |                   |               | Smuggling via an invalid       |
|          |                  |          |                   |               | Transfer-Encoding Header       |
+          +------------------+----------+                   +---------------+--------------------------------+
|          | CVE-2020-11077   | MEDIUM   |                   | 4.3.5, 3.12.6 | rubygem-puma: HTTP Smuggling   |
|          |                  |          |                   |               | through a proxy via            |
|          |                  |          |                   |               | Transfer-Encoding Header       |
+----------+------------------+          +-------------------+---------------+--------------------------------+
| rack     | CVE-2020-8184    |          | 2.2.2             | 2.2.3, 2.1.4  | rubygem-rack: percent-encoded  |
|          |                  |          |                   |               | cookies can be used to         |
|          |                  |          |                   |               | overwrite existing prefixed    |
|          |                  |          |                   |               | cookie names...                |
+----------+------------------+----------+-------------------+---------------+--------------------------------+

rahul2393 · 2020-07-15T09:29:12Z

@knqyf263 Added logic to replace all occurrences of dots with "-" in patch version since it seems like Ruby having 3 dots is language-specific, all other languages put the dash in patch versioning. After it the code working fine for Ruby gems too, check tests
https://github.com/aquasecurity/trivy/pull/552/files#diff-7dcd81704618427d55b195b1e68cffc1R72-R76

rahul2393 · 2020-07-15T10:54:42Z

knqyf263 · 2020-07-16T08:55:59Z

go.mod

@@ -3,6 +3,7 @@ module github.com/aquasecurity/trivy
 go 1.13

 require (
+	github.com/Masterminds/semver v1.5.0


I found this version was old. You have to import semver as follows.

import "github.com/Masterminds/semver/v3"

knqyf263 · 2020-07-16T09:00:20Z

pkg/scanner/utils/utils.go

-	for _, p := range rangeVersions {
-		c, err := version.NewConstraint(replacer.Replace(p))
+func MatchVersions(currentVersion *semver.Version, rangeVersions []string) bool {
+	for i := range rangeVersions {


I think there is no reason to overwrite rangeVersions and it sometimes introduces a bug, so for _, v := range rangeVersions { looks enough in this case.

knqyf263 · 2020-07-16T09:02:56Z

pkg/scanner/utils/utils.go

+			part := strings.Split(constraintParts[j], ".")
+			if len(part) > 3 {
+				constraintParts[j] = strings.Join(part[:2], ".") + "." + strings.Join(part[2:], "-")
+			}


Could you define this process as another function?

knqyf263 · 2020-07-16T09:11:41Z

pkg/scanner/utils/utils.go

+		for j := range constraintParts {
+			part := strings.Split(constraintParts[j], ".")
+			if len(part) > 3 {
+				constraintParts[j] = strings.Join(part[:2], ".") + "." + strings.Join(part[2:], "-")


I think it doesn't work with a patch version including a dot such as `1.2.3-beta.1.
https://github.com/Masterminds/semver#working-with-prerelease-versions

It might work even if we replace valid dots, but I think we should keep the version in its original form as much as possible to avoid an unexpected bug.

The best way I think of at the moment is

1.2.3-beta -> 1.2.3-beta 1.2.3-beta.1 -> 1.2.3-beta.1 1.2.3.4 -> 1.2.3-4 1.2.3.4.5 -> 1.2.3-4.5 1.2.3.4-5 -> 1.2.3-4-5

Let me know your thought.

Updated, as suggested, I think you are right, let's see if we get any other issues later
@knqyf263

knqyf263 · 2020-07-19T14:23:00Z

pkg/scanner/utils/utils.go

+		for _, m := range msgs {
+			// re-validate after removing the patch version
+			if strings.HasSuffix(m.Error(), "is a prerelease version and the constraint is only looking for release versions") {
+				if v2, err := semver.NewVersion(fmt.Sprintf("%v.%v.%v", currentVersion.Major(), currentVersion.Minor(), currentVersion.Patch())); err == nil {


nit: it is wide, so what if we split the line?

Suggested change

if v2, err := semver.NewVersion(fmt.Sprintf("%v.%v.%v", currentVersion.Major(), currentVersion.Minor(), currentVersion.Patch())); err == nil {

v2, err := semver.NewVersion(fmt.Sprintf("%v.%v.%v",

currentVersion.Major(), currentVersion.Minor(), currentVersion.Patch()))

if err == nil {

knqyf263 · 2020-07-19T14:25:37Z

pkg/scanner/utils/utils.go

+			// re-validate after removing the patch version
+			if strings.HasSuffix(m.Error(), "is a prerelease version and the constraint is only looking for release versions") {
+				if v2, err := semver.NewVersion(fmt.Sprintf("%v.%v.%v", currentVersion.Major(), currentVersion.Minor(), currentVersion.Patch())); err == nil {
+					valid, msgs = c.Validate(v2)


This msgs will not be used, right? Or, does it affect the loop variable at line 37? If it is not used, we should make it easy to understand.

Suggested change

valid, msgs = c.Validate(v2)

valid, _ = c.Validate(v2)

@knqyf263 Done

* Implemented ruby comparison version check. * Added semver package to validate and check version * Added more tests * Replaced go-version with semver * Removing go-version from dependency * Added check for ruby gem version format * Updated semver model and patch rewrite process * Refactoring

Co-authored-by: Teppei Fukuda <knqyf263@gmail.com>

Implemented ruby comparison version check.

a7871bf

rahul2393 mentioned this pull request Jul 5, 2020

Version comparison broken on Rubygems with suffixed versions #108

Closed

knqyf263 reviewed Jul 6, 2020

View reviewed changes

pkg/scanner/utils/utils_test.go Show resolved Hide resolved

rahul2393 added 2 commits July 6, 2020 20:29

Added semver package to validate and check version

c35c63b

Added more tests

8e021bf

knqyf263 reviewed Jul 9, 2020

View reviewed changes

rahul2393 added 2 commits July 11, 2020 20:20

Replaced go-version with semver

16bfff7

Removing go-version from dependency

a69891b

Added check for ruby gem version format

1982e1c

knqyf263 reviewed Jul 16, 2020

View reviewed changes

Updated semver model and patch rewrite process

2aadb4b

knqyf263 reviewed Jul 19, 2020

View reviewed changes

Refactoring

08b8264

knqyf263 approved these changes Jul 19, 2020

View reviewed changes

knqyf263 merged commit 6eebed3 into aquasecurity:master Jul 19, 2020

knqyf263 mentioned this pull request Aug 24, 2020

refactor(bundler): remove unnecessary code #610

Merged

liamg pushed a commit that referenced this pull request Jun 20, 2022

feat(mariner): added support for CBL-Mariner Distroless v2.0 (#552)

a54e3bf

Co-authored-by: Teppei Fukuda <knqyf263@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve ruby comparison version check. #552

improve ruby comparison version check. #552

rahul2393 commented Jul 5, 2020

knqyf263 commented Jul 15, 2020

rahul2393 commented Jul 15, 2020 •

edited

rahul2393 commented Jul 15, 2020

knqyf263 Jul 16, 2020

knqyf263 Jul 16, 2020

knqyf263 Jul 16, 2020

knqyf263 Jul 16, 2020

rahul2393 Jul 16, 2020 •

edited

knqyf263 Jul 19, 2020

knqyf263 Jul 19, 2020

rahul2393 Jul 19, 2020

improve ruby comparison version check. #552

improve ruby comparison version check. #552

Conversation

rahul2393 commented Jul 5, 2020

knqyf263 commented Jul 15, 2020

rahul2393 commented Jul 15, 2020 • edited

rahul2393 commented Jul 15, 2020

knqyf263 Jul 16, 2020

Choose a reason for hiding this comment

knqyf263 Jul 16, 2020

Choose a reason for hiding this comment

knqyf263 Jul 16, 2020

Choose a reason for hiding this comment

knqyf263 Jul 16, 2020

Choose a reason for hiding this comment

rahul2393 Jul 16, 2020 • edited

Choose a reason for hiding this comment

knqyf263 Jul 19, 2020

Choose a reason for hiding this comment

knqyf263 Jul 19, 2020

Choose a reason for hiding this comment

rahul2393 Jul 19, 2020

Choose a reason for hiding this comment

rahul2393 commented Jul 15, 2020 •

edited

rahul2393 Jul 16, 2020 •

edited