Skip to content

[20.03] curl: update CVE patch hashes for extra digits added to index line #111206

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 25, 2021
Merged

[20.03] curl: update CVE patch hashes for extra digits added to index line #111206

merged 1 commit into from
Mar 25, 2021
+200 −12

Conversation

twhitehead
Copy link
Contributor

Motivation for this change

The fetched CVE patches for curl changed so the hashes no longer agree.

Things done

I diffed the old and new versions. It seems github just added extra digits to the index lines. For example

diff -U2 /nix/store/4psjdyqqf4wb4cv8ijds35cpqnmyjd5p-CVE-2020-8169.patch /nix/store/rsx49gzgb1fgylyh8cggnd92nx0hl9gw-CVE-2020-8169.patch

--- /nix/store/4psjdyqqf4wb4cv8ijds35cpqnmyjd5p-CVE-2020-8169.patch	1969-12-31 19:00:01.000000000 -0500
+++ /nix/store/rsx49gzgb1fgylyh8cggnd92nx0hl9gw-CVE-2020-8169.patch	1969-12-31 19:00:01.000000000 -0500
@@ -17,5 +17,5 @@
 
 diff --git a/lib/url.c b/lib/url.c
-index f250f2ff20a..9b8b2bdde64 100644
+index f250f2ff20a3..9b8b2bdde640 100644
 --- a/lib/url.c
 +++ b/lib/url.c
@@ -38,5 +38,5 @@
    }
 diff --git a/tests/data/Makefile.inc b/tests/data/Makefile.inc
-index 004a90b2360..bb6bf0f2fd0 100644
+index 004a90b23608..bb6bf0f2fd06 100644
 --- a/tests/data/Makefile.inc
 +++ b/tests/data/Makefile.inc
@@ -51,5 +51,5 @@
 diff --git a/tests/data/test1168 b/tests/data/test1168
 new file mode 100644
-index 00000000000..283e91e0197
+index 000000000000..283e91e01974
 --- /dev/null
 +++ b/tests/data/test1168

so I just updated the hashes to match the new versions.

If this causes some sort of mass rebuild, perhaps a better solution would be to just add the old ones directly to nixpkgs to avoid having to changes any hashes. If you would prefer that, let me know and I will change it.

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Sorry, something went wrong.

@twhitehead
Copy link
Contributor Author

Actually, I see the newer versions of the curl file have a big comment right at the top that says

Note: this package is used for bootstrapping fetchurl, and thus cannot use fetchpatch! All mutable patches (generated by GitHub or cgit) that are needed here should be included directly in Nixpkgs as files.

so I'll update this pull request to directly include the patches instead.

@ofborg ofborg bot added the 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild label Jan 29, 2021
@ofborg ofborg bot requested a review from lovek323 January 29, 2021 21:43
@twhitehead
Copy link
Contributor Author

Ok. I've updated the pull request to actually include the patch. Unfortunately it seems this still results in a rebuild of curl as, at least, the ultimate store path for a file is different if you fetch it vs you directly reference it as a file path.

CCing @edolstra as this last bit seems a bit strange. I would have thought including a patch vs fetching it should not cause a rebuild as both would be the same fixed output. Fetching it vs referencing it ultimately gives two different paths, respectively

/nix/store/4psjdyqqf4wb4cv8ijds35cpqnmyjd5p-CVE-2020-8169.patch
/nix/store/f1wi4gqnkjxf0sfnc8k4fvgwrc4yir6g-CVE-2020-8169.patch

that are yet clearly the same file

diff -U2 \
  /nix/store/4psjdyqqf4wb4cv8ijds35cpqnmyjd5p-CVE-2020-8169.patch \
  /nix/store/f1wi4gqnkjxf0sfnc8k4fvgwrc4yir6g-CVE-2020-8169.patch \
&& echo the same

the same

doronbehar
doronbehar requested changes Mar 25, 2021
View reviewed changes
Copy link
Contributor

@doronbehar doronbehar left a comment

Choose a reason for hiding this comment

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

The commit message should change.

@twhitehead
curl: fix hash mismatch issue by directly include CVE patches
d237186 
Include instead of using fetchpatch due to boostrapping requirement.
@twhitehead
Copy link
Contributor Author

The commit message should change.

New commit message is curl: fix hash mismatch issue by directly include CVE patches.

erictapen
erictapen approved these changes Mar 25, 2021
View reviewed changes
Copy link
Member

@erictapen erictapen left a comment

Choose a reason for hiding this comment

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

Verified that the checked in patches are those behind the URLs. Thanks!

@erictapen erictapen merged commit 9ef94a1 into NixOS:staging-20.03 Mar 25, 2021
@erictapen erictapen changed the title curl: update CVE patch hashes for extra digits added to index line [20.03] curl: update CVE patch hashes for extra digits added to index line Mar 25, 2021
@erictapen
Copy link
Member

I just noticed this was for 20.03, not 20.09. I'm not sure the staging branch for 20.03 is still merged into release-20.03 regularly?

@twhitehead
Copy link
Contributor Author

Sorry. Sounds like I should have put in the request against release-20.03.

I can do a new pull request against release-20.03 if you want?

@erictapen
Copy link
Member

Nah, I just merged it into release-20.03: 1eea371

Just to make sure: You are aware that NixOS 20.03 is end of life for about five months now? So no guarantees about security fixes being backported.

@twhitehead
Copy link
Contributor Author

no guarantees about security fixes being backported.

Got it. Thanks! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@doronbehar doronbehar

@erictapen erictapen

@lovek323 lovek323

Assignees
No one assigned
Labels
10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@twhitehead @erictapen @doronbehar