Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Backfill release notes with security fix details #7877

Merged
merged 33 commits into from
Mar 16, 2024
Merged

Conversation

aclark4life
Copy link
Member

@aclark4life aclark4life commented Mar 13, 2024

Resolves #7864

  • Added release notes for past releases: 2.6.0, 2.5.2,2.3.2, 2.3.1.
  • Add suggested CVE format to template
  • Move Security to the top of release notes
  • Fix headings
  • Update all existing CVE notes to match template

@aclark4life aclark4life changed the title 7864 backfill Backfill release notes with security fix details Mar 13, 2024
docs/releasenotes/2.3.1.rst Outdated Show resolved Hide resolved
docs/releasenotes/2.3.1.rst Outdated Show resolved Hide resolved
docs/releasenotes/10.2.0.rst Outdated Show resolved Hide resolved
docs/releasenotes/6.2.0.rst Outdated Show resolved Hide resolved
docs/releasenotes/6.2.2.rst Outdated Show resolved Hide resolved
@aclark4life
Copy link
Member Author

aclark4life commented Mar 13, 2024

Thanks @hugovk and @nulano and @radarhere for the fixes and feedback!

@hugovk
Copy link
Member

hugovk commented Mar 13, 2024

Thanks for this! Definitely makes sense to move the security notes up.

I'm in two minds about adding extra "Fix CVE-YYYY-XXXXX" headers with the "More information about this vulnerability included in database record CVE-2023-44271" note.

Comparison:

Before After
image image

The "Note: More information about this vulnerability included in database record CVE-2023-44271" uses up a lot of space and doesn't really tell us much, it's mainly saying there's a link.

The font sizes aren't that different and it might not be totally clear that "Added ImageFont.MAX_STRING_LENGTH" is a subheading of "Fix CVE-2023-44271". They could think the note is the only thing, and relevant to the CVE.

I think I prefer the original with just a descriptive header and CVE-YYYY-XXXXX: prefix for the paragraph.

@aclark4life
Copy link
Member Author

I think I prefer the original with just a descriptive header and CVE-YYYY-XXXXX: prefix for the paragraph.

Me too although I'm trying to overemphasize CVE reporting in the release notes so I can pass to Tidelift for review so they can compare before and after lifting. We can probably come up with something in the middle. Let me think about it and try to fix when I add the remaining back fills.

One possibility is

CVE-YYYY-XXXXX

Instead of

Fix CVE-YYYY-XXXXX

Because the fix is probably implied.

As for the CVE note, the over-consumption of space was intended … but at the same time I agree there's not much going on there.

Lastly I don't worry too much about confusion over section content organization, or at least I should say they read OK to me either way.

@hugovk
Copy link
Member

hugovk commented Mar 13, 2024

I don't think the fact something was assigned a CVE ID is the most important thing.

We should write for our main audience - users - who want to know what's in a release, not Tidelift so they can see a difference pre/post-lifting.

However, I think we had very few (if any?) CVEs assigned pre-Tidelift, so something like a search for "cve" instantly shows the positive impact of lifting.

As a compromise, how about including the CVE link in the existing title like this?

image
.. _Added ImageFont.MAX_STRING_LENGTH:

:cve:`2023-44271`: Added ImageFont.MAX_STRING_LENGTH
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

To protect against potential DOS attacks when using arbitrary strings as text
input, Pillow will now raise a :py:exc:`ValueError` if the number of characters
passed into ImageFont methods is over a certain limit,
:py:data:`PIL.ImageFont.MAX_STRING_LENGTH`.

This threshold can be changed by setting
:py:data:`PIL.ImageFont.MAX_STRING_LENGTH`. It can be disabled by setting
``ImageFont.MAX_STRING_LENGTH = None``.

(The .. _Added ImageFont.MAX_STRING_LENGTH: is a reference so the old anchors still work, for people using permalinks to the old headers.)

@aclark4life
Copy link
Member Author

I don't think the fact something was assigned a CVE ID is the most important thing.

We should write for our main audience - users - who want to know what's in a release, not Tidelift so they can see a difference pre/post-lifting.

I'm going to raise Subjective here. Suffice it to say, improvements we make for Tidelift are by design for users.

However, I think we had very few (if any?) CVEs assigned pre-Tidelift, so something like a search for "cve" instantly shows the positive impact of lifting.

Easily answering that question is the primary goal of this PR. The first pre-Tidelift CVE fix was just retro-added in 2.3.1 (c451df3) and the earliest possible fix to include Tidelift support is in 6.2.0 or so.

As a compromise, how about including the CVE link in the existing title like this?
image

.. _Added ImageFont.MAX_STRING_LENGTH:

:cve:`2023-44271`: Added ImageFont.MAX_STRING_LENGTH
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

To protect against potential DOS attacks when using arbitrary strings as text
input, Pillow will now raise a :py:exc:`ValueError` if the number of characters
passed into ImageFont methods is over a certain limit,
:py:data:`PIL.ImageFont.MAX_STRING_LENGTH`.

This threshold can be changed by setting
:py:data:`PIL.ImageFont.MAX_STRING_LENGTH`. It can be disabled by setting
``ImageFont.MAX_STRING_LENGTH = None``.

(The .. _Added ImageFont.MAX_STRING_LENGTH: is a reference so the old anchors still work, for people using permalinks to the old headers.)

Yes! I will try this, thanks.

