Re: improving the report-vuln script
Hi Antoine,
At the top of the this mail: fist of all thanks for your work on the
report-vuln script. I use it extensively in it's current form in a two
staged workflow. 1. generate a template for reportbug, then feed it
int via reportbug -i ... and do some cleanups before sending the mail.
On Thu, Mar 30, 2017 at 03:04:39PM -0400, Antoine Beaupré wrote:
> Hi,
>
> I had some spare time today and I figured I would look at the backlog of
> unreported vulnerabilities here:
>
> https://security-tracker.debian.org/tracker/status/unreported
>
> This is one of the housekeeping tasks we were told was useful for the
> security team here:
>
> https://wiki.debian.org/LTS/Development#Housekeeping_Tasks
>
> The instructions for using the report-vuln script weren't quite
> accurate: first it can't really be used with reportbug, from what I can
> tell, at least not in a useful way.. Reportbug will still prompt for all
> its usual stuff which makes it less effective.
>
> Furthermore, the recommended use (in the script itself) of Mutt doesn't
> add the necessary headers, and I found the "blanks" argument rather
> confusing.
>
> Therefore, I have rewritten parts of the script to make it easier to
> automate. It's now possible to specify the severity and affected
> versions directly on the commandline, and the usage is much clearer.
>
> Before:
>
> $ ./bin/report-vuln -h
> ./bin/report-vuln [--no-blanks] <pkg> <cve id(s)>
>
> Now:
>
> usage: report-vuln [-h] [--no-blanks] [--affected AFFECTED]
> [--severity SEVERITY] [--no-cc] [--cc-list CCLIST]
> pkg cve [cve ...]
>
> positional arguments:
> pkg affected package
> cve relevant CVE for this issue, may be used multiple time
> if the issue has multiple CVEs
>
> optional arguments:
> -h, --help show this help message and exit
> --no-blanks, --blanks
> include blank fields to be filled (default: False)
I need to think/test about this a bit.
> --affected AFFECTED affected version (default: unspecified)
This one might be a bit confusing: Would you change it to --version?
Does this makes sense? or is --version more confusing because
interpreted as to show version of report-vuln? So we might leave it as
--affected.
> --severity SEVERITY severity (default: grave)
I'm not sure: I think severity important as default would be safer.
The severity is often choosed after evaluation really to grave. But I
think it should always be a deliberate choice to set a RC severity. So
I would tend to set default severity to important. This might be
disputed though. The current script actually (if --no-blanks is set)
set it already to grave. So I'm proposing a change here.
> --no-cc, --cc add X-Debbugs-CC header to
> --cc-list CCLIST list of addres to add in CC (default:
Typo: address? (but see below).
> ['team@security.debian.org', 'secure-testing-
> team@lists.alioth.debian.org'])
>
> I was not sure in which case multiple CVEs could be used - should we
> report multiple CVEs in a single bug? that seems like bad
> practice... IMHO, each bug should have its own CVE...
It always depends! There are cases (and sometimes prferences ;-))
where one fills multiple CVEs in one bugreport. This is perfectly
fine. My approach for example is: if I know all reported CVEs are with
known fixes, all supported versions in lower sites are affected as
well for the same set of CVEs, then one bugeport is fine. Otherwise I
try to split them up addording to the found/triaged versions as well
making individual bugs for individual CVEs. But supporting reporting
mutliple CVEs is perfectly fine.
> Ideally, this script would interactively submit the bug and wait for
> confirmation, but it seems this is something that is notoriously hard to
> do in the Debian BTS - I personnally haven't found a way to promptly get
> a bug identifier when submitting bugs other than waiting for the
> autoreply to kick in, which can take some time.
Possibly not feasable in the current form.
> Since this is not something the LTS team directly uses all the time -
> it's more something for the main security team - I figured it would be
> safer to submit it for review here, especially since the changes are
> rather large. Let me know if there's a better place to discuss this if
> this isn't appropriate.
Appreciated. I will try to use your new version and report back to
you, can you please though wait before commiting.
> Attached is the patch for the new features and a fresh copy which is
> only twice as long as the diff...
Thanks. Handy :)
> --- ../secure-testing/bin/report-vuln 2017-03-30 14:17:31.262515489 -0400
> +++ report-vuln 2017-03-30 14:53:50.614772451 -0400
> @@ -19,6 +19,7 @@
> #
> # export http_proxy if you need to use an http proxy to report bugs
>
> +import argparse
> import sys, re, urllib, os
>
> temp_id = re.compile('(?:CVE|cve)\-[0-9]{4}-XXXX')
> @@ -112,7 +113,7 @@
>
> return ret + '\n'
>
> -def gen_text(pkg, cveid, include_version = False, severity = 'FILLINSEVERITY'):
> +def gen_text(pkg, cveid, blanks = False, severity = 'FILLINSEVERITY', affected=None, cc=False, cclist=None):
severity: if I see it correctly you explicitly propose to set a
default, so I guess severity = 'FILLINSEVERITY' is becoming obsolte
and filling a severity when blanks are allowed is not possible
(anymore).
cc=True please. The default should be to always X-Debbugs-CC
team@security.debian.org and
secure-testing-team@lists.alioth.debian.org (this is was reportbug
does if you report a bug with tag security). This is important for
the security team's work.
> vuln_suff = 'y'
> cve_suff = ''
> time_w = 'was'
> @@ -124,8 +125,13 @@
> time_w = 'were'
>
> header = '''Package: %s\n''' % (pkg)
While we are at it: Can we change this to Source field. In the
security tracker we track affected sources. so I as well use reportbug
--src $sourcepackage when reporting bugs. Having the report-vuln do
the "right" (well biased view ...) thing, would be great!
-> header = '''Source: %s\n''' % (pkg)
*But*: If some other security team member (as most consumers of the
script at the moment) do not agree, we can leave it.
Another option: have an optional argument, if --src is passed then the
header is constructed as
header = '''Source: %s\n''' % (pkg)
and when --src not passed, then defaults to
header = '''Package: %s\n''' % (pkg)
> - if include_version:
> - header += 'Version: FILLINAFFECTEDVERSION\n'
> + if affected is None:
> + if blanks:
> + header += "Version: FILLINAFFECTEDVERSION\n"
> + else:
> + header += "Version: %s\n" % affected
> + if cc and len(cclist) > 0:
> + header += "X-Debbugs-CC: %s\n" % " ".join(cclist)
As discussed: might it be better to word --affected as --version?
(redundant disucssion here, always asked above).
> header += '''Severity: %s
> Tags: security
>
> @@ -160,31 +166,54 @@
> print '\nhttps://security-tracker.debian.org/tracker/source-package/%s' % (pkg)
> print '(issues without CVE id are assigned a TEMP one, but it may change over time)\n'
>
> - if not include_version:
> - print '''Please adjust the affected versions in the BTS as needed.\n'''
> + if not blanks:
> + print '''\nPlease adjust the affected versions in the BTS as needed.\n'''
>
> def error(msg):
> print 'error: ' + msg
> sys.exit(1)
>
> -def usage():
> - print sys.argv[0], '[--no-blanks] <pkg> <cve id(s)>'
> - sys.exit(0)
> +class NegateAction(argparse.Action):
> + '''add a toggle flag to argparse
> +
> + this is similar to 'store_true' or 'store_false', but allows
> + arguments prefixed with --no to disable the default. the default
> + is set depending on the first argument - if it starts with the
> + negative form (define by default as '--no'), the default is False,
> + otherwise True.
> + '''
> +
> + negative = '--no'
> +
> + def __init__(self, option_strings, *args, **kwargs):
> + '''set default depending on the first argument'''
> + default = not option_strings[0].startswith(self.negative)
> + super(NegateAction, self).__init__(option_strings, *args,
> + default=default, nargs=0, **kwargs)
> +
> + def __call__(self, parser, ns, values, option):
> + '''set the truth value depending on whether
> + it starts with the negative form'''
> + setattr(ns, self.dest, not option.startswith(self.negative))
>
> -def main():
> - if len(sys.argv) < 3:
> - usage()
>
> - blanks = True
> - if sys.argv[1] == '--no-blanks':
> - if len(sys.argv) < 4:
> - usage()
> - blanks = False
> - pkg = sys.argv[2]
> - cve = sys.argv[3:]
> - else:
> - pkg = sys.argv[1]
> - cve = sys.argv[2:]
> +def main():
> + parser = argparse.ArgumentParser()
> + parser.add_argument('--no-blanks', '--blanks', dest='blanks', action=NegateAction,
> + help='include blank fields to be filled (default: %(default)s)')
> + parser.add_argument('--affected', help='affected version (default: unspecified)')
> + parser.add_argument('--severity', default='grave', help='severity (default: %(default)s)')
See discussion above about the default.
> + parser.add_argument('--no-cc', '--cc', dest='cc', action=NegateAction,
> + help='add X-Debbugs-CC header to')
The default should be here to result in a always X-Debbugs-CC to
team@security.debian.org,secure-testing-team@lists.alioth.debian.org
unless someone specifies another cc-list or deliberately choosed
--no-cc.
> + parser.add_argument('--cc-list', dest='cclist', default=['team@security.debian.org', 'secure-testing-team@lists.alioth.debian.org'],
> + help='list of addres to add in CC (default: %(default)s)')
> + parser.add_argument('pkg', help='affected package')
> + parser.add_argument('cve', nargs='+', help='relevant CVE for this issue, may be used multiple time if the issue has multiple CVEs')
-> "relevant CVE for this package" instead of "relevant CVE for this
issue".
> + args = parser.parse_args()
> +
> + blanks = args.blanks
> + pkg = args.pkg
> + cve = args.cve
>
> # check for valid parameters
> p = re.compile('^[0-9a-z].*')
> @@ -197,10 +226,7 @@
> if not c.match(arg) and not temp_id.match(arg):
> error(arg + ' does not seem to be a valid CVE id')
>
> - if blanks:
> - gen_text(pkg, cve)
> - else:
> - gen_text(pkg, cve, False, 'grave')
> + gen_text(pkg, cve, affected=args.affected, blanks=args.blanks, severity=args.severity, cc=args.cc, cclist=args.cclist)
Now I see, since the severity is now explicitly either set or
defaults, filling it when blanks are alowed is not possible anymore.
Might be fine. But see above discussion about default severity.
As said I will try to work with your new script as I use it for
reporting bugs to be added to the security tracker. If you have a new
version available please attach :)
Thanks again for the time invested to improve report-vuln.
Regards,
Salvatore
Reply to: