Replaces ZipInputStream with ZipFile to fix Zip Slip vulnerability #7230

DarshitChanpura · 2023-04-18T14:47:06Z

More details about the vulenrability can be found on https://github.com/snyk/zip-slip-vulnerability

Description

ZipInputStream can cause a potential zipslip vulnerability. This PR aims at fixing the vulnerability by replacing it with ZipFile class.

Issues Resolved

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff
Commit changes are listed out in CHANGELOG.md file (See: Changelog)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

github-actions · 2023-04-18T14:55:36Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/14226/
CommitID: f76bc76
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2023-04-18T15:10:44Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/14227/
CommitID: fe9c318
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

dblock · 2023-04-19T14:47:50Z

distribution/tools/plugin-cli/src/main/java/org/opensearch/plugins/InstallPluginCommand.java

@@ -749,12 +752,11 @@ private Path unzip(Path zip, Path pluginsDir) throws IOException, UserException
                if (entry.isDirectory() == false) {
                    try (OutputStream out = Files.newOutputStream(targetFile)) {
                        int len;
-                        while ((len = zipInput.read(buffer)) >= 0) {
+                        while ((len = zipFile.getInputStream(entry).read(buffer)) >= 0) {


I think this reopens the stream every time we loop (or relies on the fact that zipFile.getInputStream memoizes a stream), let's do it once?

dblock

Looks good if you can please make gradle check pass + address my comment on stream reuse and changelog entry. Thanks!

dblock · 2023-04-19T14:48:28Z

CHANGELOG.md

@@ -79,6 +79,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Fix compression support for h2c protocol ([#4944](https://github.com/opensearch-project/OpenSearch/pull/4944))
 - Support OpenSSL Provider with default Netty allocator ([#5460](https://github.com/opensearch-project/OpenSearch/pull/5460))
 - Avoid negative memory result in IndicesQueryCache stats calculation ([#6917](https://github.com/opensearch-project/OpenSearch/pull/6917))
+- Replaces ZipInputStream with ZipFile to fix ZipSlip vulnerability ([#7230](https://github.com/opensearch-project/OpenSearch/pull/7230))


replace "fix ZipSlip..." by "fix CVE-..."

I don't know of a specific CVE for this vulnerability. But I do see a group of different vulnerabilities related to zip slip exploit. Refer here for more details:

https://github.com/snyk/zip-slip-vulnerability

https://cve.mitre.org/cgi-bin/cvekey.cgi?keyword=directory+traversal+crafted+zip

Ok, I don't see a CVE in the list on https://github.com/snyk/zip-slip-vulnerability either.

I think the correct spelling is with a space though, isn't it? "Zip Slip".

More details about the vulenrability can be found on https://github.com/snyk/zip-slip-vulnerability Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

github-actions · 2023-04-19T21:33:33Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/14339/
CommitID: 89c0a8a
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2023-04-19T21:40:15Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/14340/
CommitID: a25aa5d
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

github-actions · 2023-04-19T22:10:42Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/14345/
CommitID: 6a17c5e
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

github-actions · 2023-04-19T22:39:11Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/14349/
CommitID: c5721da
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

DarshitChanpura · 2023-04-19T22:59:25Z

Need to re-run Gradle Precommit. The failure seems to be flaky on the Github runner's end.

github-actions · 2023-04-19T23:10:06Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/14351/
CommitID: 679f960
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2023-04-20T14:27:54Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/14370/
CommitID: 679f960
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

dblock · 2023-04-21T17:00:19Z

distribution/tools/plugin-cli/src/main/java/org/opensearch/plugins/InstallPluginCommand.java

@@ -748,13 +751,13 @@ private Path unzip(Path zip, Path pluginsDir) throws IOException, UserException
                }
                if (entry.isDirectory() == false) {
                    try (OutputStream out = Files.newOutputStream(targetFile)) {
+                        InputStream input = zipFile.getInputStream(entry);


Does the input stream not need to be closed, including on error?

Yes, I will add input.close().

You will need to close it in a finally block, otherwise it will leak on read error.

Include it in the try-with-resources:

try (OutputStream out = Files.newOutputStream(targetFile); InputStream input = zipFile.getInputStream(entry)) { ... }

dblock · 2023-04-21T17:02:44Z

distribution/tools/plugin-cli/build.gradle

@@ -78,3 +80,28 @@ thirdPartyAudit.ignoreViolations(
  'org.bouncycastle.jcajce.provider.ProvSunTLSKDF$TLSExtendedMasterSecretGenerator',
  'org.bouncycastle.jcajce.provider.ProvSunTLSKDF$TLSExtendedMasterSecretGenerator$2'
 )
+
+thirdPartyAudit.ignoreMissingClasses(


Why is this needed?

Without these, the task ':distribution:tools:plugin-cli:thirdPartyAudit' fails. See details here.

@reta do you know if this is ok?

@reta do you know if this is ok?

Yeah, I think this is fine, the common-compress may have optional dependencies on ZSTD and other codecs, so we need to exclude these classes (we don't use these codecs here)

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

github-actions · 2023-04-24T15:42:12Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/14482/
CommitID: 413e378
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

github-actions · 2023-04-24T16:18:58Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/14483/
CommitID: 62a173e

codecov-commenter · 2023-04-24T16:22:39Z

Codecov Report

Merging #7230 (a828a70) into main (e1da84d) will increase coverage by 0.03%.
The diff coverage is 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff              @@
##               main    #7230      +/-   ##
============================================
+ Coverage     70.65%   70.69%   +0.03%     
+ Complexity    59518    59486      -32     
============================================
  Files          4859     4859              
  Lines        285527   285527              
  Branches      41150    41149       -1     
============================================
+ Hits         201749   201861     +112     
+ Misses        67216    67051     -165     
- Partials      16562    16615      +53

Impacted Files	Coverage Δ
...a/org/opensearch/plugins/InstallPluginCommand.java	`85.14% <100.00%> (ø)`

... and 501 files with indirect coverage changes

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

github-actions · 2023-04-24T18:24:50Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/14497/
CommitID: f97e548
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2023-04-24T18:38:32Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/14498/
CommitID: 435321e

DarshitChanpura · 2023-04-24T19:01:54Z

Gradle precommit failure seems to be a memory issue with GIthub runner. Re-run should probably solve it.

dblock · 2023-04-25T15:26:40Z

distribution/tools/plugin-cli/build.gradle

@@ -78,3 +80,28 @@ thirdPartyAudit.ignoreViolations(
  'org.bouncycastle.jcajce.provider.ProvSunTLSKDF$TLSExtendedMasterSecretGenerator',
  'org.bouncycastle.jcajce.provider.ProvSunTLSKDF$TLSExtendedMasterSecretGenerator$2'
 )
+
+thirdPartyAudit.ignoreMissingClasses(


@reta do you know if this is ok?

dblock · 2023-04-25T15:27:54Z

distribution/tools/plugin-cli/src/main/java/org/opensearch/plugins/InstallPluginCommand.java

            byte[] buffer = new byte[8192];
-            while ((entry = zipInput.getNextEntry()) != null) {
+            while (entries.hasMoreElements()) {
+                entry = entries.nextElement();


Does entries.nextElment() return null when there are no more entries? You could collapse the loop in that case without needing to check entries.hasMoreElements() into while (entry = entries.nextElement()) { }.

entries.nextElement() returns NoSucElementException when no more elements exist.
According to definition:

Throws: NoSuchElementException – if no more elements exist.

dblock · 2023-04-25T15:28:39Z

distribution/tools/plugin-cli/src/main/java/org/opensearch/plugins/InstallPluginCommand.java

@@ -748,13 +751,13 @@ private Path unzip(Path zip, Path pluginsDir) throws IOException, UserException
                }
                if (entry.isDirectory() == false) {
                    try (OutputStream out = Files.newOutputStream(targetFile)) {
+                        InputStream input = zipFile.getInputStream(entry);


You will need to close it in a finally block, otherwise it will leak on read error.

reta · 2023-04-26T09:46:48Z

distribution/tools/plugin-cli/src/main/java/org/opensearch/plugins/InstallPluginCommand.java

@@ -748,13 +751,14 @@ private Path unzip(Path zip, Path pluginsDir) throws IOException, UserException
                }
                if (entry.isDirectory() == false) {
                    try (OutputStream out = Files.newOutputStream(targetFile)) {
+                        InputStream input = zipFile.getInputStream(entry);


👍 to @dblock and @andrross comments, and please use zipInput.transferTo(out);

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

github-actions · 2023-04-26T20:32:43Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/14575/
CommitID: a828a70

opensearch-trigger-bot · 2023-04-28T17:26:28Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-7230-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 84227681555b4aec12725c4454c39834cb6839d2
# Push it to GitHub
git push --set-upstream origin backport/backport-7230-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-7230-to-2.x.

dblock · 2023-04-28T17:45:56Z

@DarshitChanpura want to manually backport to 2.x?

…pensearch-project#7230) * Replaces ZipInputStream with ZipFile to fix ZipSlip vulnerability More details about the vulenrability can be found on https://github.com/snyk/zip-slip-vulnerability Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Adds a CHANGELOG entry Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Changes the base package for ZipFile to avoid forbiddenApisError Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Updates SHAs Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Adds LICENSE and NOTICE files for commons-compress package Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Updates thirdParty ignoreMissingClasses list Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Closes input stream Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Fixes NoSuchFileException when reading files Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Fixes spotless errors Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Fixes CHANGELOG.md Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Addresses PR comments Signed-off-by: Darshit Chanpura <dchanp@amazon.com> --------- Signed-off-by: Darshit Chanpura <dchanp@amazon.com> (cherry picked from commit 8422768)

…7230) (#7366) * Replaces ZipInputStream with ZipFile to fix ZipSlip vulnerability More details about the vulenrability can be found on https://github.com/snyk/zip-slip-vulnerability Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Adds a CHANGELOG entry Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Changes the base package for ZipFile to avoid forbiddenApisError Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Updates SHAs Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Adds LICENSE and NOTICE files for commons-compress package Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Updates thirdParty ignoreMissingClasses list Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Closes input stream Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Fixes NoSuchFileException when reading files Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Fixes spotless errors Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Fixes CHANGELOG.md Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Addresses PR comments Signed-off-by: Darshit Chanpura <dchanp@amazon.com> --------- Signed-off-by: Darshit Chanpura <dchanp@amazon.com> (cherry picked from commit 8422768)

…pensearch-project#7230) * Replaces ZipInputStream with ZipFile to fix ZipSlip vulnerability More details about the vulenrability can be found on https://github.com/snyk/zip-slip-vulnerability Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Adds a CHANGELOG entry Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Changes the base package for ZipFile to avoid forbiddenApisError Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Updates SHAs Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Adds LICENSE and NOTICE files for commons-compress package Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Updates thirdParty ignoreMissingClasses list Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Closes input stream Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Fixes NoSuchFileException when reading files Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Fixes spotless errors Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Fixes CHANGELOG.md Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Addresses PR comments Signed-off-by: Darshit Chanpura <dchanp@amazon.com> --------- Signed-off-by: Darshit Chanpura <dchanp@amazon.com> Signed-off-by: Shivansh Arora <hishiv@amazon.com>

dblock reviewed Apr 19, 2023

View reviewed changes

dblock requested changes Apr 19, 2023

View reviewed changes

DarshitChanpura added 3 commits April 19, 2023 17:29

Replaces ZipInputStream with ZipFile to fix ZipSlip vulnerability

58a90bf

More details about the vulenrability can be found on https://github.com/snyk/zip-slip-vulnerability Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Adds a CHANGELOG entry

55bd7b9

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Changes the base package for ZipFile to avoid forbiddenApisError

a25aa5d

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

DarshitChanpura force-pushed the zipstreams-bug-fix branch from 89c0a8a to a25aa5d Compare April 19, 2023 21:31

Updates SHAs

6a17c5e

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Adds LICENSE and NOTICE files for commons-compress package

c5721da

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Updates thirdParty ignoreMissingClasses list

679f960

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

dblock reviewed Apr 21, 2023

View reviewed changes

DarshitChanpura added 2 commits April 24, 2023 11:00

Closes input stream

fa4307a

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Fixes NoSuchFileException when reading files

413e378

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Fixes spotless errors

62a173e

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

DarshitChanpura marked this pull request as ready for review April 24, 2023 18:02

DarshitChanpura requested review from reta, anasalkouz and andrross as code owners April 24, 2023 18:02

DarshitChanpura requested review from owaiskazi19, Rishikesh1159, ryanbogan, saratvemulapalli, shwetathareja, dreamer-89, tlfeng, VachaShah and xuezhou25 as code owners April 24, 2023 18:02

DarshitChanpura changed the title ~~Replaces ZipInputStream with ZipFile to fix ZipSlip vulnerability~~ Replaces ZipInputStream with ZipFile to fix zip slip vulnerability Apr 24, 2023

DarshitChanpura changed the title ~~Replaces ZipInputStream with ZipFile to fix zip slip vulnerability~~ Replaces ZipInputStream with ZipFile to fix Zip Slip vulnerability Apr 24, 2023

Fixes CHANGELOG.md

435321e

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

DarshitChanpura force-pushed the zipstreams-bug-fix branch from f97e548 to 435321e Compare April 24, 2023 18:06

dblock reviewed Apr 25, 2023

View reviewed changes

reta reviewed Apr 26, 2023

View reviewed changes

Addresses PR comments

a828a70

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

reta approved these changes Apr 27, 2023

View reviewed changes

dblock approved these changes Apr 28, 2023

View reviewed changes

dblock merged commit 8422768 into opensearch-project:main Apr 28, 2023

dblock added the backport 2.x Backport to 2.x branch label Apr 28, 2023

DarshitChanpura mentioned this pull request May 2, 2023

[Backport 2.x] Replaces ZipInputStream with ZipFile to fix Zip Slip vulnerability (#7230) #7366

Merged

6 tasks

Replaces ZipInputStream with ZipFile to fix Zip Slip vulnerability #7230

Replaces ZipInputStream with ZipFile to fix Zip Slip vulnerability #7230

Conversation

DarshitChanpura commented Apr 18, 2023

Description

Issues Resolved

Check List

github-actions bot commented Apr 18, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Apr 18, 2023

Gradle Check (Jenkins) Run Completed with:

Choose a reason for hiding this comment

dblock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 19, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Apr 19, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Apr 19, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Apr 19, 2023

Gradle Check (Jenkins) Run Completed with:

DarshitChanpura commented Apr 19, 2023

github-actions bot commented Apr 19, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Apr 20, 2023

Gradle Check (Jenkins) Run Completed with:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 24, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Apr 24, 2023

Gradle Check (Jenkins) Run Completed with:

codecov-commenter commented Apr 24, 2023 • edited Loading

Codecov Report

github-actions bot commented Apr 24, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Apr 24, 2023

Gradle Check (Jenkins) Run Completed with:

DarshitChanpura commented Apr 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DarshitChanpura Apr 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reta Apr 26, 2023 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Apr 26, 2023

Gradle Check (Jenkins) Run Completed with:

opensearch-trigger-bot bot commented Apr 28, 2023

dblock commented Apr 28, 2023

codecov-commenter commented Apr 24, 2023 •

edited

Loading

DarshitChanpura Apr 26, 2023 •

edited

Loading

reta Apr 26, 2023 •

edited

Loading