Merge ~eslerm/ubuntu-cve-tracker:check-cves-variables into ubuntu-cve-tracker:master

[Mark Esler]

check-cves: variable clarity updates

This is mostly an update to make variables clearer.

I should have said "constants" not "globals".

I'm not certain all of them should be added. Particularly `BUILT_USING_MAP` which pylint suggested. `UNTRIAGED_JSON` as well, but I don't believe code using this variable is proper (several parameters can override it).

There is more work to clarify human_process_cve(), but this seems like a good point to stop and ask for a review.

[Mark Esler (eslerm)]

I used pylint to inform some of these changes: https://pylint.readthedocs.io/en/stable/user_guide/messages/convention/invalid-name.html

[Marc Deslauriers (mdeslaur)]

This script doesn't have a main function, it's just a script. If you really want to run something like pylint over it and want to switch everything it thinks are "global" variables to uppercase, you should probably convert the script to use a main function first.

[Mark Esler (eslerm)]

The plan is to use a main function. My refactor isn't complete, but had begun to implement this https://git.launchpad.net/~eslerm/ubuntu-cve-tracker/tree/scripts/check_cves.py?h=cve-translate

It's easier to work with the code after it is clearer, which is why I'm suggesting style changes first.

Potentially I _could_ drop all of these variables to lower case if needed.

[Alex Murray (alexmurray)]

LGTM

review: Approve

diff --git a/scripts/check-cves b/scripts/check-cves
index be0b713..c1239fd 100755
--- a/scripts/check-cves
+++ b/scripts/check-cves
@@ -79,20 +79,20 @@ for release in list(source.keys()):
 # remove common words which also happen to be names
 # of source packages since our ignore suggestion is
 # likely to sometimes contain these
-common_words = ['an', 'and', 'context', 'file', 'modules', 'the', 'when']
-allsrcs.difference_update(set(common_words))
+COMMON_WORDS = ['an', 'and', 'context', 'file', 'modules', 'the', 'when']
+allsrcs.difference_update(set(COMMON_WORDS))
 allsrcs.update(set(cve_lib.package_db.keys()))
 
-built_using_map = None
+BUILT_USING_MAP = None
 
-destdir = "."
+DEST_DIR = "."
 
 # Skip stuff older than 2005
-cve_limit = 2004
+CVE_LIMIT = 2004
 
-mistriaged_hint = 'Previously triaged as ignored in Ubuntu\n\n'
+MISTRIAGED_HINT = 'Previously triaged as ignored in Ubuntu\n\n'
 
