Another Improper Input Validation in CVSS v2 parsing

[pandatix]

During differential fuzzing with github.com/pandatix/go-cvss I discovered that your implementation does not properly handle the case of a CVSS v2 environmental parsing for vectors that does not have environmental metrics defined.
This could be categorized as CWE-20.

In order to be compliant with the first.org specification you must validate vectors that does not have environmental metrics defined.

The following Go code illustrates this issue.
Notice the input vector comes from the specification section 3.3.1 for the CVE-2002-0392.

package main

import (
	"fmt"

	"github.com/goark/go-cvss/v2/metric"
)

func main() {
	raw := "AV:N/AC:L/Au:N/C:N/I:N/A:C/E:F/RL:OF/RC:C"
	vec, err := metric.NewEnvironmental().Decode(raw)

	fmt.Printf("vec: %v\n", vec)
	fmt.Printf("err: %v\n", err)
}

produces ->

vec: 
err: no metrics

[spiegel-im-spiegel]

metric.Environmental type of CVSSv2 requires Base, Temporal, and Environmental metrics (issue #26).
If Environmental metrics are not defined, use metric.Temporal type.

package main

import (
	"fmt"

	"github.com/goark/go-cvss/v2/metric"
)

func main() {
	raw := "AV:N/AC:L/Au:N/C:N/I:N/A:C/E:F/RL:OF/RC:C"
	vec, err := metric.NewTemporal().Decode(raw)

	fmt.Printf("vec: %v\n", vec)
	fmt.Printf("err: %v\n", err)
	fmt.Printf("Severity: %v (%v)\n", vec.Severity(), vec.Score())
}

(see https://go.dev/play/p/FI9sWSuGw85)

Or supplement the environmental metrics explicitly.

package main

import (
	"fmt"

	"github.com/goark/go-cvss/v2/metric"
)

func main() {
	raw := "AV:N/AC:L/Au:N/C:N/I:N/A:C/E:F/RL:OF/RC:C/CDP:ND/TD:ND/CR:ND/IR:ND/AR:ND"
	vec, err := metric.NewEnvironmental().Decode(raw)

	fmt.Printf("vec: %v\n", vec)
	fmt.Printf("err: %v\n", err)
	fmt.Printf("Severity: %v (%v)\n", vec.Severity(), vec.Score())
}

(see https://go.dev/play/p/x9m33VaKuFp)

[spiegel-im-spiegel]

another code:

package main

import (
	"fmt"

	"github.com/goark/go-cvss/v2/metric"
)

func main() {
	raw := "AV:N/AC:L/Au:N/C:N/I:N/A:C/E:F/RL:OF/RC:C"
	vec, err := metric.NewEnvironmental().Decode(raw)
	if err == nil {
		fmt.Printf("vector (Environmental): %v\n", vec)
		fmt.Printf("Severity (Environmental): %v (%v)\n", vec.Severity(), vec.Score())
	} else if vec.Temporal.GetError() == nil {
		fmt.Printf("vector (Temporal): %v\n", vec.Temporal)
		fmt.Printf("Severity (Temporal): %v (%v)\n", vec.Temporal.Severity(), vec.Temporal.Score())
	} else if vec.Base.GetError() == nil {
		fmt.Printf("vec (Base): %v\n", vec.Base)
		fmt.Printf("Severity (Base): %v (%v)\n", vec.Base.Severity(), vec.Base.Score())
	} else {
		fmt.Printf("err: %v\n", vec.Base.GetError())
	}
}

(see https://go.dev/play/p/4n8PkrUBsP7)

[pandatix]

Ok, thanks for the code snippets and answer.
#26 needed all the metrics of the group (base OR temporal OR environmental) to be defined in the vector in order to be valid (for instance, you can't only have one metric from the temporal group, but needed them all even if other are set to ND).

Nevertheless, let's suppose I have an untrusted input data source (worst case). How could I validate the vectors ? The fact is that I don't know if there will be only Base group defined, Base/Temporal, Base/Environmental or Base/Temporal/Environmental combinations.
That's why I'm using the environmental metrics by default (largest case). If there is no temporal and environmental, I expect the implementation not to parse/consider them as they are not specified in the input and return no error. Moreover, what I don't want is having to deal with :

1. parse environmental and check if error is "no metric"
2. if it is, parse temporal and check if error is "no metric"
3. if it is, parse base
   Currently, I have to deal with 3 error sources possibly leading implementers to errors only to parse a string (what is the semantic and order between each ? which error states that the vector is invalid or not ?)

[spiegel-im-spiegel]

Release v1.6.1:

package main

import (
    "errors"
    "fmt"

    "github.com/goark/go-cvss/cvsserr"
    "github.com/goark/go-cvss/v2/metric"
)

func main() {
    raw := "AV:N/AC:L/Au:N/C:N/I:N/A:C/E:F/RL:OF/RC:C"
    vec, err := metric.NewEnvironmental().Decode(raw)
    fmt.Printf("err: %v\n", err)
    fmt.Printf("vector: %v\n", vec)
    switch true {
    case errors.Is(err, cvsserr.ErrNoEnvironmentalMetrics):
        fmt.Printf("Severity (Temporal): %v (%v)\n", vec.Temporal.Severity(), vec.Temporal.Score())
    case errors.Is(err, cvsserr.ErrNoTemporalMetrics):
        fmt.Printf("Severity (Base): %v (%v)\n", vec.Base.Severity(), vec.Base.Score())
    default:
        fmt.Printf("Severity (Environmental): %v (%v)\n", vec.Severity(), vec.Score())
    }
}

(see https://go.dev/play/p/QGT6akF3h0s)

[pandatix]

This behavior differs between your implementation of CVSS v2 and v3. For instance, the following Go code shows in the same conditions (no environmental metrics defined in the vector despite using an environmental object to decode) you don't raise an issue.

package main

import (
	"fmt"

	"github.com/goark/go-cvss/v3/metric"
)

func main() {
	raw := "CVSS:3.1/AV:N/AC:L/PR:L/UI:R/S:C/C:L/I:L/A:N/E:F/RL:O/RC:C"
	vec, err := metric.NewEnvironmental().Decode(raw)

	fmt.Printf("vec: %v\n", vec)
	fmt.Printf("err: %v\n", err)
}

produces ->

vec: CVSS:3.1/AV:N/AC:L/PR:L/UI:R/S:C/C:L/I:L/A:N/E:F/RL:O/RC:C/CR:X/IR:X/AR:X/MAV:X/MAC:X/MPR:X/MUI:X/MS:X/MC:X/MI:X/MA:X
err: <nil>

[spiegel-im-spiegel]

Elements for Temporal and Environmental metrics are optional in CVSSv3 vector string, so if an element is omitted it will be completed with an X (Not Defined).
A CVSSv2 vector string requires all elements, so omitting an element for each metrics causes an error.

This is the intended behavior.

[pandatix]

As specified in the first.org specification :

• Section 2.2 :

Since temporal metrics are optional [...]

• Section 2.3 :

Since environmental metrics are optional [...]

CVSS v2 vectors does not require all metrics, they require all of a group as soon as one metric of this group is specified (see Table 13). Subsequently, this is not the intended behavior.