Обсуждение: CVE details page

Hi,

When we have a release that contains CVEs, we currently link to a CVE
authority to display the full details about that CVE. This has presented
a few issues:

- The CVE authority does not publish the CVE details when the release is
made; the window for this happening can vary
- As a result, we can't link to that page from the news announcement;
when we have in the past, we'll get reports about the URL 404ing

This patchset aims to remedy this by creating a page that houses the
details about a CVE. It includes the additional description that is
provided to the CVE authority and allows for the details to be published
as soon as the CVE is published. See attached screenshot.

0001 updates the current CVE ID validator to match what MITRE has put
forth on the numbering (7 digits! It does say in places it can be
"arbitrary amounts" but the official examples go up to 7 digits), and
0002 refactors a function we used to generate our internal CVE IDs so it
can be used in multiple places, e.g. its use in 0003.

The security team has reviewed the proposed visual contents and has
given its consent.

Thanks,

Jonathan

On Mon, Mar 22, 2021 at 4:43 PM Jonathan S. Katz <jkatz@postgresql.org> wrote:
>
> Hi,
>
> When we have a release that contains CVEs, we currently link to a CVE
> authority to display the full details about that CVE. This has presented
> a few issues:
>
> - The CVE authority does not publish the CVE details when the release is
> made; the window for this happening can vary
> - As a result, we can't link to that page from the news announcement;
> when we have in the past, we'll get reports about the URL 404ing
>
> This patchset aims to remedy this by creating a page that houses the
> details about a CVE. It includes the additional description that is
> provided to the CVE authority and allows for the details to be published
> as soon as the CVE is published. See attached screenshot.
>
> 0001 updates the current CVE ID validator to match what MITRE has put
> forth on the numbering (7 digits! It does say in places it can be
> "arbitrary amounts" but the official examples go up to 7 digits), and

This one should probably change the error message as well?


> 0002 refactors a function we used to generate our internal CVE IDs so it
> can be used in multiple places, e.g. its use in 0003.

I applaud you for adding what may be the first docstring in pgweb :)

I don't think you need to be consistent with the previous error since
it's a "never happens" error, you can just let the ValidationError
through. I also don't mind if you prefer keeping it :)

0003
* can we make the purging a bit more specific? That is only purge the
actually edited one? See for example how news/ does it.

* is there really a need to support case insensitive cve in the URL?
We don't support case insensitive URLs anywhere else... I suggest also
making the URLs we generate ourselves be lowercase, even if we keep
the insensitivity in the matching

* The query for "versions" needs a .elect_related('version')

Rest LGTM. (did not review the HTML itself, but since the output looks
good and has already been approved..)

-- 
 Magnus Hagander
 Me: https://www.hagander.net/
 Work: https://www.redpill-linpro.com/

On 3/24/21 2:26 PM, Magnus Hagander wrote:
> On Mon, Mar 22, 2021 at 4:43 PM Jonathan S. Katz <jkatz@postgresql.org> wrote:
>>
>> Hi,
>>
>> When we have a release that contains CVEs, we currently link to a CVE
>> authority to display the full details about that CVE. This has presented
>> a few issues:
>>
>> - The CVE authority does not publish the CVE details when the release is
>> made; the window for this happening can vary
>> - As a result, we can't link to that page from the news announcement;
>> when we have in the past, we'll get reports about the URL 404ing
>>
>> This patchset aims to remedy this by creating a page that houses the
>> details about a CVE. It includes the additional description that is
>> provided to the CVE authority and allows for the details to be published
>> as soon as the CVE is published. See attached screenshot.
>>
>> 0001 updates the current CVE ID validator to match what MITRE has put
>> forth on the numbering (7 digits! It does say in places it can be
>> "arbitrary amounts" but the official examples go up to 7 digits), and
>
> This one should probably change the error message as well?

Yeah, though I think there's only one person who reads it at this point.
However, for that one person, I have adjusted the message.

>> 0002 refactors a function we used to generate our internal CVE IDs so it
>> can be used in multiple places, e.g. its use in 0003.
>
> I applaud you for adding what may be the first docstring in pgweb :)

There's some others that I've added! This may be the first one you caught ;)

> I don't think you need to be consistent with the previous error since
> it's a "never happens" error, you can just let the ValidationError
> through. I also don't mind if you prefer keeping it :)

I prefer brevity, so I removed the reraise and updated the comment.

> 0003
> * can we make the purging a bit more specific? That is only purge the
> actually edited one? See for example how news/ does it.

Done.

> * is there really a need to support case insensitive cve in the URL?

...I'm not quite sure what possessed me there. I think it's the fact
that most sites tend to use the capital letters for CVE, both in the
URLs and the listings, so one copying/pasting would copy that directly.