docs/releasenotes/9.0.0.rst Outdated Show resolved Hide resolved
docs/releasenotes/8.2.0.rst Outdated Show resolved Hide resolved
docs/releasenotes/8.2.0.rst Show resolved Hide resolved
docs/releasenotes/8.1.2.rst Outdated Show resolved Hide resolved
docs/releasenotes/8.1.2.rst Show resolved Hide resolved
docs/releasenotes/6.2.0.rst Outdated Show resolved Hide resolved
docs/releasenotes/6.2.0.rst Outdated Show resolved Hide resolved
docs/releasenotes/6.2.0.rst Outdated Show resolved Hide resolved
docs/releasenotes/2.3.2.rst Outdated Show resolved Hide resolved
docs/releasenotes/2.3.1.rst Show resolved Hide resolved
aclark4life and others added 18 commits March 15, 2024 10:01
Before back fill, clean up.

- Add suggested CVE format to template
- Move Security to the top of release notes
- Fix headings
- Update all existing CVE notes to match template
Co-authored-by: Hugo van Kemenade <[email protected]>
Co-authored-by: Hugo van Kemenade <[email protected]>
Co-authored-by: Hugo van Kemenade <[email protected]>
Co-authored-by: Hugo van Kemenade <[email protected]>
Co-authored-by: Hugo van Kemenade <[email protected]>
Co-authored-by: Ondrej Baranovič <[email protected]>
- Include CVE link in title (via @hugovk)
- Retro-add release notes for 2.3.2, 2.5.2 for CVE-2014-3589
Co-authored-by: Hugo van Kemenade <[email protected]>
Co-authored-by: Hugo van Kemenade <[email protected]>
Co-authored-by: Hugo van Kemenade <[email protected]>
aclark4life and others added 11 commits March 15, 2024 10:01
Co-authored-by: Hugo van Kemenade <[email protected]>
Co-authored-by: Hugo van Kemenade <[email protected]>
Co-authored-by: Hugo van Kemenade <[email protected]>
- Back fill release notes for 3.1.1
- Add credits to 2.3.2, 2.5.2
- Restore accidentally overwritten contents
- Update to match updated template
- Categorized previously uncategorized notes under ``Other Changes``
- TODO: Fix categorization of notes in ``Other Changes`` that belong in other categories
@aclark4life
Copy link
Member Author

Thanks again @hugovk @nulano @radarhere ! I think this is about ready to go. If folks still don't like the 10.3.0 release notes entry, I can remove. But I do want to keep track of that list of CVEs I just added in 115179e somewhere. Maybe sneak it into deprecations and removals e.g. "We removed all these vulnerabilities" 😄

@aclark4life aclark4life merged commit 913dc60 into main Mar 16, 2024
10 of 11 checks passed
@aclark4life aclark4life deleted the 7864-backfill branch March 16, 2024 10:06
Comment on lines +102 to +104
- 1995-2009: No known CVEs
- 2010-2018: :cve:`2014-1932`, :cve:`2014-3589`, :cve:`2016-0740`, :cve:`2016-3076`
- 2019-2024: :cve:`2019-16865`, :cve:`2019-19911`, :cve:`2020-10177`, :cve:`2020-15999`,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These date ranges seem arbitrary and unexplained.

Why start in 1995? CVEs began in 1999:

The CVE List was officially launched for the public in September 1999.

https://www.cve.org/About/History

Why start the second range in 2010? The first listed CVE is 2014.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1995-2009: PIL
2010-2018: Pillow
2019-2024: Pillow w/Tidelift support


Added release notes for past releases: ``2.6.0``, ``2.5.2``,
``2.3.2``, ``2.3.1``. With these additions we are able to
provide a comprehensive list of all Pillow CVE records from
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are only 23 CVEs listed here.

But there are 48 listed at #7864, do we not need those extra ones?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! I'm not sure, but I do want to track all CVEs from 1995 (dawn of CVEs) to now.

Comment on lines +16 to +22
Other Changes
=============

Relaxed precision of some tests
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Relaxed imagedraw tests to allow slight errors for x86 vs x64.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this was indeed something added to 2.6.0, but it seems insignificant on its own. We rarely mention changes to tests in release notes.

Plus there's also all the 2.6.0-rc1 changes not included, and we've not added the non-CVE fixes to the other new files, and the other non-CVE releases are skipped, so I think we could skip this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine to remove, yeah

Comment on lines +4 to +5
Other Changes
=============
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels a bit odd starting off with "Other changes"?

If we're going to apply the template to older release notes (I wouldn't worry too much about it), we should re-categorise all the entries rather than dropping them all in "Other".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, that was a placeholder for … moving them out of the "Other Changes" category into the appropriate category.

Comment on lines +4 to +5
Other Changes
=============
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

Comment on lines +22 to +23
Other changes
=============
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could use "API Additions"

Comment on lines +4 to +5
Deprecations
============
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also "Backwards Incompatible Changes" for removals

@@ -30,6 +21,12 @@ Images can now be appended to PDF files in place by passing in
Other Changes
=============

New BLP File Format
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could also be "API Changes", or "API Additions"

docs/releasenotes/10.3.0.rst Outdated Show resolved Hide resolved
docs/releasenotes/2.6.0.rst Show resolved Hide resolved
@hugovk hugovk mentioned this pull request Mar 16, 2024
Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this.

If possible, please do wait for an approving review before merge, especially on big PRs.

I was intending to do another pass, so I've made #7885 with most of the suggestions.

@aclark4life
Copy link
Member Author

If possible, please do wait for an approving review before merge, especially on big PRs.

Sorry, will do!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Backfill release notes with security fix details
4 participants