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. :) >> ['t...@security.debian.org', 'secure-testing- >> t...@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 > t...@security.debian.org and > secure-testing-t...@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 > t...@security.debian.org,secure-testing-t...@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=['t...@security.debian.org', >> 'secure-testing-t...@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 sub...@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