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.

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

php: Fix CVE-2018-17082 (nixos-unstable) #50511

Closed
wants to merge 1 commit into from
Closed

php: Fix CVE-2018-17082 (nixos-unstable) #50511

wants to merge 1 commit into from

Conversation

Ekleog
Copy link
Member

@Ekleog Ekleog commented Nov 17, 2018 •
edited

php: 7.2.10 -> 7.2.12, 7.1.22 -> 7.1.24

Also make Darwin align itself to Linux versions, due to CVE-2018-17082
forcing our hand on this. This means Darwin must compile without intl.

Motivation for this change

The alternative would be to backport the fix to a previous version of php. It looks pretty easy, but I know the PHP code base very little and wouldn't trust simply using this patch to fix the issue unless spending much more time on it.

cc @etu who introduced the split between darwin and linux

@GrahamcOfBorg build php71 php72
@GrahamcOfBorg test elk owncloud

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.

@Ekleog Ekleog changed the title php: 7.2.10 -> 7.2.12, 7.1.22 -> 7.1.24 php: Fix CVE-2018-17082 Nov 17, 2018
@Ekleog Ekleog mentioned this pull request Nov 17, 2018
Closed
9 tasks
@Ekleog Ekleog changed the title php: Fix CVE-2018-17082 php: Fix CVE-2018-17082 (nixos-unstable) Nov 17, 2018
@Ekleog Ekleog mentioned this pull request Nov 17, 2018
Closed
9 tasks
@Ekleog
Copy link
Member Author

Ekleog commented Nov 17, 2018

See also backports at #50505 and (almost-a-backport) #50377.

@Ekleog Ekleog mentioned this pull request Nov 17, 2018
Closed
1 task
@GrahamcOfBorg
Copy link

Success on aarch64-linux (full log)

Attempted: php

Partial log (click to expand)

/nix/store/fwsncrrpccpjd1dq4jqqkwrz1b687hb8-php-7.2.12/bin/pear: interpreter directive changed from "/bin/sh" to "/nix/store/dsyc1z7ck08ga7l0b1jcxx35wj69qcii-bash-4.4-p23/bin/sh"
/nix/store/fwsncrrpccpjd1dq4jqqkwrz1b687hb8-php-7.2.12/bin/php-config: interpreter directive changed from " /bin/sh" to "/nix/store/dsyc1z7ck08ga7l0b1jcxx35wj69qcii-bash-4.4-p23/bin/sh"
/nix/store/fwsncrrpccpjd1dq4jqqkwrz1b687hb8-php-7.2.12/bin/phpize: interpreter directive changed from "/bin/sh" to "/nix/store/dsyc1z7ck08ga7l0b1jcxx35wj69qcii-bash-4.4-p23/bin/sh"
checking for references to /build in /nix/store/fwsncrrpccpjd1dq4jqqkwrz1b687hb8-php-7.2.12...
moving /nix/store/fwsncrrpccpjd1dq4jqqkwrz1b687hb8-php-7.2.12/sbin/* to /nix/store/fwsncrrpccpjd1dq4jqqkwrz1b687hb8-php-7.2.12/bin
shrinking RPATHs of ELF executables and libraries in /nix/store/83zhn2cjrglp1s6dsq00da8mz2qa5xj5-php-7.2.12-dev
strip is /nix/store/p9akxn2sfy4wkhqdqa3li97pc6jaz3r1-binutils-2.30/bin/strip
patching script interpreter paths in /nix/store/83zhn2cjrglp1s6dsq00da8mz2qa5xj5-php-7.2.12-dev
checking for references to /build in /nix/store/83zhn2cjrglp1s6dsq00da8mz2qa5xj5-php-7.2.12-dev...
/nix/store/fwsncrrpccpjd1dq4jqqkwrz1b687hb8-php-7.2.12

@GrahamcOfBorg
Copy link

Success on x86_64-linux (full log)

Attempted: php

Partial log (click to expand)

/nix/store/z38071vwz4kvhqiyv3qf89nqkjz185l5-php-7.2.12/bin/php-config: interpreter directive changed from " /bin/sh" to "/nix/store/r47p5pzx52m3n34vdgqpk5rvqgm0m24m-bash-4.4-p23/bin/sh"
/nix/store/z38071vwz4kvhqiyv3qf89nqkjz185l5-php-7.2.12/bin/peardev: interpreter directive changed from "/bin/sh" to "/nix/store/r47p5pzx52m3n34vdgqpk5rvqgm0m24m-bash-4.4-p23/bin/sh"
/nix/store/z38071vwz4kvhqiyv3qf89nqkjz185l5-php-7.2.12/bin/pecl: interpreter directive changed from "/bin/sh" to "/nix/store/r47p5pzx52m3n34vdgqpk5rvqgm0m24m-bash-4.4-p23/bin/sh"
checking for references to /build in /nix/store/z38071vwz4kvhqiyv3qf89nqkjz185l5-php-7.2.12...
moving /nix/store/z38071vwz4kvhqiyv3qf89nqkjz185l5-php-7.2.12/sbin/* to /nix/store/z38071vwz4kvhqiyv3qf89nqkjz185l5-php-7.2.12/bin
shrinking RPATHs of ELF executables and libraries in /nix/store/38xykx7qnndkmfkwq0sgndmsdmh5yy19-php-7.2.12-dev
strip is /nix/store/vcc4svb8gy29g4pam2zja6llkbcwsyiq-binutils-2.30/bin/strip
patching script interpreter paths in /nix/store/38xykx7qnndkmfkwq0sgndmsdmh5yy19-php-7.2.12-dev
checking for references to /build in /nix/store/38xykx7qnndkmfkwq0sgndmsdmh5yy19-php-7.2.12-dev...
/nix/store/z38071vwz4kvhqiyv3qf89nqkjz185l5-php-7.2.12

@etu
Copy link
Contributor

etu commented Nov 17, 2018

@Ekleog I first used a non-assert solution and got it pointed out to me that that was a bad idea because then it goes unnoted that the change of feature flags have happened. Which I agree to.

That's why it ended up like this but got documented. And I've been following the issue on the PHP bugtracker to see if they actually will resolve this but it's not very hopeful. Which actually kinda surprise me since they broke the build in a minor release and don't seem able to fix it.

I'm also in favor of aligning it up again and if you and others think that this assert warning solution is good enough for this, I'm all for it.

I've also looked through the intl extension in the docs a bit https://secure.php.net/intl and, I must say. From my years (8+) of working professionally with PHP. I don't think that I've ever to my knowledge have used this extension myself. It may be used by some popular libraries, I don't know about that.

But overall I'm in favor of this.

With this fix we could close #50014.

@Ekleog
Copy link
Member Author

Ekleog commented Nov 18, 2018

@etu Hmm, it's possible to add the assert and require that intl is manually set to false… would that do the job? It may be a harsh bump for stable-channel users, but it's likely better than leaving it go unnoticed indeed? Also, Darwin users are more or less pushed towards nixpkgs-unstable anyway IIRC, so it may be less big a deal than it'd have been had it been on all linuxes.

@etu
Copy link
Contributor

etu commented Nov 18, 2018

@Ekleog As the person who pointed me out to me said: "Better to fail on rebuild than the user upgrading and go home for the weekend and see it fail while it's weekend."

@etu
Copy link
Contributor

etu commented Nov 21, 2018

@Ekleog Status on this?

@Ekleog Ekleog force-pushed the php-cve-2018-17082-unstable branch 2 times, most recently from 2833966 to 344d88d Compare November 25, 2018 13:43
@Ekleog
php: 7.2.10 -> 7.2.12, 7.1.22 -> 7.1.24
d0bdf4d 
Also make Darwin align itself to Linux versions, due to CVE-2018-17082
forcing our hand on this. This means Darwin must compile without intl.
@Ekleog
Copy link
Member Author

Ekleog commented Nov 25, 2018

@etu Sorry for the delay, I was away from the Internet last week! I think unstable should go without the manual change, because the default will need to change before 19.03 anyway, and unstable users should be aware of this kind of things.

So I've pushed two new versions of this PR, for unstable with the change of default value and an error if intl is forced to true, and for 18.09 with no change of the default value and an error if intl is not forced to false.

This will likely break tests for 18.09, unfortunately… but I don't really see a better way forward, as silent breakage or leaving a security vulnerability are two important issues… I guess we'll have to fix tests once we notice they break, though I'm not sure how often tests are run for Darwin on 18.09.

TL;DR: I think this is good to go, and we'll have to handle the broken stuff when it becomes apparent.

Ekleog added a commit to Ekleog/nixpkgs that referenced this pull request Nov 25, 2018
@Ekleog
php: 7.2.10 -> 7.2.12, 7.1.22 -> 7.1.24
c36ade1 
Also make Darwin align itself to Linux versions, due to CVE-2018-17082
forcing our hand on this. This means Darwin must compile without intl.

Based on commit 283396658a69f5a4ed3d832bc849c53fa8e05ce1, with the
change that here we refuse to build if the user doesn't manually disable
intl.

See [1] for the reasoning.

[1] NixOS#50511 (comment)
@GrahamcOfBorg
Copy link

Success on x86_64-linux (full log)

Attempted: php

Partial log (click to expand)

/nix/store/bm6yqn88skmdp2a7w9lrc0abawai52js-php-7.2.12/lib/build/config.guess: interpreter directive changed from " /bin/sh" to "/nix/store/n2hjbpkf4c0m48945ivxs3lmsczzw2rg-bash-4.4-p23/bin/sh"
/nix/store/bm6yqn88skmdp2a7w9lrc0abawai52js-php-7.2.12/lib/build/shtool: interpreter directive changed from "/bin/sh" to "/nix/store/n2hjbpkf4c0m48945ivxs3lmsczzw2rg-bash-4.4-p23/bin/sh"
/nix/store/bm6yqn88skmdp2a7w9lrc0abawai52js-php-7.2.12/lib/build/config.sub: interpreter directive changed from " /bin/sh" to "/nix/store/n2hjbpkf4c0m48945ivxs3lmsczzw2rg-bash-4.4-p23/bin/sh"
checking for references to /build in /nix/store/bm6yqn88skmdp2a7w9lrc0abawai52js-php-7.2.12...
moving /nix/store/bm6yqn88skmdp2a7w9lrc0abawai52js-php-7.2.12/sbin/* to /nix/store/bm6yqn88skmdp2a7w9lrc0abawai52js-php-7.2.12/bin
shrinking RPATHs of ELF executables and libraries in /nix/store/kr0bbcgkpxgiajg19kpgbx3l4yi1g1nj-php-7.2.12-dev
strip is /nix/store/rpbg8gmqxhz8g61p1plz5d2srs84pvmv-binutils-2.30/bin/strip
patching script interpreter paths in /nix/store/kr0bbcgkpxgiajg19kpgbx3l4yi1g1nj-php-7.2.12-dev
checking for references to /build in /nix/store/kr0bbcgkpxgiajg19kpgbx3l4yi1g1nj-php-7.2.12-dev...
/nix/store/bm6yqn88skmdp2a7w9lrc0abawai52js-php-7.2.12

@GrahamcOfBorg
Copy link

Success on aarch64-linux (full log)

Attempted: php

Partial log (click to expand)

/nix/store/hpw3ysi8khgffm75y3v64xyml53avjw7-php-7.2.12/bin/pear: interpreter directive changed from "/bin/sh" to "/nix/store/n1kfdl37qpzh3xn6klbym1ay6xpxvmw1-bash-4.4-p23/bin/sh"
/nix/store/hpw3ysi8khgffm75y3v64xyml53avjw7-php-7.2.12/bin/php-config: interpreter directive changed from " /bin/sh" to "/nix/store/n1kfdl37qpzh3xn6klbym1ay6xpxvmw1-bash-4.4-p23/bin/sh"
/nix/store/hpw3ysi8khgffm75y3v64xyml53avjw7-php-7.2.12/bin/phpize: interpreter directive changed from "/bin/sh" to "/nix/store/n1kfdl37qpzh3xn6klbym1ay6xpxvmw1-bash-4.4-p23/bin/sh"
checking for references to /build in /nix/store/hpw3ysi8khgffm75y3v64xyml53avjw7-php-7.2.12...
moving /nix/store/hpw3ysi8khgffm75y3v64xyml53avjw7-php-7.2.12/sbin/* to /nix/store/hpw3ysi8khgffm75y3v64xyml53avjw7-php-7.2.12/bin
shrinking RPATHs of ELF executables and libraries in /nix/store/cyi8mnwl5d52nan88dp350lkhg41sfi6-php-7.2.12-dev
strip is /nix/store/6dpnd5aniypn8124mmy8f88s4mq2zl07-binutils-2.30/bin/strip
patching script interpreter paths in /nix/store/cyi8mnwl5d52nan88dp350lkhg41sfi6-php-7.2.12-dev
checking for references to /build in /nix/store/cyi8mnwl5d52nan88dp350lkhg41sfi6-php-7.2.12-dev...
/nix/store/hpw3ysi8khgffm75y3v64xyml53avjw7-php-7.2.12

@etu
Copy link
Contributor

etu commented Nov 25, 2018

Yeah, there's those two choices. And neither choice is a good choice.

So one have to pick what's best. And it this case I agree that it's different on unstable and stable 🙂

@delroth delroth mentioned this pull request Nov 27, 2018
Merged
10 tasks
@delroth
Copy link
Contributor

delroth commented Nov 27, 2018

Counterproposal on #51091 to keep backwards compatibility -- the patches are trivial enough that imo it's worth it. I'll let you pick your favorite option :)

@Ekleog
Copy link
Member Author

Ekleog commented Nov 27, 2018

Superseded by now-merged #51091

@Ekleog Ekleog closed this Nov 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@etu etu etu approved these changes

Assignees
No one assigned
Labels
6.topic: nixos 8.has: changelog 8.has: documentation 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Ekleog @GrahamcOfBorg @etu @delroth