[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

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

>>   --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.


>> 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".


>> +        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!


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

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: