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

glibc: 2.38-23 -> 2.38-27 #258972

Merged
merged 1 commit into from
Oct 4, 2023
Merged

glibc: 2.38-23 -> 2.38-27 #258972

merged 1 commit into from
Oct 4, 2023

Conversation

edef1c
Copy link
Member

@edef1c edef1c commented Oct 4, 2023

Description of changes

Fixes CVE-2023-4911.

Backport to stable: #258975

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@edef1c
Copy link
Member Author

edef1c commented Oct 4, 2023

Looks like this is somewhat of a dupe of #258856 and #258857, but those are currently held back by being a cherry-pick rather than a patchlevel update.

@edef1c edef1c mentioned this pull request Oct 4, 2023
12 tasks
@edef1c edef1c added 8.has: port to stable A PR already has a backport to the stable release. and removed 9.needs: port to stable A PR needs a backport to the stable release. labels Oct 4, 2023
@Ma27
Copy link
Member

Ma27 commented Oct 4, 2023

Thanks a lot @edef1c ! Currently starting a few test-builds and will do a review+merge when that's done. Hope I'll get everything done today.

Fixes CVE-2023-4911.

Change-Id: I154c79a7c6822051903ef15c547c3567691b1457
Copy link
Contributor

@flokli flokli left a comment

Choose a reason for hiding this comment

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

With 70cf1cf (the version before the rename) I did run some builds (nixosTests.nscd and nixosTests.avahi), which should give some exposure into nss lookups still working.

coreutils, libpng-apng-1.6.40 and gnutls were a bit flaky, but ultimately the builds succeeded!

I tried to reproduce the same 2.38-master.patch.gz as @edef1c, but for some reason mine does have a slightly different format (Author and Date, instead of Author[Date] and Commit[Date] fields), but the patch contents are the same.

I'd say let's get this in, we can have a discussion around the patch workflow later.

@flokli flokli merged commit 1e41c33 into NixOS:staging Oct 4, 2023
7 of 9 checks passed
@edef1c edef1c deleted the cve-2023-4911 branch October 4, 2023 15:51
@Ma27
Copy link
Member

Ma27 commented Oct 4, 2023

I tried to reproduce the same 2.38-master.patch.gz as @edef1c, but for some reason mine does have a slightly different format (Author and Date, instead of Author[Date] and Commit[Date] fields), but the patch contents are the same.

Same here, but I assumed differences in git versions (and this is non-functional, so 🤷 )

I'd say let's get this in, we can have a discussion around the patch workflow later.

👍

@alyssais
Copy link
Member

I tried to reproduce the same 2.38-master.patch.gz as @edef1c, but for some reason mine does have a slightly different format (Author and Date, instead of Author[Date] and Commit[Date] fields), but the patch contents are the same.

That sounds like --format=full vs --format=fuller.

Copy link
Member

Choose a reason for hiding this comment

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

