Re: improving the report-vuln script
On 2017-03-31 06:07:08, Salvatore Bonaccorso wrote:
> 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.
Interesting! I couldn't figure out how to use reportbug
productively... surely it would be nice to have a better tip for this in
the file itself...
> On Thu, Mar 30, 2017 at 03:04:39PM -0400, Antoine Beaupré wrote:
[...]
>> --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.
I found --version confusing, especially since there was already
"include_version" in the code (which was even more confusing, because it
actually meant "include blanks".
Furthermore, --version, by convention, shows the script version number
and users may not expect it to actually run the script (a bit like
--help).
>> --severity SEVERITY severity (default: grave)
>
> I'm not sure: I think severity important as default would be safer.
Sure. I just kept the existing default.
> 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.
I agree with the proposed change, makes sense. It's one of the reasons I
added the flag in the first place - I didn't want to file RC bugs for
issues that were no-dsa...
>> --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).
Typical. :)
>> ['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.
Okay, this is the heuristic I was looking for. How about we explain this
in the "epilog" of the usage or at least somewhere?
>> 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.
Right.
>> 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.
No problem, of course. I wish I could have pushed this to a feature
branch or something... ;)
>> Attached is the patch for the new features and a fresh copy which is
>> only twice as long as the diff...
>
> Thanks. Handy :)
let's see...
>> --- ../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).
I didn't realize this, but that is correct.
We may also be at a point where it may make more sense to just pass the
"args" namespace down into gen_text instead of ever expanding the list
of arguments...
> 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.
I agree. I just avoided changing policy for the first pass. This is the
second reason why I started to patch the script. (The first reason being
that I didn't understand what was going on with the blanks stuff. :) )
>> 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)
I also thought about this: I thought we could have a binary "--source /
--package" flag (or "--source / --no-source", anyways like we use the
NegateAction now) that would default to source, changing, again,
policy. As it turns out, the second test I did on this package, for
guacamole-client, *was* binary package specific, so "source" was not
technically correct, and could have introduced significant confusion. So
I left the current setting, but it would make perfect sense to have this
changeable as well.
>> - 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).
i'd vote for "affected" as it's unambiguous. we could then also have
"--fixed" if we so desire. ;)
[...]
>> + 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.
as you noticed, this is simply a matter of reversing the --cc / --no-cc
flags to change the default.
>> + 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".
okay.
>> + 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 :)
no change just yet. i did clone the SVN into git again, and may be able
to create a feature branch for this somewhere we could work on, although
i shiver of thinking who would host our 1GB git repo. ;)
> Thanks again for the time invested to improve report-vuln.
Thanks for your review!
A.
PS: I noticed you cc'd submit@bugs.debian.org, was that intentional? If
so, what's the issue number that was generated?
PPS: I wonder what you think of the NegateAction. It's something I wrote
for another project, inspired by some random stuff I found on stack
overflow, IIRC. I wonder where/if I should publish this more
officially..
--
When I came back to the United States, I decided that if you could use
propaganda for war, you could certainly use it for peace. And
"propaganda" got to be a bad word because of the Germans using it, so
what I did was to try and find some other words so we found the words
"public relations". - Edward Bernays
Reply to: