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

icedtea_web: 1.7.1 -> 1.8.3 (fixes CVE-2019-10185, CVE-2019-10181, CVE-2019-10182) #66422

Merged
merged 4 commits into from Aug 13, 2019
Merged

icedtea_web: 1.7.1 -> 1.8.3 (fixes CVE-2019-10185, CVE-2019-10181, CVE-2019-10182) #66422

merged 4 commits into from Aug 13, 2019

Conversation

stefano-m
Copy link
Contributor

@stefano-m stefano-m commented Aug 10, 2019

Use the new official repository on GitHub and build the new launcher written in
Rust.

Also fixes the following security vulnerabilities:

  • CVE-2019-10185: zip-slip attack during auto-extraction of a JAR file.

  • CVE-2019-10181: executable code could be injected in a JAR file without
    compromising the signature verification.

  • CVE-2019-10182: improper path sanitization from elements in JNLP
    files.

References:
AdoptOpenJDK/IcedTea-Web#327

Motivation for this change
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 nix-review --run "nix-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.

Manual testing:

Notify maintainers

cc @grahamc (NixOS security team)

Use the new official repository on GitHub and build the new launcher written in
Rust.

Also fixes the following security vulnerabilities:

- CVE-2019-10185: zip-slip attack during auto-extraction of a JAR file.

- CVE-2019-10181: executable code could be injected in a JAR file without
  compromising the signature verification.

- CVE-2019-10182: improper path sanitization from <jar/> elements in JNLP
  files.

References:
AdoptOpenJDK/IcedTea-Web#327
@stefano-m
Copy link
Contributor Author

Note that this version of IcedTea-Web comes with a launcher written in Rust. I had to add a few patches to make this work and I don't know whether this can be achieved in a better way.

@stefano-m stefano-m changed the title icedtea_web: 1.7.1 -> 1.8.3 icedtea_web: 1.7.1 -> 1.8.3 (fixes CVE-2019-10185, CVE-2019-10181, CVE-2019-10182) Aug 10, 2019
@ofborg ofborg bot requested a review from wizeman August 10, 2019 07:47
@risicle
Copy link
Contributor

risicle commented Aug 10, 2019

WFM non-nixos linux x86_64

Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

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

Can we install the desktop items in postInstall ?

there might be other things too.

Also we should update the homepage.


I'm noticing there's attributes like adoptopenjdk-jre-bin. Perhaps we shouldn't touch this icedtea_web expression and add an adoptopenjdk-icedtea-web?

@stefano-m
Copy link
Contributor Author

@worldofpeace thanks for the suggestions.

Can we install the desktop items in postInstall ?

I can have a look, but FTR the main reason for the update was to patch the CVEs

Also we should update the homepage.

Well spotted, I will fix that.

I'm noticing there's attributes like adoptopenjdk-jre-bin. Perhaps we shouldn't touch this icedtea_web expression and add an adoptopenjdk-icedtea-web?

I am happy to add the adoptopenjdk-icedtea-web attribute, but I'd alias icedtea_web to it anyway. The existing 1.7 version is affected by serious CVEs and I'd rather get rid of it in master. I am working on backporting the patches for 1.7 on the 19.03 branch only.

Hope that this makes sense.

@stefano-m
Copy link
Contributor Author

@worldofpeace
Copy link
Contributor

I see. However at the moment the package is defined in development/compilers/icedtea-web, should I rename the folder too?
In this case, I'd rather submit a new PR with the package name change.

So, should I go ahead and rename everything in this PR, or would it be better merge this PR with the CE fixes and have a new PR with the package rename?
My preference goes to the latter option, what do you think?

I'm fine going with your preference then.

The follow up should do the following:

  1. rename the folder and attribute to adoptopenjdk-icedtea-web in all-packages.nix
  2. any references to icedtea_web should be changed to the new attribute (there's only one)
  3. add the alias in aliases.nix
icedtea_web = adoptopenjdk-icedtea-web; # added YYYY-MM-DD

@stefano-m
Copy link
Contributor Author

I'm fine going with your preference then.

Cool. I will remove the alias and force-push then.

Thanks!

@worldofpeace
Copy link
Contributor

@GrahamcOfBorg build icedtea_web

@stefano-m
Copy link
Contributor Author

I guess we should remove the has: package (new) label too?

@worldofpeace
Copy link
Contributor

I guess we should remove the has: package (new) label too?

ofborg will get that

@worldofpeace
Copy link
Contributor

Built this manually on aarch64-linux since you need allowUnfree and to accept the license.

The dependency on GTK was removed in
AdoptOpenJDK/IcedTea-Web@c7aae0e

Also, remove that pesky commented-out line from `preConfigure` too!
Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

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

I've built this on NixOS

  • x86_64-linux
  • aarch64-linux

and ran all the Web Start examples.

@worldofpeace worldofpeace merged commit a41873d into NixOS:master Aug 13, 2019
@worldofpeace
Copy link
Contributor

Thanks for your well constructed PR @stefano-m, it was very easy to review.

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.

None yet

3 participants