> We don't support case insensitive URLs anywhere else... I suggest also
> making the URLs we generate ourselves be lowercase, even if we keep
> the insensitivity in the matching

I would suggest the opposite, that we keep it uppercase as this seems
consistent with how the others in the CVE game do it, e.g.

https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2018-10925
https://nvd.nist.gov/vuln/detail/CVE-2018-10925
https://access.redhat.com/security/cve/CVE-2018-1058

I've modified the URL matching to be all uppercase, but keeping our
matching logic case insensitive.

> * The query for "versions" needs a .elect_related('version')

That I do agree with and somehow missed that. Thanks!

> Rest LGTM. (did not review the HTML itself, but since the output looks
> good and has already been approved..)

Cool. Please see updated patches.

Thanks,

Jonathan

On Wed, Mar 24, 2021 at 8:57 PM Jonathan S. Katz <jkatz@postgresql.org> wrote:
>
> On 3/24/21 2:26 PM, Magnus Hagander wrote:
> > On Mon, Mar 22, 2021 at 4:43 PM Jonathan S. Katz <jkatz@postgresql.org> wrote:
> >>
> >> 0002 refactors a function we used to generate our internal CVE IDs so it
> >> can be used in multiple places, e.g. its use in 0003.
> >
> > I applaud you for adding what may be the first docstring in pgweb :)
>
> There's some others that I've added! This may be the first one you caught ;)

Guilty as charged :)


> > * is there really a need to support case insensitive cve in the URL?
>
> ...I'm not quite sure what possessed me there. I think it's the fact
> that most sites tend to use the capital letters for CVE, both in the
> URLs and the listings, so one copying/pasting would copy that directly.

If we d it, we should really support (cve|CVE), not cVe for example.

There might be a point in supporting both "cve" and "CVE" but in that
case making it redirect to the canonical form.

(We all know the issues of having the same thing on multiple URLs)

The more I think about it, the more such a redirect seems like a good idea...


> > We don't support case insensitive URLs anywhere else... I suggest also
> > making the URLs we generate ourselves be lowercase, even if we keep
> > the insensitivity in the matching
>
> I would suggest the opposite, that we keep it uppercase as this seems
> consistent with how the others in the CVE game do it, e.g.
>
> https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2018-10925

That's a query param though, so that's slightly different. The fact
that it's a classic cgi also says a few things about the age :)


> https://nvd.nist.gov/vuln/detail/CVE-2018-10925

fair enough.

> https://access.redhat.com/security/cve/CVE-2018-1058

If that's not a beautiful contradiction of the two ways to do it, I
don't know what is :)


> I've modified the URL matching to be all uppercase, but keeping our
> matching logic case insensitive.

I do still prefer lowercase, but not enough to insist on it :)

But do consider the redirect, that might help some ppl.


> > * The query for "versions" needs a .elect_related('version')
>
> That I do agree with and somehow missed that. Thanks!

Thinking more, we should also have a struct.py in this directory, so
it goes ni the sitemap and becomes searchable. We should *already*
have had that, but it becomes more importantn ow that we have >1 page.
But already today you won't actually get search hits in our security
listing, which is a problem in itself... But let's fix them both at
once.

//Magnus

On 3/25/21 8:20 AM, Magnus Hagander wrote:
> On Wed, Mar 24, 2021 at 8:57 PM Jonathan S. Katz <jkatz@postgresql.org> wrote:
>>
>> I've modified the URL matching to be all uppercase, but keeping our
>> matching logic case insensitive.
>
> I do still prefer lowercase, but not enough to insist on it :)
>
> But do consider the redirect, that might help some ppl.

OK, so I did:

- matching cve/CVE
- added a redirect to rewrite to CVE

>>> * The query for "versions" needs a .elect_related('version')
>>
>> That I do agree with and somehow missed that. Thanks!
>
> Thinking more, we should also have a struct.py in this directory, so
> it goes ni the sitemap and becomes searchable. We should *already*
> have had that, but it becomes more importantn ow that we have >1 page.
> But already today you won't actually get search hits in our security
> listing, which is a problem in itself... But let's fix them both at
> once.

OK, I believe I have handled that. I included it in 0003.

Thanks,

Jonathan

On Sat, Mar 27, 2021 at 8:35 PM Jonathan S. Katz <jkatz@postgresql.org> wrote:
>
> On 3/25/21 8:20 AM, Magnus Hagander wrote:
> > On Wed, Mar 24, 2021 at 8:57 PM Jonathan S. Katz <jkatz@postgresql.org> wrote:
> >>
> >> I've modified the URL matching to be all uppercase, but keeping our
> >> matching logic case insensitive.
> >
> > I do still prefer lowercase, but not enough to insist on it :)
> >
> > But do consider the redirect, that might help some ppl.
>
> OK, so I did:
>
> - matching cve/CVE
> - added a redirect to rewrite to CVE

