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

Jump to bottom

ncurses: 6.1 -> 6.1-20181027 #49859

Merged
merged 1 commit into from Nov 14, 2018
Merged

ncurses: 6.1 -> 6.1-20181027 #49859

merged 1 commit into from Nov 14, 2018

Conversation

andrew-d
Copy link
Contributor

@andrew-d andrew-d commented Nov 7, 2018

Motivation for this change

This includes fixes for CVE-2018-10754 (ref #49787).

While we're changing things, also set the --with-manpage-format=normal configure flag, which prevents the configure script from looking in /usr to determine whether to compress manpages. This was already the format on NixOS (where these directories don't exist), but making this explicit makes the build more reproducible on other distros.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-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)
  • Fits CONTRIBUTING.md.

cc @wkennington (ncurses maintainer)

@andrew-d
ncurses: upgrade from 6.1 -> 6.1-20181027
f485b2e 
This includes fixes for CVE-2018-10754.

While we're changing things, also set the `--with-manpage-format=normal`
configure flag, which prevents the `configure` script from looking in
/usr to determine whether to compress manpages. This was already the
format on NixOS (where these directories don't exist), but making this
explicit makes the build more reproducible on other distros.
@andrew-d andrew-d changed the title ncurses: upgrade from 6.1 -> 6.1-20181027 ncurses: 6.1 -> 6.1-20181027 Nov 7, 2018
@andrew-d
Copy link
Contributor Author

@c0bw3b or @ckauhaus - Anything I can do to help get this merged?

@andrew-d andrew-d mentioned this pull request Nov 10, 2018
Open
@c0bw3b
Copy link
Contributor

c0bw3b commented Nov 10, 2018 •
edited

The changelog between 20180127 (release 6.1) and 20181027 is quite big. I'm not sure I would rely on the latest dev snapshot for such a system lib.
Especially keeping in mind that CVE-fixes often needs to be backported to nixpkgs-stable and nixos-release.

Instead it would be safer to fetchpatch just the fix for CVE-2018-10754 which is:
https://invisible-mirror.net/archives/ncurses/6.1/ncurses-6.1-20180414.patch.gz
and apply it on top of 6.1 along with the other patch.

👍 for the manpage flag

@andrew-d
Copy link
Contributor Author

@c0bw3b - Yeah, I tried to apply just that patch, but the ncurses patches have a Prereq: line that prevents them from being applied out-of-order, and forcing it with patchFlags = "-p1 -f"; doesn't help due to actual patch conflicts. Given that, I think there's a few options:

  1. Apply all 11 patches between 6.1 and the fixed version (see below)
  2. Given that we're already applying a bunch of patches, bump to the latest version (there are 37 total patches between 6.1 and the version in this PR).
  3. Try to backport the specific security fixes in a custom patch. I'm less confident in this approach, given the potential impact, so I'd prefer to avoid this one, but it's an option.

The 11 patches that we'd need to apply are:

ncurses-6.1-20180129.patch.gz                                                                                                                                                      
ncurses-6.1-20180203.patch.gz
ncurses-6.1-20180210.patch.gz
ncurses-6.1-20180217.patch.gz
ncurses-6.1-20180224.patch.gz
ncurses-6.1-20180303.patch.gz
ncurses-6.1-20180317.patch.gz
ncurses-6.1-20180324.patch.gz
ncurses-6.1-20180331.patch.gz
ncurses-6.1-20180407.patch.gz
ncurses-6.1-20180414.patch.gz

@dtzWill
Copy link
Member

dtzWill commented Nov 14, 2018

I personally prefer (and have been using in ALLVM) the simplicity of using ncurses devel snapshots. FWIW there's a new version (the changes are particularly minor: https://invisible-island.net/ncurses/NEWS.html#index-t20181110 ).

I think this is different than the usual "developer snapshot" in that:

  • Developer seems to take them more seriously: snapshots are official and are given proper changelogs.
  • ncurses developer isn't some random person pushing "testing CI" commits to github (although everyone knows CI never works on your first try no matter how careful you are ;))

Personally I think these are safer than many official releases for other projects, but this project is both critical for basic usability and used directly or transitively by basically everything, so maybe that's just the higher baseline professionalism expected from such a project. I certainly appreciate and approve of being conservative here, just where to draw the line seems less clear. If nothing else we can't be updating weekly or we'd never get anything done other than rebuilding last week's upgrade just in time to start building it all again :P.

@c0bw3b
Copy link
Contributor

c0bw3b commented Nov 14, 2018

Ok thanks for the experience feedback @dtzWill
Seems Debian/Ubuntu and Fedora are also using those "unstable" releases. https://security-tracker.debian.org/tracker/CVE-2018-10754

And in Nix/NixOS it has been done before in the 6.0 branch ( #28334 or fea02e3 )

@andrew-d ignore my previous comment, no good reasons to be too conservative here.

@ckauhaus
Copy link
Contributor

Ok, someone with write access going to merge this now?

@c0bw3b
Copy link
Contributor

c0bw3b commented Nov 14, 2018

@GrahamcOfBorg build ncurses

@GrahamcOfBorg
Copy link

Success on aarch64-linux (full log)

Attempted: ncurses

Partial log (click to expand)

rmdir: failed to remove '/nix/store/grryp0ysgzyspa5j4zr2ra3snb5gmkr0-ncurses-6.1-20181027-dev/bin': Directory not empty
Moving /nix/store/grryp0ysgzyspa5j4zr2ra3snb5gmkr0-ncurses-6.1-20181027-dev/bin/tput to /nix/store/kh9vradd0j88pya08raq5hdwjs2605iy-ncurses-6.1-20181027/bin/tput
rmdir: failed to remove '/nix/store/grryp0ysgzyspa5j4zr2ra3snb5gmkr0-ncurses-6.1-20181027-dev/bin': Directory not empty
Moving /nix/store/grryp0ysgzyspa5j4zr2ra3snb5gmkr0-ncurses-6.1-20181027-dev/bin/tset to /nix/store/kh9vradd0j88pya08raq5hdwjs2605iy-ncurses-6.1-20181027/bin/tset
rmdir: failed to remove '/nix/store/grryp0ysgzyspa5j4zr2ra3snb5gmkr0-ncurses-6.1-20181027-dev/bin': Directory not empty
Moving /nix/store/grryp0ysgzyspa5j4zr2ra3snb5gmkr0-ncurses-6.1-20181027-dev/bin/captoinfo to /nix/store/kh9vradd0j88pya08raq5hdwjs2605iy-ncurses-6.1-20181027/bin/captoinfo
rmdir: failed to remove '/nix/store/grryp0ysgzyspa5j4zr2ra3snb5gmkr0-ncurses-6.1-20181027-dev/bin': Directory not empty
Moving /nix/store/grryp0ysgzyspa5j4zr2ra3snb5gmkr0-ncurses-6.1-20181027-dev/bin/infotocap to /nix/store/kh9vradd0j88pya08raq5hdwjs2605iy-ncurses-6.1-20181027/bin/infotocap
rmdir: failed to remove '/nix/store/grryp0ysgzyspa5j4zr2ra3snb5gmkr0-ncurses-6.1-20181027-dev/bin': Directory not empty
/nix/store/kh9vradd0j88pya08raq5hdwjs2605iy-ncurses-6.1-20181027

@GrahamcOfBorg
Copy link

Success on x86_64-linux (full log)

Attempted: ncurses

Partial log (click to expand)

rmdir: failed to remove '/nix/store/6dcrhriljs6ipk6vv62xl38221yl3b2v-ncurses-6.1-20181027-dev/bin': Directory not empty
Moving /nix/store/6dcrhriljs6ipk6vv62xl38221yl3b2v-ncurses-6.1-20181027-dev/bin/tput to /nix/store/nj16bxng8qp3pihhkzlb65g4j8zb3di5-ncurses-6.1-20181027/bin/tput
rmdir: failed to remove '/nix/store/6dcrhriljs6ipk6vv62xl38221yl3b2v-ncurses-6.1-20181027-dev/bin': Directory not empty
Moving /nix/store/6dcrhriljs6ipk6vv62xl38221yl3b2v-ncurses-6.1-20181027-dev/bin/tset to /nix/store/nj16bxng8qp3pihhkzlb65g4j8zb3di5-ncurses-6.1-20181027/bin/tset
rmdir: failed to remove '/nix/store/6dcrhriljs6ipk6vv62xl38221yl3b2v-ncurses-6.1-20181027-dev/bin': Directory not empty
Moving /nix/store/6dcrhriljs6ipk6vv62xl38221yl3b2v-ncurses-6.1-20181027-dev/bin/captoinfo to /nix/store/nj16bxng8qp3pihhkzlb65g4j8zb3di5-ncurses-6.1-20181027/bin/captoinfo
rmdir: failed to remove '/nix/store/6dcrhriljs6ipk6vv62xl38221yl3b2v-ncurses-6.1-20181027-dev/bin': Directory not empty
Moving /nix/store/6dcrhriljs6ipk6vv62xl38221yl3b2v-ncurses-6.1-20181027-dev/bin/infotocap to /nix/store/nj16bxng8qp3pihhkzlb65g4j8zb3di5-ncurses-6.1-20181027/bin/infotocap
rmdir: failed to remove '/nix/store/6dcrhriljs6ipk6vv62xl38221yl3b2v-ncurses-6.1-20181027-dev/bin': Directory not empty
/nix/store/nj16bxng8qp3pihhkzlb65g4j8zb3di5-ncurses-6.1-20181027

@c0bw3b c0bw3b merged commit 64c4cc3 into NixOS:staging Nov 14, 2018
@andrew-d andrew-d deleted the andrew/upgrade-ncurses branch November 14, 2018 23:04
@guaraqe guaraqe mentioned this pull request Nov 28, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
1.severity: security 10.rebuild-darwin: 501+ 10.rebuild-darwin-stdenv 10.rebuild-linux: 501+
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@andrew-d @c0bw3b @dtzWill @ckauhaus @GrahamcOfBorg