Add option to dump minimal JSON

[dim0x69]

Hi,

currently CVSS3.as_json() adds metrics to the JSON representation of the vector, which were not part of the user-supplied vector. For example, if no "Modified Attack Vector" is supplied in the vector, the JSON will contain the MAV property with the value from AV.

This PR adds the parameter "minimal=True" to "CVSS3.as_json", which allows to dump only a minimal JSON.

[skontar]

Probably could be optimized:

    def is_environmental_used(self):
        for metric in ENVIRONMENTAL_METRICS:
            if metric in self.original_metrics:
                return True
        return False

Applies to the other function also.

[mprpic]

@skontar Hmm, looking this it's not actually clear to me as to why the optional "modified" metrics are being set to their non-modified equivalents. If my vector has AV:N, I don't think that implies that it should also have MAV:N, no? If not provided, I would assume MAV would be set to X.

As far as this PR goes, I think a nicer solution would be simply constructing the JSON from the fields that are defined in the original_metrics attribute, since those are the ones the user actually provided. That way you'd support a "minimal" representation with a varying set of fields. @dim0x69, thoughts?

[skontar]

@mprpic do I understand correctly that the original implementation is not clear to you? I can look at it next week to see why it was implemented the way it is. I do not remember anymore.

[mprpic]

@skontar Yea, basically I'm curious as to why this:

    def add_missing_optional(self):
        """
        Adds missing optional parameters, so they match the mandatory ones. Original metrics are
        also stored, as they may be used for printing back the minimal vector.
        """
        self.original_metrics = copy.copy(self.metrics)
        for abbreviation in ['MAV', 'MAC', 'MPR', 'MUI', 'MC', 'MI', 'MA']:
            if abbreviation not in self.metrics or self.metrics[abbreviation] == 'X':
                self.metrics[abbreviation] = self.metrics[abbreviation[1:]]

is not simply this:

    def add_missing_optional(self):
        self.original_metrics = copy.copy(self.metrics)
        for abbreviation in ['MAV', 'MAC', 'MPR', 'MUI', 'MC', 'MI', 'MA']:
            if abbreviation not in self.metrics:
                self.metrics[abbreviation] == 'X'

The doc string even notes so they match the mandatory ones, but I'm not sure why that is desired 🙂

[skontar]

@mprpic I think that .metrics represent what the system needs to "think" the metrics are to compute values correctly, so they are only used for computation. Basically, if you put X or do not put anything for modified metric, it needs to behave for computation like the value is the un-modified value. .original_metrics are what is stored by the user.

There are self.metrics, self.original_metrics, and self.missing_metrics. Maybe the incorrect ones are used for JSON generation from the beginning? clean_vector uses original_metrics.

[skontar]

@mprpic Seems like get_value_description should have used original_metric instead of metric. Maybe using it would actually do what minimal=True is expected to do? Just thoughts, I can look into it in more detail next week, but maybe you figure it out.

[mprpic]

@skontar Yep, I agree that the json function should be updated to use original metrics; I noted that in my comment above (#36 (comment)).

I'm still not clear though what represent what the system needs to "think" the metrics are means though. If the metric is missing, it should be assumed to be undefined (X), not the same as a different metric, no? Or is this relationship between base and modified metrics defined somewhere in the spec that I'm just not aware of?

[skontar]

I'm still not clear though what represent what the system needs to "think" the metrics are means though. If the metric is missing, it should be assumed to be undefined (X), not the same as a different metric, no? Or is this relationship between base and modified metrics defined somewhere in the spec that I'm just not aware of?

I think that the equation/math which uses modified metrics does math based on "effective" value of those modified metrics. That "effective" value is basically "if the modified metric is defined, use that, otherwise use non-modified value".

The Modified Exploitability sub score is,
8.22 × 𝑀. 𝐴𝑡𝑡𝑎𝑐𝑘𝑉𝑒𝑐𝑡𝑜𝑟 × 𝑀. 𝐴𝑡𝑡𝑎𝑐𝑘𝐶𝑜𝑚𝑝𝑙𝑒𝑥𝑖𝑡𝑦 × 𝑀. 𝑃𝑟𝑖𝑣𝑖𝑙𝑒𝑔𝑒𝑅𝑒𝑞𝑢𝑖𝑟𝑒𝑑 × 𝑀. 𝑈𝑠𝑒𝑟𝐼𝑛𝑡𝑒𝑟𝑎𝑐𝑡𝑖𝑜n

For the math purpose they cannot really be not defined. Not defined effectively means the same as non-modified variant.

[skontar]

Each Modified Environmental metric has the same values as its corresponding Base metric, plus a value of Not Defined. Not Defined is the default and uses the metric value of the associated Base metric.

https://www.first.org/cvss/v3.1/specification-document

[mprpic]

Each Modified Environmental metric has the same values as its corresponding Base metric, plus a value of Not Defined. Not Defined is the default and uses the metric value of the associated Base metric.

https://www.first.org/cvss/v3.1/specification-document

A ha! That is the verbiage I was looking for. Perhaps we could add a reference to this in the doc string to make it clearer for future users.

[skontar]

@dim0x69 if user did not supply specific modified metric, would you prefer to not see it in the JSON at all or is "Not Defined (X)" fine for your purpose? I am thinking that using metric instead of original_metric in JSON generation is actually a bug we need to resolve and having full JSON with some values showing as "Not Defined" would be OK?

[mprpic]

Just fyi, I submitted a separate PR (#39) that cleans up the commits here based on the feedback submitted here, and adds the same logic to CVSS2. I think we can close this one if the other one is merged.

[mprpic]

#39 was merged. Closing this one! Thank you for your contributions, @dim0x69 (your commits were still included in the other PR)!