Merge ~pfsmorigo/ubuntu-cve-tracker:pfsmorigo/priority-reason into ubuntu-cve-tracker:master

[Paulo Flabiano Smorigo]

[Alex Murray (alexmurray)]

A few general questions:

1. Should the name just be Reason instead of Priority-Reason to make it easier?
2. What if instead we extended the syntax to allow the reason to be listed directly underneath the priority, e.g.

Priority: low
  classified as low since it is not a real vulnerability

as I think this is easier to read personally and it keeps the reason with the Priority

3. Or instead it could be a comment after the priority (but this would mean we couldn't easily do long Reason's) e.g.

Priority: low # classified as low since it is not a real vulnerability

4. Like Notes: do we want to capture who added the reason?
5. Do we also need to update all the boilerplates?
6. I think the check for this should be removed from cve_lib.py and instead added to check-syntax as this feels more like a policy check than a low-level format check
7. Can we please have a unit test in test_cve_lib.py?

[Paulo Flabiano Smorigo (pfsmorigo)]

Thanks for the feedback. About the questions...

1. Leaving just as Reason feels vague but I'm ok to use it instead. This change also works for the package priority so, for instance, Priority-Reason_koffice would be just Reason_koffice.

2 and 3. I'm ok with both alternatives as long as we have a good way to parse it properly. Maybe parentesis like we do for package states: "Priority: low (it is not a real vulnerability)"

4. Not sure. We can always check git for the author. Not sure if it matters for the person browsing the CVE.

5. If we go with 2 and 3 suggestions, only the boilerplates that has a low/high/critical would be modified. Nowadays only one boilerplate has low (thunderbird). Now, if we make it mandatory, we would need to change all CVE that fits the criteria.

6. check-syntax uses a function in cve_lib.py to verify the CVEs. I just added an additional check there for the priority reason.

[Paulo Flabiano Smorigo (pfsmorigo)]

AlexM: I changed the format to be a comment in parenthesis after the tag. I still using priority-reason for the name but I'm ok about using other term. Also, maybe we can start without enforcing it for now. Otherwise we will need to add a reason for all the CVEs we have that fits the criteria.

[Camila Camargo de Matos (ccdm94)]

Since the priority reason will need to be short because it will be included in the parenthesis, maybe it is interesting to come with some "default" possible texts to include, so that we can use them as reference. For example, when we update the status of a release to 'not-affected', for example, and the reason behind it not being affected is because the function being patched doesn't exist, or because the code doesn't include some other function which is needed to trigger the vulnerability, we usually put something like 'code not present' for all cases. I don't know if this PR is the right one to possibly include these changes, but it could be useful for some cases that are commonly found during triage. An example would be a 'code not compiled in Ubuntu' for a priority set as 'low'.

[Alex Murray (alexmurray)]

LGTM!

review: Approve

[Paulo Flabiano Smorigo (pfsmorigo)]

Hello Alex and other reviewers,

A couple of months ago we had a discussion and I did all the changes at the time. I was waiting for the review but I just realised that I didn't push the new changes to this PR so I did it now. I took the opportunity to change a little bit and merged all commits into one.

This commit will:
 - Change the priority field to a tuple in the format ('priority', 'priority reason');
 - The format for the reason will be a line after the Priority field with a space at the begin:

Priority: high
 classified as high because...

 - It will make check_syntax complains if priority is other than 'medium' and there is no reason for it;
 - The check only complains if the PublishDate is after this month (avoid check in old CVEs);

[Alex Murray (alexmurray)]

LGTM still - thanks @pfsmorigo

review: Approve