Wouldn't:
+    if request.path.find('cve') != -1:
+        return redirect('/support/security/CVE-{}/'.format(cve),
permanent=True)

you captured the "cve" vs "CVE" part already in urls.py as a separate
parameter? And then just "if blah != 'CVE' return redirect"?

Either works, but looking for cve anywhere in the URL seems to set up
for a future bug should we ever for example want to move the cve's
into a subdir..


> >>> * The query for "versions" needs a .elect_related('version')
> >>
> >> That I do agree with and somehow missed that. Thanks!
> >
> > Thinking more, we should also have a struct.py in this directory, so
> > it goes ni the sitemap and becomes searchable. We should *already*
> > have had that, but it becomes more importantn ow that we have >1 page.
> > But already today you won't actually get search hits in our security
> > listing, which is a problem in itself... But let's fix them both at
> > once.
>
> OK, I believe I have handled that. I included it in 0003.


Rest LGTM.


-- 
 Magnus Hagander
 Me: https://www.hagander.net/
 Work: https://www.redpill-linpro.com/

> On Mar 28, 2021, at 7:26 AM, Magnus Hagander <magnus@hagander.net> wrote:
>
> On Sat, Mar 27, 2021 at 8:35 PM Jonathan S. Katz <jkatz@postgresql.org> wrote:
>>
>>> On 3/25/21 8:20 AM, Magnus Hagander wrote:
>>> On Wed, Mar 24, 2021 at 8:57 PM Jonathan S. Katz <jkatz@postgresql.org> wrote:
>>>>
>>>> I've modified the URL matching to be all uppercase, but keeping our
>>>> matching logic case insensitive.
>>>
>>> I do still prefer lowercase, but not enough to insist on it :)
>>>
>>> But do consider the redirect, that might help some ppl.
>>
>> OK, so I did:
>>
>> - matching cve/CVE
>> - added a redirect to rewrite to CVE
>
> Wouldn't:
> +    if request.path.find('cve') != -1:
> +        return redirect('/support/security/CVE-{}/'.format(cve),
> permanent=True)
>
> you captured the "cve" vs "CVE" part already in urls.py as a separate
> parameter? And then just "if blah != 'CVE' return redirect"?
>
> Either works, but looking for cve anywhere in the URL seems to set up
> for a future bug should we ever for example want to move the cve's
> into a subdir..

We don’t capture the “cve” string in a variable and I think it’s a bit overkill
to do so. We only capture the numbers.

And if we made that move to a subdir, we’d have to update the URL
pattern anyway, so this seems to be a bit of prefactoring.

So I’m disinclined to change it.

Unless you really think this is a blocker, I’d like to get this merged so
I can start backfilling the data, which will take a hot minute to do.

Thanks,

Jonathan

On 3/28/21 9:44 AM, Jonathan S. Katz wrote:
>
>
>> On Mar 28, 2021, at 7:26 AM, Magnus Hagander <magnus@hagander.net> wrote:
>>
>> On Sat, Mar 27, 2021 at 8:35 PM Jonathan S. Katz <jkatz@postgresql.org> wrote:
>>>
>>>> On 3/25/21 8:20 AM, Magnus Hagander wrote:
>>>> On Wed, Mar 24, 2021 at 8:57 PM Jonathan S. Katz <jkatz@postgresql.org> wrote:
>>>>>
>>>>> I've modified the URL matching to be all uppercase, but keeping our
>>>>> matching logic case insensitive.
>>>>
>>>> I do still prefer lowercase, but not enough to insist on it :)
>>>>
>>>> But do consider the redirect, that might help some ppl.
>>>
>>> OK, so I did:
>>>
>>> - matching cve/CVE
>>> - added a redirect to rewrite to CVE
>>
>> Wouldn't:
>> +    if request.path.find('cve') != -1:
>> +        return redirect('/support/security/CVE-{}/'.format(cve),
>> permanent=True)
>>
>> you captured the "cve" vs "CVE" part already in urls.py as a separate
>> parameter? And then just "if blah != 'CVE' return redirect"?
>>
>> Either works, but looking for cve anywhere in the URL seems to set up
>> for a future bug should we ever for example want to move the cve's
>> into a subdir..
>
> We don’t capture the “cve” string in a variable and I think it’s a bit overkill
> to do so. We only capture the numbers.
>
> And if we made that move to a subdir, we’d have to update the URL
> pattern anyway, so this seems to be a bit of prefactoring.
>
> So I’m disinclined to change it.

Looking at it more closely, I do see how it's "one step away" from
capturing the string value. I do think it's a bit overkill given the
vast majority of traffic will be to the uppercase, but I'm now a bit
more open-minded to applying your suggestion and calling it complete.

Jonathan