Why is this a binary checked into git?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's explained in common.nix:

      /* No tarballs for stable upstream branch, only https://sourceware.org/git/glibc.git and using git would complicate bootstrapping.
          $ git fetch --all -p && git checkout origin/release/2.38/master && git describe
          glibc-2.38-27-g750a45a783
          $ git show --minimal --reverse glibc-2.38.. | gzip -9n --rsyncable - > 2.38-master.patch.gz

         To compare the archive contents zdiff can be used.
          $ zdiff -u 2.38-master.patch.gz ../nixpkgs/pkgs/development/libraries/glibc/2.38-master.patch.gz

Copy link
Member

Choose a reason for hiding this comment

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

This explains the need for the patch but not why it's gzipped?

Copy link
Contributor

Choose a reason for hiding this comment

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

Because it's already 3KB without being gzipped, and people are careful about not increasing the size of the nixpkgs repo.

Copy link
Member

Choose a reason for hiding this comment

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

Don't people always download the Nixpkgs repo compressed in some way, though? If you do a git clone, you get compressed packs. If you download a channel, you get a tar.xz. If you download from GitHub, you get a zip or a tar.gz or whatever. I'd expect this file being a .gz to make the compressed size of Nixpkgs as a whole slightly worse, because now it's opaque to the compressor and can't be deduplicated with other similar files.

Copy link
Member

Choose a reason for hiding this comment

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

We sometimes backport security fixes and tests that validate they work. In some cases the test artifacts are in fact binary (think image formats, asn1, ...), but we'd still like to have them, to ensure the patch works, and does not regress.

This is especially relevant, since fetchpatch drops binary hunks, and even then we sometimes need to put a rebased patch into the tree.

Copy link
Member

Choose a reason for hiding this comment

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

That sounds different though — the patch itself would still be a text file, right?

Copy link
Member

Choose a reason for hiding this comment

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

Those artifacts should then either be generated from text-based formats or be fetched from somewhere else.

Binaries in Git are incredibly inefficient in both size and, more importantly, performance and Nixpkgs can already be a challenge in those regards.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I'm a little late to the party 😅

I just skimmed through old PRs and it seems as if we started at around 2.31 or 2.32 with that when we needed to apply a bunch of patches for several issues and decided to just follow the release branch.
I guess it sounds reasonable to switch to a plaintext patchset especially given @alyssais' explanation on the expected influence on nixpkgs tarballs.

cc @TredwellGit @vcunat in case they have anything to add.

Copy link
Member

Choose a reason for hiding this comment

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

fwiw #287594 contains a patch rather than a gz file.

Ma27 added a commit to Ma27/nixpkgs that referenced this pull request Feb 9, 2024
Announcement: https://lists.gnu.org/archive/html/info-gnu/2024-01/msg00017.html

This release seems relatively harmless in terms of potential fallout.
Most notably is the removal of `crypt(3)` in favor of libxcrypt which
we've done already and compatibility from ISO C2X.

Also decided to drop the old *.gz approach in favor of inlining the
patch with the changes from the release branch directly: it's relatively
small in contrast to certain lockfiles in this repo and having a textual
version makes reviews & diffs easier. See also
NixOS#258972 (comment) for
more context.
@Ma27 Ma27 mentioned this pull request Feb 9, 2024
13 tasks
Ma27 added a commit to Ma27/nixpkgs that referenced this pull request Feb 11, 2024
Announcement: https://lists.gnu.org/archive/html/info-gnu/2024-01/msg00017.html

This release seems relatively harmless in terms of potential fallout.
Most notably is the removal of `crypt(3)` in favor of libxcrypt which
we've done already and compatibility from ISO C2X.

Also decided to drop the old *.gz approach in favor of inlining the
patch with the changes from the release branch directly: it's relatively
small in contrast to certain lockfiles in this repo and having a textual
version makes reviews & diffs easier. See also
NixOS#258972 (comment) for
more context.
Ma27 added a commit to Ma27/nixpkgs that referenced this pull request Mar 2, 2024
Announcement: https://lists.gnu.org/archive/html/info-gnu/2024-01/msg00017.html

This release seems relatively harmless in terms of potential fallout.
Most notably is the removal of `crypt(3)` in favor of libxcrypt which
we've done already and compatibility from ISO C2X.

Also decided to drop the old *.gz approach in favor of inlining the
patch with the changes from the release branch directly: it's relatively
small in contrast to certain lockfiles in this repo and having a textual
version makes reviews & diffs easier. See also
NixOS#258972 (comment) for
more context.
tm-drtina pushed a commit to awakesecurity/nixpkgs that referenced this pull request Apr 27, 2024
Announcement: https://lists.gnu.org/archive/html/info-gnu/2024-01/msg00017.html

This release seems relatively harmless in terms of potential fallout.
Most notably is the removal of `crypt(3)` in favor of libxcrypt which
we've done already and compatibility from ISO C2X.

Also decided to drop the old *.gz approach in favor of inlining the
patch with the changes from the release branch directly: it's relatively
small in contrast to certain lockfiles in this repo and having a textual
version makes reviews & diffs easier. See also
NixOS#258972 (comment) for
more context.
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.

9 participants