diff --git a/scripts/active_edit b/scripts/active_edit
index 4e2be92..d9068d6 100755
--- a/scripts/active_edit
+++ b/scripts/active_edit
@@ -232,7 +232,10 @@ def create_or_update_cve(cve, packages, priority=None, bug_urls=None, ref_urls=N
             print('Bugs:', file=fp)
             for url in (bug_urls if bug_urls else []):
                 print(" %s" % url, file=fp)
-            print('Priority: %s' % (priority if priority else "untriaged"), file=fp)
+            priority_text = priority if priority else "untriaged"
+            if priority in cve_lib.PRIORITY_REASON_REQUIRED:
+                priority_text += "\n XXX-Reason-XXX"
+            print('Priority: %s' % priority_text, file=fp)
             print('Discovered-by:', file=fp)
             print('Assigned-to:', file=fp)
             print('CVSS:', file=fp)
diff --git a/scripts/check-syntax b/scripts/check-syntax
index f3a5128..2bf4202 100755
--- a/scripts/check-syntax
+++ b/scripts/check-syntax
@@ -764,7 +764,7 @@ def check_cve(cve):
     if (
         len(supported)
         and (is_active(cve) or is_embargoed(cve))
-        and ("Priority" not in data or data["Priority"] not in cve_lib.priorities)
+        and ("Priority" not in data or data["Priority"][0] not in cve_lib.priorities)
     ):
         filename = srcmap["Priority"][0] if "Priority" in srcmap else cvepath
         linenum = srcmap["Priority"][1] if "Priority" in srcmap else 1
diff --git a/scripts/cve_lib.py b/scripts/cve_lib.py
index 9b2e44e..d90dbd7 100755
--- a/scripts/cve_lib.py
+++ b/scripts/cve_lib.py
@@ -64,6 +64,8 @@ else:
     boilerplates_dir = "boilerplates"
 
 PRODUCT_UBUNTU = "ubuntu"
+PRIORITY_REASON_REQUIRED = ["low", "high", "critical"]
+PRIORITY_REASON_DATE_START = "2023-07-11"
 
 # common to all scripts
 # these get populated by the contents of subprojects defined below
@@ -1856,8 +1858,8 @@ def contextual_priority(cveinfo, pkg=None, rel=None):
             if rel_p in cveinfo:
                 return 2, cveinfo[rel_p]
         if pkg_p in cveinfo:
-            return 1, cveinfo[pkg_p]
-    return 0, cveinfo.get('Priority', 'untriaged')
+            return 1, cveinfo[pkg_p][0]
+    return 0, cveinfo['Priority'][0] if 'Priority' in cveinfo else 'untriaged'
 
 
 def find_cve(cve):
@@ -2095,6 +2097,7 @@ def load_cve(cve, strict=False, srcmap=None):
         raise ValueError("File does not exist: '%s'" % (cve))
     linenum = 0
     notes_parser = NotesParser()
+    priority_reason = {}
     cvss_entries = []
     with codecs.open(cve, encoding="utf-8") as inF:
         lines = inF.readlines()
@@ -2112,6 +2115,12 @@ def load_cve(cve, strict=False, srcmap=None):
                     code, newmsg = notes_parser.parse_line(cve, line, linenum, code)
                     if code != EXIT_OKAY:
                         msg += newmsg
+                elif lastfield.startswith('Priority'):
+                    priority_part = lastfield.split('_')[1] if '_' in lastfield else None
+                    if priority_part in priority_reason:
+                        priority_reason[priority_part].append(line.strip())
+                    else:
+                        priority_reason[priority_part] = [line.strip()]
                 elif 'Patches_' in lastfield:
                     try:
                         _, pkg = lastfield.split('_', 1)
@@ -2186,7 +2195,7 @@ def load_cve(cve, strict=False, srcmap=None):
                     msg += "%s: %d: bad field with 'Priority_': '%s'\n" % (cve, linenum, field)
                     code = EXIT_FAIL
                     continue
-            data.setdefault(field, value)
+            data.setdefault(field, [value])
             srcmap.setdefault(field, (cve, linenum))
             if value not in ['untriaged', 'not-for-us'] + priorities:
                 msg += "%s: %d: unknown Priority '%s'\n" % (cve, linenum, value)
@@ -2265,7 +2274,7 @@ def load_cve(cve, strict=False, srcmap=None):
 
     # Fill in defaults for missing fields
     if 'Priority' not in data:
-        data.setdefault('Priority', 'untriaged')
+        data.setdefault('Priority', ['untriaged'])
         srcmap.setdefault('Priority', (cve, 1))
     # Perform override fields
     if 'PublicDateAtUSN' in data:
@@ -2277,6 +2286,14 @@ def load_cve(cve, strict=False, srcmap=None):
         data['PublicDate'] = data['CRD']
         srcmap['PublicDate'] = srcmap['CRD']
 
+    if data["PublicDate"] > PRIORITY_REASON_DATE_START and \
+            data["Priority"][0] in PRIORITY_REASON_REQUIRED and not priority_reason:
+        msg += "%s: %d: needs a reason for being '%s'\n" % (cve, linenum, data["Priority"][0])
+        code = EXIT_FAIL
+    for item in priority_reason:
+        field = 'Priority' if not item else 'Priority_' + item
+        data[field].append(' '.join(priority_reason[item]))
+
     # entries need an upstream entry if any entries are from the internal
     # list of subprojects
     for pkg in affected:
@@ -2339,7 +2356,7 @@ def load_table(cves, uems, opt=None, rcves=[], icves=[]):
         # Allow for Priority overrides
         priority[cve]['default'] = 'untriaged'
         try:
-            priority[cve]['default'] = info['Priority']
+            priority[cve]['default'] = info['Priority'][0]
         except KeyError:
             priority[cve]['default'] = 'untriaged'
 
diff --git a/scripts/cve_packages b/scripts/cve_packages
index da038c4..c4903af 100755
--- a/scripts/cve_packages
+++ b/scripts/cve_packages
@@ -253,9 +253,9 @@ for cve in sorted(cves):
             if cveinfo[cve]['CVSS']:
                 p = cveinfo[cve]['CVSS'][0]['baseSeverity'].lower()
             else:
-                p = priority[cve]['default']
+                p = priority[cve]['default'][0]
         else:
-            p = priority[cve]['default']
+            p = priority[cve]['default'][0]
 
         if pkg in packages:
             packages[pkg] += 1
@@ -263,7 +263,7 @@ for cve in sorted(cves):
             packages[pkg] = 1
 
         if pkg in priority[cve]:
-            p = priority[cve][pkg]
+            p = priority[cve][pkg][0]
 
         if pkg in priorities[p]:
             priorities[p][pkg] += 1
diff --git a/scripts/oval_lib.py b/scripts/oval_lib.py
index ed64318..f8decdd 100644
--- a/scripts/oval_lib.py
+++ b/scripts/oval_lib.py
@@ -1266,7 +1266,7 @@ class OvalGeneratorCVE:
             'cve_title': escape(header['Candidate']),
             'description': escape('{0} {1}'.format(header['Description'],
                                   header['Ubuntu-Description']).strip() + instruction),
-            'priority': escape(header['Priority']),
+            'priority': escape(header['Priority'][0]),
             'criteria': '',
             'references': '',
             'notes': ''
@@ -1316,7 +1316,7 @@ class OvalGeneratorCVE:
         # convert additional data <advisory> metadata elements
         advisory = []
         advisory.append('<severity>{0}</severity>'.format(
-            escape(header['Priority'].title())))
+            escape(header['Priority'][0].title())))
         advisory.append(
             '<rights>Copyright (C) {0}Canonical Ltd.</rights>'.format(escape(
                 header['PublicDate'].split('-', 1)[0] + ' '
@@ -2471,7 +2471,7 @@ class OvalGeneratorUSN():
             return None
 
         public_date = cve_object['PublicDate']
-        priority = cve_object['Priority']
+        priority = cve_object['Priority'][0]
         references = cve_object['References']
         # TODO: deal with multiple CVSS?
         cve_info = {
diff --git a/scripts/publish-cves-to-website-api.py b/scripts/publish-cves-to-website-api.py
index aa80880..d6af0e7 100755
--- a/scripts/publish-cves-to-website-api.py
+++ b/scripts/publish-cves-to-website-api.py
@@ -166,7 +166,7 @@ def post_single_cve(cve_filename):
     for [author, note] in cve_data["Notes"]:
         notes.append({"author": author, "note": note})
 
-    priority = cve_data["Priority"]
+    priority = cve_data["Priority"][0]
 
     if priority == "untriaged":
         priority = "unknown"
diff --git a/scripts/report-date.py b/scripts/report-date.py
index c983072..e350e0c 100755
--- a/scripts/report-date.py
+++ b/scripts/report-date.py
@@ -78,9 +78,9 @@ for priority in ['untriaged'] + cve_lib.priorities:
                     continue
 
                 # Only report if we have a matching priority
-                this_priority = info[cve]['Priority']
+                this_priority = info[cve]['Priority'][0]
                 if info[cve].has_key('Priority_%s' % (pkg)):
-                    this_priority = info[cve]['Priority_%s' % (pkg)]
+                    this_priority = info[cve]['Priority_%s' % (pkg)][0]
                 if this_priority == priority:
                     pkglist.add(pkg)
 
diff --git a/scripts/sis-generate-usn b/scripts/sis-generate-usn
index 4a46e18..4535210 100755
--- a/scripts/sis-generate-usn
+++ b/scripts/sis-generate-usn
@@ -198,8 +198,8 @@ def check_cve_priority(cve):
         print("ERROR: {} does not exist in UCT in either {}".format(cve, state),
               file=sys.stderr)
         sys.exit(1)
-    if cve_data['Priority'] in ['untriaged', 'not-for-us']:
-        print("ERROR: {} has Priority {}".format(cve, cve_data['Priority']),
+    if cve_data['Priority'][0] in ['untriaged', 'not-for-us']:
+        print("ERROR: {} has Priority {}".format(cve, cve_data['Priority'][0]),
               file=sys.stderr)
         sys.exit(1)
 
diff --git a/test/okay/cve-id-N7.json b/test/okay/cve-id-N7.json
index 6685f37..afbffa1 100644
--- a/test/okay/cve-id-N7.json
+++ b/test/okay/cve-id-N7.json
@@ -13,7 +13,7 @@
   "Notes": [],
   "Mitigation": "",
   "Bugs": "",
-  "Priority": "negligible",
+  "Priority": ["negligible"],
   "Discovered-by": "",
   "Assigned-to": "",
   "CVSS": [],
diff --git a/test/okay/cve-id-NNNN.json b/test/okay/cve-id-NNNN.json
index 1d19260..0c008ca 100644
--- a/test/okay/cve-id-NNNN.json
+++ b/test/okay/cve-id-NNNN.json
@@ -13,7 +13,7 @@
   "Notes": [],
   "Mitigation": "",
   "Bugs": "",
-  "Priority": "negligible",
+  "Priority": ["negligible"],
   "Discovered-by": "",
   "Assigned-to": "",
   "CVSS": [],
diff --git a/test/okay/patches-missing-1.json b/test/okay/patches-missing-1.json
index f438ff6..c78d347 100644
--- a/test/okay/patches-missing-1.json
+++ b/test/okay/patches-missing-1.json
@@ -11,7 +11,7 @@
   "Notes": [],
   "Mitigation": "",
   "Bugs": "",
-  "Priority": "medium",
+  "Priority": ["medium"],
   "Discovered-by": "",
   "Assigned-to": "",
   "CVSS": [],
diff --git a/test/okay/patches-missing-2.json b/test/okay/patches-missing-2.json
index d057b6e..92fac9d 100644
--- a/test/okay/patches-missing-2.json
+++ b/test/okay/patches-missing-2.json
@@ -11,7 +11,7 @@
   "Notes": [],
   "Mitigation": "",
   "Bugs": "",
-  "Priority": "medium",
+  "Priority": ["medium"],
   "Discovered-by": "",
   "Assigned-to": "",
   "CVSS": [],
diff --git a/test/okay/patches-missing-3.json b/test/okay/patches-missing-3.json
index 950893d..b58408e 100644
--- a/test/okay/patches-missing-3.json
+++ b/test/okay/patches-missing-3.json
@@ -13,7 +13,7 @@
   "Notes": [],
   "Mitigation": "",
   "Bugs": "",
-  "Priority": "medium",
+  "Priority": ["medium"],
   "Discovered-by": "",
   "Assigned-to": "",
   "CVSS": [],
diff --git a/test/okay/patches-missing-4.json b/test/okay/patches-missing-4.json
index 6c03170..5ab7913 100644
--- a/test/okay/patches-missing-4.json
+++ b/test/okay/patches-missing-4.json
@@ -13,7 +13,7 @@
   "Notes": [],
   "Mitigation": "",
   "Bugs": "",
-  "Priority": "medium",
+  "Priority": ["medium"],
   "Discovered-by": "",
   "Assigned-to": "",
   "CVSS": [],
diff --git a/test/okay/priority-critical b/test/okay/priority-critical
index 5d20f02..78a81de 100644
--- a/test/okay/priority-critical
+++ b/test/okay/priority-critical
@@ -11,6 +11,7 @@ Notes:
 Mitigation: 
 Bugs: 
 Priority: critical
+ reason for being critial
 Discovered-by:
 Assigned-to:
 CVSS:
diff --git a/test/okay/priority-critical.json b/test/okay/priority-critical.json
index 5b99773..89cbce9 100644
--- a/test/okay/priority-critical.json
+++ b/test/okay/priority-critical.json
@@ -13,7 +13,7 @@
   "Notes": [],
   "Mitigation": "",
   "Bugs": "",
-  "Priority": "critical",
+  "Priority": ["critical", "reason for being critial"],
   "Discovered-by": "",
   "Assigned-to": "",
   "CVSS": [],
diff --git a/test/okay/priority-high b/test/okay/priority-high
index f97416a..5ec543f 100644
--- a/test/okay/priority-high
+++ b/test/okay/priority-high
@@ -11,6 +11,7 @@ Notes:
 Mitigation: 
 Bugs: 
 Priority: high
+ reason for being high
 Discovered-by:
 Assigned-to:
 CVSS:
diff --git a/test/okay/priority-high.json b/test/okay/priority-high.json
index 1c91a1f..f893e73 100644
--- a/test/okay/priority-high.json
+++ b/test/okay/priority-high.json
@@ -13,7 +13,7 @@
   "Notes": [],
   "Mitigation": "",
   "Bugs": "",
-  "Priority": "high",
+  "Priority": ["high", "reason for being high"],
   "Discovered-by": "",
   "Assigned-to": "",
   "CVSS": [],
diff --git a/test/okay/priority-low b/test/okay/priority-low
index 62d21c5..8c5050c 100644
--- a/test/okay/priority-low
+++ b/test/okay/priority-low
@@ -11,6 +11,7 @@ Notes:
 Mitigation:
 Bugs:
 Priority: low
+ reason for being low
 Discovered-by:
 Assigned-to:
 CVSS:
diff --git a/test/okay/priority-low.json b/test/okay/priority-low.json
index 76eb0f6..8eadf86 100644
--- a/test/okay/priority-low.json
+++ b/test/okay/priority-low.json
@@ -13,7 +13,7 @@
   "Notes": [],
   "Mitigation": "",
   "Bugs": "",
-  "Priority": "low",
+  "Priority": ["low", "reason for being low"],
   "Discovered-by": "",
   "Assigned-to": "",
   "CVSS": [],
diff --git a/test/okay/priority-medium.json b/test/okay/priority-medium.json
index acd1505..03aa9f2 100644
--- a/test/okay/priority-medium.json
+++ b/test/okay/priority-medium.json
@@ -13,7 +13,7 @@
   "Notes": [],
   "Mitigation": "",
   "Bugs": "",
-  "Priority": "medium",
+  "Priority": ["medium"],
   "Discovered-by": "",
   "Assigned-to": "",
   "CVSS": [],
diff --git a/test/okay/priority-negligible.json b/test/okay/priority-negligible.json
index 16783ce..5035758 100644
--- a/test/okay/priority-negligible.json
+++ b/test/okay/priority-negligible.json
@@ -13,7 +13,7 @@
   "Notes": [],
   "Mitigation": "",
   "Bugs": "",
-  "Priority": "negligible",
+  "Priority": ["negligible"],
   "Discovered-by": "",
   "Assigned-to": "",
   "CVSS": [],
diff --git a/test/okay/priority-untriaged.json b/test/okay/priority-untriaged.json
index ab2ece0..a596046 100644
--- a/test/okay/priority-untriaged.json
+++ b/test/okay/priority-untriaged.json
@@ -13,7 +13,7 @@
   "Notes": [],
   "Mitigation": "",
   "Bugs": "",
-  "Priority": "untriaged",
+  "Priority": ["untriaged"],
   "Discovered-by": "",
   "Assigned-to": "",
   "CVSS": [],