[Reportbug-maint] Bug#878088: Bug#878088: reportbug: please inform security and lts teams about security update regressions

[Guido Günther]

Hi Markus,
thanks for pursuing this! Some minor nitpicks:

On Mon, Jan 29, 2018 at 12:11:03AM +0100, Markus Koschany wrote:
[..snip..]
> +            if utils.is_security_update(package, pkgversion):
> +                if ui.yes_no('Do you want to report a regression because of a security update? ',
> +                             'Yes, please inform the LTS and security teams.',
> +                             'No or I am not sure.', True):
> +                    regex = re.compile('(\+|~)deb(\d+)u(\d+)')
> +                    secversion = regex.search(pkgversion)
> +                    distnumber = secversion.group(2)
> +                    support = 'none'

Using

   support = None

is a bit more pythonic.

> +                    email_address = []
> +                    try:
> +                        r = requests.get('https://security-tracker.debian.org/tracker/distributions.json',
> +                                    timeout=self.options.timeout)

This will not catch 404 or similar http status codes. If you don't care
about the type of error you can just do …

                           r.raise_for_status()

… ff you dont handle the error this …

> +                        data = r.json()

… will fail like:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python2.7/dist-packages/requests/models.py", line 892, in json
    return complexjson.loads(self.text, **kwargs)
  File "/usr/lib/python2.7/dist-packages/simplejson/__init__.py", line 518, in loads
    return _default_decoder.decode(s)
  File "/usr/lib/python2.7/dist-packages/simplejson/decoder.py", line 370, in decode
    obj, end = self.raw_decode(s)
  File "/usr/lib/python2.7/dist-packages/simplejson/decoder.py", line 400, in raw_decode
    return self.scan_once(s, idx=_w(s, idx).end())
simplejson.errors.JSONDecodeError: Expecting value: line 1 column 1 (char 0)


> +                        for key, value in data.items():
> +                            if distnumber == value['major-version']:
> +                                if value['support']:
> +                                    support = value['support']
> +                                if value['contact']:
> +                                    email_address = value['contact']
> +
> +                        if support != 'none':

(With the abouve "support = None"):

                           if support is not None:

> +                            listcc += [email_address]
> +

… The raise_for_status exception is caught here alreadyx since
requests.exceptions.HTTPError is a subclass of
requests.exceptions.RequestException):


> +                    except requests.exceptions.RequestException:     

> +                        ewrite('Unable to connect to security-tracker.debian.org.\n'
> +                               'Please try again later or contact the LTS or security team via email directly.\n')
> +

It would also improve readability if this whole code block moved into a get_security_contact()
function.

Cheers,
 -- Guido


>          if severity and rtype:
>              severity = debbugs.convert_severity(severity, rtype)
>  
> diff -Nru reportbug-7.1.8/debian/changelog reportbug-7.1.8+nmu1/debian/changelog
> --- reportbug-7.1.8/debian/changelog	2017-12-29 05:25:43.000000000 +0100
> +++ reportbug-7.1.8+nmu1/debian/changelog	2018-01-23 20:43:14.000000000 +0100
> @@ -1,3 +1,10 @@
> +reportbug (7.1.8+nmu1) UNRELEASED; urgency=medium
> +
> +  * Non-maintainer upload.
> +  * 
> +
> + -- Markus Koschany <apo at debian.org>  Tue, 23 Jan 2018 20:43:14 +0100
> +
>  reportbug (7.1.8) unstable; urgency=medium
>  
>    * reportbug/debbugs.py
> diff -Nru reportbug-7.1.8/reportbug/utils.py reportbug-7.1.8+nmu1/reportbug/utils.py
> --- reportbug-7.1.8/reportbug/utils.py	2017-12-29 05:25:43.000000000 +0100
> +++ reportbug-7.1.8+nmu1/reportbug/utils.py	2018-01-23 20:43:14.000000000 +0100
> @@ -39,6 +39,8 @@
>  import socket
>  import subprocess
>  import pipes
> +import apt
> +import gzip
>  
>  from .urlutils import open_url
>  from string import ascii_letters, digits
> @@ -1333,3 +1335,80 @@
>                      break
>  
>      return lsminfo
> +
> +
> +def is_security_update(pkgname, pkgversion):
> +    """Determine whether a given package is a security update.
> +
> +    Detection of security update versions works most reliably if the
> +    package version under investigation is the currently installed
> +    version.  If this is not the case, the probability of false
> +    negatives increases.
> +
> +    Parameters
> +    ----------
> +    pkgname : str
> +        package name
> +    pkgversion : str
> +        package version
> +
> +    Returns
> +    -------
> +    bool
> +        True if there is evidence that this version is a security
> +        update, otherwise False
> +    """
> +    # Check 1:
> +    # If it does not follow the debXuY version number pattern, it is
> +    # definitely no security update.
> +    #
> +    # This check is not sufficient to detect security updates reliably,
> +    # since other stable updates also use the same version pattern.
> +    regex = re.compile('(\+|~)deb(\d+)u(\d+)')
> +    secversion = regex.search(pkgversion)
> +    if not secversion:
> +        return False
> +
> +    # Check 2:
> +    # If the package comes from the Debian-Security package source, it
> +    # is definitely a security update.
> +    #
> +    # This check does not identify all security updates, since some of
> +    # them are distributed through the normal channels as part of a
> +    # stable release update.
> +    try:
> +        p = apt.Cache()[pkgname]
> +        if 'Debian-Security' in [o.label for o in
> +                        p.versions[pkgversion].origins]:
> +            return True
> +    except:
> +        pass
> +
> +    # Check 3:
> +    # Inspect the package changelog if it mentions any vulnerability,
> +    # identified by a CVE number, in the section of the latest version.
> +
> +    cl = None
> +    for cl in ['/usr/share/doc/{}/changelog.Debian.gz'.format(pkgname),
> +               '/usr/share/doc/{}/changelog.gz'.format(pkgname)]:
> +        if os.path.exists(cl):
> +            break
> +
> +    try:
> +        with gzip.open(cl, 'rt') as f:
> +            ln = f.readline()
> +            if pkgversion not in ln:
> +                raise KeyError
> +
> +            for ln in f.readlines():
> +                # stop reading at the end of the first section
> +                if ln.rstrip() != '' and (ln.startswith(' -- ') or not ln.startswith(' ')):
> +                    break
> +
> +                if 'CVE-20' in ln.upper():
> +                    return True
> +    except:
> +        pass
> +
> +    # guess 'no security update, but normal stable update' by default
> +    return False