-ignore_strings = [
+IGNORE_STRINGS = [
     "** REJECT **", "Internet Explorer", "Microsoft Edge", "Windows 98",
     "Windows 2000", "Windows XP", "Windows Server 2003", "Windows NT",
     "Mercury Board", "ZeroBoard", "AntiVirus", "Microsoft", "SGI IRIX",
@@ -136,7 +136,6 @@ def _spawn_editor(path):
     subprocess.call([editor, path])
 
 def debug(msg):
-    global opt
     if args.debug:
         print(msg, file=sys.stderr)
 
@@ -227,7 +226,7 @@ def import_debian(handler):
     cves = dict()
 
     today = datetime_date.today()
-    known = set(CVEKnownList + CVEIgnoreList)
+    known = set(cve_known_list + cve_ignore_list)
 
     def ever_existed(pkg):
         for rel in source:
@@ -236,8 +235,8 @@ def import_debian(handler):
         return False
 
     def mistriaged(cve):
-        if cve in CVEIgnoreNotForUsSet and \
-           cve not in CVEIgnoreMistriagedSet and \
+        if cve in CVE_IGNORE_NFU_SET and \
+           cve not in CVE_IGNORE_MISTRIAGED_LIST and \
            handler.debian[cve]['state'] == 'FOUND':
             # check that at least one of the assigned packages exist
             # in Ubuntu
@@ -259,7 +258,7 @@ def import_debian(handler):
                 continue
 
             year = int(re.split('-', cve)[1])
-            if year < cve_limit:
+            if year < CVE_LIMIT:
                 continue
 
             # If we already know about the CVE, skip it unless is
@@ -267,7 +266,7 @@ def import_debian(handler):
             if cve in known:
                 if mistriaged(cve):
                     # add a note about how this was originally classified
-                    dsas[dsa]['desc'] = mistriaged_hint + dsas[dsa]['desc']
+                    dsas[dsa]['desc'] = MISTRIAGED_HINT + dsas[dsa]['desc']
                 else:
                     continue
 
@@ -294,16 +293,16 @@ def import_debian(handler):
             continue
 
         year = int(re.split('-', cve)[1])
-        if year < cve_limit:
+        if year < CVE_LIMIT:
             if args.verbose:
-                print(f"Skipping {cve}, year {year} predates {cve_limit}", file=sys.stderr)
+                print(f"Skipping {cve}, year {year} predates {CVE_LIMIT}", file=sys.stderr)
             continue
 
         # If we already know about the CVE, skip it unless is mistriaged
         if cve in known:
             if mistriaged(cve):
                 # add a note about how this was originally classified
-                handler.debian[cve]['desc'] = mistriaged_hint + handler.debian[cve]['desc']
+                handler.debian[cve]['desc'] = MISTRIAGED_HINT + handler.debian[cve]['desc']
             else:
                 if args.verbose:
                     print(f"Skipping {cve}, already known", file=sys.stderr)
@@ -539,14 +538,14 @@ def dpkg_compare_versions(v1, op, v2):
 
 
 def get_built_using(pkgs=[]):
-    global built_using_map
-    if built_using_map is None:
-        built_using_map = source_map.load_built_using_collection(
+    global BUILT_USING_MAP
+    if BUILT_USING_MAP is None:
+        BUILT_USING_MAP = source_map.load_built_using_collection(
                               source_map.load(data_type='packages'))
 
     out = ""
     for pkg in pkgs:
-        out += source_map.get_built_using(built_using_map, pkg)
+        out += source_map.get_built_using(BUILT_USING_MAP, pkg)
 
     return out
 
@@ -602,7 +601,7 @@ class CVEHandler(xml.sax.handler.ContentHandler):
         timestamp = time.strftime("%Y-%m-%d %H:%M:%S", time.gmtime())
 
         # Append to timestamp file list
-        with open(f'{destdir}/check-cves.log', 'a') as f:
+        with open(f'{DEST_DIR}/check-cves.log', 'a') as f:
             f.write(
                 f"{timestamp} UTC - "
                 f"{self.num_added} added, "
@@ -780,10 +779,10 @@ class CVEHandler(xml.sax.handler.ContentHandler):
         if not self.curr_cve or not self.curr_desc:
             return
         # Skip CVEs we know about already unless this is a mistriaged CVE
-        if self.curr_cve in self.cve_ignore and mistriaged_hint not in self.curr_desc:
+        if self.curr_cve in self.cve_ignore and MISTRIAGED_HINT not in self.curr_desc:
             return
 
-        limit = cve_limit
+        limit = CVE_LIMIT
         if not args.refresh and not args.score_refresh:
             limit = 2005
         if int(self.curr_cve.split("-")[1]) < limit:
@@ -832,7 +831,7 @@ class CVEHandler(xml.sax.handler.ContentHandler):
             if subproject in source:
                 aliases = source_map.get_all_aliases(source, subproject)
                 for hint in software_hints_from_cve_description:
-                    if hint in common_words:
+                    if hint in COMMON_WORDS:
                         continue
 
                     if hint in source[subproject]:
@@ -863,7 +862,7 @@ class CVEHandler(xml.sax.handler.ContentHandler):
             sys.stdout = CVEOutput(file, line_prefix)
 
         # Check if this was once an embargoed issue
-        if cve in EmbargoList:
+        if cve in CVE_EMBARGO_LIST:
             print('**!!** no longer embargoed **!!**')
             print('==========================details from embargo entry==========================')
             with open(os.path.join('embargoed', cve)) as f:
@@ -927,7 +926,7 @@ class CVEHandler(xml.sax.handler.ContentHandler):
                     print("    - " + affected_subproject + ": " + " - ".join(
                         software_hints_per_external_releases[affected_subproject]))
         # once again, announce formerly embargoed status
-        if cve in EmbargoList:
+        if cve in CVE_EMBARGO_LIST:
             print('**!!**            no longer embargoed             **!!**')
             print('**!!** ensure this is correct before unembargoing **!!**')
 
@@ -940,12 +939,12 @@ class CVEHandler(xml.sax.handler.ContentHandler):
         reason = ""
         packages = []
         # Skip CVEs that are obviously not about Ubuntu
-        for s in ignore_strings:
+        for s in IGNORE_STRINGS:
             if re.search('(^| )%s' % re.escape(s), self.cve_data[cve]['desc'], flags=re.MULTILINE) and self.cve_data[cve]['desc'].find("Linux") < 0:
                 action = 'ignore'
                 reason = s
         # if cve is in embargo list (but now public), default to unembargo action
-        if cve in EmbargoList:
+        if cve in CVE_EMBARGO_LIST:
             action = 'unembargo'
             reason = ""
         else:
@@ -1011,36 +1010,36 @@ class CVEHandler(xml.sax.handler.ContentHandler):
         return words
 
     def human_process_cve(self, cve, action='skip', reason='', package=''):
-        info = ''
-        while info == "" or info[0] not in ['i', 'a', 's', 'q', 'r']:
+        user_input = ''
+        while user_input == "" or user_input[0] not in ['i', 'a', 's', 'q', 'r']:
             prompt_user(f'\nA]dd (or R]epeat), I]gnore forever, S]kip for now, or Q]uit? [{action}] ')
-            info = sys.stdin.readline().strip().lower()
-            if info == "":
-                info = action
+            user_input = sys.stdin.readline().strip().lower()
+            if user_input == "":
+                user_input = action
 
-        if info.startswith('q'):
+        if user_input.startswith('q'):
             self.printReport()
             self.updateTimestamp()
             sys.exit(0)
-        elif info.startswith('a') or info.startswith('r'):
+        elif user_input.startswith('a') or user_input.startswith('r'):
             do_repeat = False
-            if info.startswith('r'):
-                info = self.saved_package
+            if user_input.startswith('r'):
+                user_input = self.saved_package
                 do_repeat = True
             else:
-                info = ""
-                while info == "":
+                user_input = ""
+                while user_input == "":
                     prompt_user('Package(s) affected? ')
                     if package == "":
                         package = self.saved_package
                     if package != "":
                         prompt_user(f'[{package}] ')
-                    info = sys.stdin.readline().strip()
-                    if info == '':
-                        info = package
-            self.saved_package = info
+                    user_input = sys.stdin.readline().strip()
+                    if user_input == '':
+                        user_input = package
+            self.saved_package = user_input
 
-            dst = self.add_cve(cve, info.split(), None)
+            dst = self.add_cve(cve, user_input.split(), None)
 
             if do_repeat:
                 subprocess.call(['./scripts/active_dup', self.saved_cve, cve])
@@ -1048,11 +1047,11 @@ class CVEHandler(xml.sax.handler.ContentHandler):
             self.saved_cve = cve
 
             print('\n===================== Dependant packages ======================')
-            print(f' Detecting packages built using: {info}...', end='')
+            print(f' Detecting packages built using: {user_input}...', end='')
             sys.stdout.flush()
             built_using = ""
             try:
-                built_using = get_built_using(info)
+                built_using = get_built_using(user_input)
             except Exception as e:
                 print(f"ERROR: {e}", file=sys.stderr)
                 pass  # for now just show the error but don't break triage
@@ -1062,13 +1061,13 @@ class CVEHandler(xml.sax.handler.ContentHandler):
                 print(source_map.get_built_using_header())
                 print(built_using)
                 print("IMPORTANT: the above packages are candidates for rebuilds when fixes are applied to:")
-                print(" %s" % "\n ".join(info))
+                print(" %s" % "\n ".join(user_input))
             else:
                 print("none detected")
 
-        elif info.startswith('i'):
-            info = ""
-            while info == "":
+        elif user_input.startswith('i'):
+            ignored_reason = ""
+            while ignored_reason == "":
                 print('Reason to be ignored?')
                 prompts = []
 
@@ -1085,24 +1084,23 @@ class CVEHandler(xml.sax.handler.ContentHandler):
                     print(f"   {chr(97 + i)}) {prompts[i]}")
                 prompt_user(' > ')
 
-                info = sys.stdin.readline().strip()
-                if len(info) == 1 and info.isalpha():
+                ignored_reason_input = sys.stdin.readline().strip()
+                # NOTE: user is selecting a choice from prompts
+                if len(ignored_reason_input) == 1 and ignored_reason_input.isalpha():
                     try:
                         # ord('a') == 97
-                        info = prompts[ord(info) - 97]
+                        ignored_reason = prompts[ord(ignored_reason_input) - 97]
                     except IndexError:
                         print('\nError: invalid reason.\n')
-                        info = ""
-                # Enter defaults to only suggestion if only one exists
-                elif len(info) == 0 and len(prompts) == 1:
-                    info = prompts[0]
-                elif len(info) < 3:  # Fat fingers protection
+                # TODO: reassess if < 2 is a better value
+                #       or add a mechanism to catch certain 3 letter words
+                #       e.g., IBM is currently invalid
+                elif len(ignored_reason_input) < 3:  # Fat fingers protection
                     print('\nError: Reason must be at least 3 characters long!\n')
-                    info = ""
-            self.saved_ignore_cache.insert(info)
-            self.ignore_cve(cve, info)
+            self.saved_ignore_cache.insert(ignored_reason)
+            self.ignore_cve(cve, ignored_reason)
 
-        elif info.startswith('s'):
+        elif user_input.startswith('s'):
             self.skip_cve()
         print('')
 
@@ -1146,7 +1144,7 @@ class CVEHandler(xml.sax.handler.ContentHandler):
                 if priority not in cve_lib.priorities and not priority == 'untriaged':
                     raise ValueError(f'Invalid priority on line {line_num}:\n{orig_line}')
 
-                if os.path.exists(f'{destdir}/active/{cve}'):
+                if os.path.exists(f'{DEST_DIR}/active/{cve}'):
                     raise ValueError(f'Updating an existing CVE is not supported (line {line_num}):\n{orig_line}')
 
                 if preprocess:
@@ -1160,7 +1158,7 @@ class CVEHandler(xml.sax.handler.ContentHandler):
                 if action == 'edit':
                     _spawn_editor(cve_path)
             elif action == 'unembargo':
-                if cve not in EmbargoList:
+                if cve not in CVE_EMBARGO_LIST:
                     raise ValueError(f'CVE {cve} is not in the embargo database (line {line_num}):\n{orig_line}')
 
                 if os.path.exists(os.path.join('active', cve)):
@@ -1198,7 +1196,7 @@ class CVEHandler(xml.sax.handler.ContentHandler):
         desc = ''
 
         # Check if this was once an embargoed issue
-        if cve in EmbargoList:
+        if cve in CVE_EMBARGO_LIST:
             desc += '# **!!** no longer embargoed **!!**\n'
             desc += '# ==========================details from embargo entry==========================\n'
             with open(os.path.join('embargoed', cve)) as f:
@@ -1238,7 +1236,7 @@ class CVEHandler(xml.sax.handler.ContentHandler):
         action = 'skip'
         data = ""
         # Skip CVEs that are obviously not about Ubuntu
-        for s in ignore_strings:
+        for s in IGNORE_STRINGS:
             if self.cve_data[cve]['desc'].find(s) >= 0 and self.cve_data[cve]['desc'].find("Linux") < 0:
                 action = 'ignore'
                 data = s
@@ -1255,11 +1253,11 @@ class CVEHandler(xml.sax.handler.ContentHandler):
 
     def add_cve(self, cve, packages, priority=None):
         # remove from not-for-us.txt if adding and ensure we remove any
-        # mistriaged_hint from the description
-        if cve in CVEIgnoreNotForUsSet:
+        # MISTRIAGED_HINT from the description
+        if cve in CVE_IGNORE_NFU_SET:
             cmd = ['sed', '-i', f'/^{cve} #.*$/d', './ignored/not-for-us.txt']
             subprocess.call(cmd)
-            self.cve_data[cve]['desc'] = self.cve_data[cve]['desc'].replace(mistriaged_hint, '')
+            self.cve_data[cve]['desc'] = self.cve_data[cve]['desc'].replace(MISTRIAGED_HINT, '')
 
         # Build up list of reference urls
         ref_urls = []
@@ -1376,10 +1374,10 @@ class CVEHandler(xml.sax.handler.ContentHandler):
         self.num_added += 1
 
     def ignore_cve(self, cve, reason):
-        # Append to ignore list unless is already in CVEIgnoreList and then
+        # Append to ignore list unless is already in cve_ignore_list and then
         # append to the ignored/ignore-mistriaged.txt
-        txtfile = 'ignore-mistriaged.txt' if cve in CVEIgnoreNotForUsSet else 'not-for-us.txt'
-        with open(f'{destdir}/ignored/{txtfile}', 'a') as f:
+        txtfile = 'ignore-mistriaged.txt' if cve in CVE_IGNORE_NFU_SET else 'not-for-us.txt'
+        with open(f'{DEST_DIR}/ignored/{txtfile}', 'a') as f:
             f.write(f'{cve} # {reason}\n')
 
         self.num_ignored += 1
@@ -1388,74 +1386,74 @@ class CVEHandler(xml.sax.handler.ContentHandler):
         self.num_skipped += 1
 
 
-ignored_notforus_path = 'ignored/not-for-us.txt'
-if destdir != './' and destdir != '.':
-    ignored_notforus_path = os.path.join(destdir, ignored_notforus_path)
-# CVEIgnoreNotForUsSet is a set of all CVEs that we have previously
+IGNORED_NFU_PATH = 'ignored/not-for-us.txt'
+if DEST_DIR != './' and DEST_DIR != '.':
+    IGNORED_NFU_PATH = os.path.join(DEST_DIR, IGNORED_NFU_PATH)
+# CVE_IGNORE_NFU_SET is a set of all CVEs that we have previously
 # chosen to ignore since they don't apply to software in Ubuntu
-CVEIgnoreNotForUsSet = set(cve_lib.parse_CVEs_from_uri(ignored_notforus_path))
+CVE_IGNORE_NFU_SET = set(cve_lib.parse_CVEs_from_uri(IGNORED_NFU_PATH))
 
-ignored_mistriaged_path = 'ignored/ignore-mistriaged.txt'
-if destdir != './' and destdir != '.':
-    ignored_mistriaged_path = os.path.join(destdir, ignored_mistriaged_path)
-# CVEIgnoreMistriagedSet is a set of all CVEs that we want to definitely
+IGNORED_MISTRIAGED_PATH = 'ignored/ignore-mistriaged.txt'
+if DEST_DIR != './' and DEST_DIR != '.':
+    IGNORED_MISTRIAGED_PATH = os.path.join(DEST_DIR, IGNORED_MISTRIAGED_PATH)
+# CVE_IGNORE_MISTRIAGED_LIST is a set of all CVEs that we want to definitely
 # ignore when doing mistriaged CVE detection - they should exist in both
 # CVEIgnoreNotForUsList and CVEIgnoreMistriagedList
-CVEIgnoreMistriagedSet = set(cve_lib.parse_CVEs_from_uri(ignored_mistriaged_path))
+CVE_IGNORE_MISTRIAGED_LIST = set(cve_lib.parse_CVEs_from_uri(IGNORED_MISTRIAGED_PATH))
 
-# CVEIgnoreList is a list of all CVEs we know about already.  These will be
+# cve_ignore_list is a list of all CVEs we know about already.  These will be
 # ignored when checking MITRE for new CVEs
-CVEIgnoreList = list(CVEIgnoreNotForUsSet)
+cve_ignore_list = list(CVE_IGNORE_NFU_SET)
 
-CVEKnownList = []
-CVEKnownList += [cve for cve in os.listdir(destdir + "/ignored/") if cve.startswith('CVE-')]
-CVEKnownList += [cve for cve in os.listdir(destdir + "/retired/") if cve.startswith('CVE-')]
-(ActiveList, EmbargoList) = cve_lib.get_cve_list()
-CVEKnownList += [cve for cve in ActiveList if cve not in EmbargoList]
+cve_known_list = []
+cve_known_list += [cve for cve in os.listdir(DEST_DIR + "/ignored/") if cve.startswith('CVE-')]
+cve_known_list += [cve for cve in os.listdir(DEST_DIR + "/retired/") if cve.startswith('CVE-')]
+(CVE_ACTIVE_LIST, CVE_EMBARGO_LIST) = cve_lib.get_cve_list()
+cve_known_list += [cve for cve in CVE_ACTIVE_LIST if cve not in CVE_EMBARGO_LIST]
 
 if not args.refresh and not args.mistriaged and not args.score_refresh:
-    CVEIgnoreList += CVEKnownList
+    cve_ignore_list += cve_known_list
 
 if args.known:
-    cvelist = CVEIgnoreList
+    cvelist = cve_ignore_list
     if args.skip_nfu:
-        cvelist = CVEKnownList
+        cvelist = cve_known_list
     for cve in sorted(cvelist):
         print(cve)
     sys.exit(0)
 
 parser = xml.sax.make_parser()
-handler = CVEHandler(CVEIgnoreList)
+handler = CVEHandler(cve_ignore_list)
 parser.setContentHandler(handler)
 
 # if has specified to triage only specific CVEs, check these are not
 # ignored
-specific_cves = None
+SPECIFIC_CVES = None
 if args.cve:
-    specific_cves = set()
+    SPECIFIC_CVES = set()
     for cve in args.cve.split(","):
         # ignore empty CVE
         if cve.strip() == "":
             continue
         # error out if is ignored
-        if cve in CVEIgnoreList:
+        if cve in cve_ignore_list:
             print(f"{cve} already exists in UCT - please remove it then retriage.")
             sys.exit(1)
-        specific_cves.add(cve)
+        SPECIFIC_CVES.add(cve)
 
-untriaged_json = ""
+UNTRIAGED_JSON = ""
 if args.untriaged:
-    untriaged_json = read_locate_cves_output(args.untriaged)
-    args.uris.append(untriaged_json)
+    UNTRIAGED_JSON = read_locate_cves_output(args.untriaged)
+    args.uris.append(UNTRIAGED_JSON)
 
 if args.mbox:
-    untriaged_json = read_mbox_file(args.mbox)
-    args.uris.append(untriaged_json)
+    UNTRIAGED_JSON = read_mbox_file(args.mbox)
+    args.uris.append(UNTRIAGED_JSON)
 
 rhel8oval_import_json = ""
 if args.rhel8oval:
-    untriaged_json = read_rhel8oval_file(args.rhel8oval)
-    args.uris.append(untriaged_json)
+    UNTRIAGED_JSON = read_rhel8oval_file(args.rhel8oval)
+    args.uris.append(UNTRIAGED_JSON)
 
 debian_import_json = ""
 if (args.import_missing_debian or args.mistriaged) and handler.debian is not None:
@@ -1483,8 +1481,8 @@ for uri in args.uris:
     print('')
 
 # Leaving our fake json around is icky
-if os.path.exists(untriaged_json):
-    os.unlink(untriaged_json)
+if os.path.exists(UNTRIAGED_JSON):
+    os.unlink(UNTRIAGED_JSON)
 if os.path.exists(debian_import_json):
     os.unlink(debian_import_json)
 
@@ -1508,7 +1506,7 @@ def refresh_cves(cve_refresh_list, full_refresh=True):
         # Find the on-disk CVE file
         cvefile = ""
         for status in ['active', 'retired', 'ignored']:
-            check = f'{destdir}/{status}/{cve}'
+            check = f'{DEST_DIR}/{status}/{cve}'
             if os.path.exists(check):
                 cvefile = check
                 break
@@ -1568,10 +1566,10 @@ def refresh_cves(cve_refresh_list, full_refresh=True):
 
 
 if args.refresh or args.score_refresh:
-    if args.cve and specific_cves is not set():
-        cve_refresh_list = specific_cves
+    if args.cve and SPECIFIC_CVES is not set():
+        cve_refresh_list = SPECIFIC_CVES
     else:
-        cve_refresh_list = CVEKnownList
+        cve_refresh_list = cve_known_list
 
     # with OptParse args.refresh and args.score_refresh will each
     # either be True or None. We want full_refresh to be False when
@@ -1590,12 +1588,12 @@ if experimental:
     handler.display_command_file_usage(fout, '# ')
 
 for cve in new_cves:
-    if args.cve and cve not in specific_cves:
+    if args.cve and cve not in SPECIFIC_CVES:
         # ignore this cve
         continue
     # if this got marked as mistriaged, probablistically choose it for
     # processing
-    if mistriaged_hint in handler.cve_data[cve]['desc']:
+    if MISTRIAGED_HINT in handler.cve_data[cve]['desc']:
         if args.mistriaged == 0:
             # ignore this one
             continue