On 3/28/21 10:01 AM, Jonathan S. Katz wrote:
> On 3/28/21 9:44 AM, Jonathan S. Katz wrote:
>>
>>
>>> On Mar 28, 2021, at 7:26 AM, Magnus Hagander <magnus@hagander.net> wrote:
>>>
>>> On Sat, Mar 27, 2021 at 8:35 PM Jonathan S. Katz <jkatz@postgresql.org> wrote:
>>>>
>>>>> On 3/25/21 8:20 AM, Magnus Hagander wrote:
>>>>> On Wed, Mar 24, 2021 at 8:57 PM Jonathan S. Katz <jkatz@postgresql.org> wrote:
>>>>>>
>>>>>> I've modified the URL matching to be all uppercase, but keeping our
>>>>>> matching logic case insensitive.
>>>>>
>>>>> I do still prefer lowercase, but not enough to insist on it :)
>>>>>
>>>>> But do consider the redirect, that might help some ppl.
>>>>
>>>> OK, so I did:
>>>>
>>>> - matching cve/CVE
>>>> - added a redirect to rewrite to CVE
>>>
>>> Wouldn't:
>>> +    if request.path.find('cve') != -1:
>>> +        return redirect('/support/security/CVE-{}/'.format(cve),
>>> permanent=True)
>>>
>>> you captured the "cve" vs "CVE" part already in urls.py as a separate
>>> parameter? And then just "if blah != 'CVE' return redirect"?
>>>
>>> Either works, but looking for cve anywhere in the URL seems to set up
>>> for a future bug should we ever for example want to move the cve's
>>> into a subdir..
>>
>> We don’t capture the “cve” string in a variable and I think it’s a bit overkill
>> to do so. We only capture the numbers.
>>
>> And if we made that move to a subdir, we’d have to update the URL
>> pattern anyway, so this seems to be a bit of prefactoring.
>>
>> So I’m disinclined to change it.
>
> Looking at it more closely, I do see how it's "one step away" from
> capturing the string value. I do think it's a bit overkill given the
> vast majority of traffic will be to the uppercase, but I'm now a bit
> more open-minded to applying your suggestion and calling it complete.

And here is patch 0003 with that change.

I'll apply in a bit and start backfilling the data.

Thanks!

Jonathan

On Sun, Mar 28, 2021 at 4:03 PM Jonathan S. Katz <jkatz@postgresql.org> wrote:
>
> On 3/28/21 10:01 AM, Jonathan S. Katz wrote:
> > On 3/28/21 9:44 AM, Jonathan S. Katz wrote:
> >>
> >>
> >>> On Mar 28, 2021, at 7:26 AM, Magnus Hagander <magnus@hagander.net> wrote:
> >>>
> >>> On Sat, Mar 27, 2021 at 8:35 PM Jonathan S. Katz <jkatz@postgresql.org> wrote:
> >>>>
> >>>>> On 3/25/21 8:20 AM, Magnus Hagander wrote:
> >>>>> On Wed, Mar 24, 2021 at 8:57 PM Jonathan S. Katz <jkatz@postgresql.org> wrote:
> >>>>>>
> >>>>>> I've modified the URL matching to be all uppercase, but keeping our
> >>>>>> matching logic case insensitive.
> >>>>>
> >>>>> I do still prefer lowercase, but not enough to insist on it :)
> >>>>>
> >>>>> But do consider the redirect, that might help some ppl.
> >>>>
> >>>> OK, so I did:
> >>>>
> >>>> - matching cve/CVE
> >>>> - added a redirect to rewrite to CVE
> >>>
> >>> Wouldn't:
> >>> +    if request.path.find('cve') != -1:
> >>> +        return redirect('/support/security/CVE-{}/'.format(cve),
> >>> permanent=True)
> >>>
> >>> you captured the "cve" vs "CVE" part already in urls.py as a separate
> >>> parameter? And then just "if blah != 'CVE' return redirect"?
> >>>
> >>> Either works, but looking for cve anywhere in the URL seems to set up
> >>> for a future bug should we ever for example want to move the cve's
> >>> into a subdir..
> >>
> >> We don’t capture the “cve” string in a variable and I think it’s a bit overkill
> >> to do so. We only capture the numbers.
> >>
> >> And if we made that move to a subdir, we’d have to update the URL
> >> pattern anyway, so this seems to be a bit of prefactoring.
> >>
> >> So I’m disinclined to change it.
> >
> > Looking at it more closely, I do see how it's "one step away" from
> > capturing the string value. I do think it's a bit overkill given the
> > vast majority of traffic will be to the uppercase, but I'm now a bit
> > more open-minded to applying your suggestion and calling it complete.
>
> And here is patch 0003 with that change.
>
> I'll apply in a bit and start backfilling the data.

LGTM. Shoot!

--
 Magnus Hagander
 Me: https://www.hagander.net/
 Work: https://www.redpill-linpro.com/