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.7.2 (plus CVE patches) #66444

Merged
merged 3 commits into from Aug 15, 2019
Merged

icedtea_web: 1.7.1 -> 1.7.2 (plus CVE patches) #66444

merged 3 commits into from Aug 15, 2019

Conversation

stefano-m
Copy link
Contributor

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

On Wed, 31 Jul 2019 it was announced that IcedTea-Web was affected by the below
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.

Version 1.7 was patched, but no release was made. Moreover, the patches apply
cleanly only to 1.7.2, not the current 1.7.1.

Rather than marking 1.7.1 as insecure, update to 1.7.2 and apply the official
patches.

Note that the patch file has been edited to remove the addition of binary
blobs (that cannot be applied).

References:

https://www.openwall.com/lists/oss-security/2019/07/31/2
AdoptOpenJDK/IcedTea-Web#327
AdoptOpenJDK/IcedTea-Web#346

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.

Same manual testing as done in #66422 but using the shell launchers instead of the rust ones.

Notify maintainers

cc @grahamc (NixOS security team)
@worldofpeace (reviewer of related #66422)

@worldofpeace
Copy link
Contributor

Note that the patch file has been edited to remove the addition of binary
blobs (that cannot be applied).

I believe this is because patchPhase in setup.sh uses patch and only git apply can do binary.
It's kinda undesirable to have really large patches like this in nixpkgs.
Perhaps we could fetch the patches and in postPatch use git apply?
You'd have to use a git repo though, fetched with fetchgit and using leaveDotGit.

Also, is it possible these binary blobs were important?

If doing that proves too difficult, maybe we can upload it somewhere else.

@stefano-m
Copy link
Contributor Author

@worldofpeace the patch comes straight from AdoptOpenJDK/IcedTea-Web#346 (i.e. https://patch-diff.githubusercontent.com/raw/AdoptOpenJDK/IcedTea-Web/pull/346.patch)

The binary blobs are

  • tests/netx/unit/net/sourceforge/jnlp/runtime/j1.jar
  • tests/netx/unit/net/sourceforge/jnlp/runtime/jar03_dotdotN1.jar

They are used in tests, the Changelog in the diff has some more information. I don't think that the nix package has ever run all tests, so I think it should be OK to not have them in the scope of this PR.

Looking forward to hearing from you.

Thanks

@worldofpeace
Copy link
Contributor

Ok,

@worldofpeace the patch comes straight from AdoptOpenJDK/IcedTea-Web#346 (i.e. https://patch-diff.githubusercontent.com/raw/AdoptOpenJDK/IcedTea-Web/pull/346.patch)

The binary blobs are

* `tests/netx/unit/net/sourceforge/jnlp/runtime/j1.jar`

* `tests/netx/unit/net/sourceforge/jnlp/runtime/jar03_dotdotN1.jar`

They are used in tests, the Changelog in the diff has some more information. I don't think that the nix package has ever run all tests, so I think it should be OK to not have them in the scope of this PR.

Looking forward to hearing from you.

Thanks

Ok, that clears up "Also, is it possible these binary blobs were important?" but I still think we shouldn't have such a large patch in nixpkgs for practical reasons.

Do you disagree @grahamc?

@stefano-m
Copy link
Contributor Author

but I still think we shouldn't have such a large patch in nixpkgs for practical reasons.

I'm not sure I understand. I have removed the blobs from the patch, so it shouldn't be too big. It should include just the code changes.

$ du -sh pkgs/development/compilers/icedtea-web/cve-fixes.patch
48K     pkgs/development/compilers/icedtea-web/cve-fixes.patch

Am I missing something?

@worldofpeace
Copy link
Contributor

but I still think we shouldn't have such a large patch in nixpkgs for practical reasons.

I'm not sure I understand. I have removed the blobs from the patch, so it shouldn't be too big. It should include just the code changes.

$ du -sh pkgs/development/compilers/icedtea-web/cve-fixes.patch
48K     pkgs/development/compilers/icedtea-web/cve-fixes.patch

Am I missing something?

Yes, we have a guideline to try our best to fetch patches from remote places and not keep them in nixpkgs. I personally support this since it keeps nixpkgs much smaller.

@stefano-m
Copy link
Contributor Author

Yes, we have a guideline to try our best to fetch patches from remote places and not keep them in nixpkgs. I personally support this since it keeps nixpkgs much smaller.

I see! Thanks for the explanation. I'm happy to rework this to include the patch remotely then.

@stefano-m
Copy link
Contributor Author

I removed the patch file and use fetchurl instead. Then use git apply in postPatch as suggested.

The package builds fine locally and the executables continue to work.

@worldofpeace
Copy link
Contributor

Can we remove gtk2 here as well?

@worldofpeace
Copy link
Contributor

worldofpeace commented Aug 13, 2019

Testing now, this should be good with the above comment.

@worldofpeace
Copy link
Contributor

There's a distinct change in executable names

Before

result/bin
├── itweb-settings
├── javaws
└── policyeditor

After

result/bin
├── itweb-settings.sh
├── itw-modularjdk.args
├── javaws.sh
└── policyeditor.sh

I'd say that's backwards incompatible.

@stefano-m
Copy link
Contributor Author

There's a distinct change in executable names

@worldofpeace are you sure about that?

I get the following:

$ tree result/bin
result/bin
├── itweb-settings.sh
├── itw-modularjdk.args
├── javaws.sh
└── policyeditor.sh

The executable names change in version 1.8.x, but should stay the same in 1.7.2

@worldofpeace
Copy link
Contributor

Building 65729e0 I get

result/bin
├── itweb-settings
├── javaws
└── policyeditor

On 7a859977bcdc3461bf95129612a7945bb452c8c2

result/bin
├── itweb-settings.sh
├── itw-modularjdk.args
├── javaws.sh
└── policyeditor.sh

Perhaps something's up with my setup, could you build those two revisions and see the diff?

@stefano-m
Copy link
Contributor Author

I see! My change definitely builds sh files. I will have a look at the difference. I guess that we could always remove the .sh extension in a post install phase.

Thanks for spotting that!

@stefano-m
Copy link
Contributor Author

I checked and the executables are all shell scripts. I will remove the extension and filter out itw-modularjdk.args. I'm not sure why the difference in outputs though.

@stefano-m
Copy link
Contributor Author

Removed sh extensions in 97a0d2c

Note that we need to keep bin/itw-modularjdk.args because it's referenced in the launchers though.

Re-tested manually and working as expected.

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.

Still works, thanks for fixing this 👍

On Wed, 31 Jul 2019 it was announced that IcedTea-Web was affected by the below
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.

Version 1.7 was patched, but no release was made. Moreover, the patches apply
cleanly only to 1.7.2, not the current 1.7.1.

Rather than marking 1.7.1 as insecure, update to 1.7.2 and apply the official
patches.

References:

https://www.openwall.com/lists/oss-security/2019/07/31/2
AdoptOpenJDK/IcedTea-Web#327
AdoptOpenJDK/IcedTea-Web#346
icedtea-web 1.7.2 builds its launchers shell scripts with the "sh" extension,
while version 1.7.1 did not.

For backwards-compatibility, remove the extension from the executable in
postInstall.

Note that version 1.7.2 also creates a file called itw-modularjdk.args in the
bin directory. This file is referenced by the shell launchers, so we leave it
there (it's not executable anyway).
@worldofpeace worldofpeace merged commit e36f91f into NixOS:release-19.03 Aug 15, 2019
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