fix(security): add guard against Zip Slip vulnerability (CWE-22) (#18)

Signed-off-by: dankeboy36 <dankeboy36@gmail.com>

.github/workflows/build.yml

@@ -1,4 +1,8 @@
 name: Build
+permissions:
+  contents: read
+  pull-requests: write
+
 on:
   push:
     branches:

.github/workflows/pr-title.yml

@@ -1,4 +1,7 @@
 name: PR Title
+permissions:
+  contents: read
+  pull-requests: write
 
 on:
   pull_request_target:
@@ -12,6 +15,6 @@ jobs:
   validate:
     runs-on: ubuntu-latest
     steps:
-      - uses: amannn/action-semantic-pull-request@v3.1.0
+      - uses: amannn/action-semantic-pull-request@d2ab30dcffc66150340abb5b947d518a3c3ce9cb # v3.1.0
         env:
           GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}

fake-tools/zip-slip/notice.txt

@@ -0,0 +1 @@
+https://stackoverflow.com/a/74126959

src/extract.js

@@ -11,7 +11,6 @@ const bz2 = require('unbzip2-stream')
 const unzip = require('unzip-stream')
 
 const { createLog } = require('./log')
-const { ProgressStream } = require('./progress')
 
 /**
  * @typedef {Object} ExtractParams
@@ -72,11 +71,20 @@ async function extract({ source, archiveType, counter }) {
 async function extractZip({ source, destinationPath, counter }) {
   const log = createLog('extractZip')
 
+  const invalidEntries = []
   const transformEntry = new Transform({
     objectMode: true,
     transform: async (entry, _, next) => {
       counter?.onEnter(entry.size)
       const entryPath = entry.path
+      // unzip-stream guards against `..` entry paths by converting them to `.`
+      // https://github.com/mhr3/unzip-stream/commit/d5823009634ad448873ec984bed84c18ee92f9b5#diff-fda971882fda4a106029f88d4b0a6eebeb04e7847cae8516b332b5b57e7e3370R153-R154
+      if (entryPath.split(path.sep).includes('.')) {
+        log('invalid archive entry', entryPath)
+        invalidEntries.push(entryPath)
+        next()
+        return
+      }
       const destinationFilePath = path.join(destinationPath, entryPath)
       log('extracting', destinationFilePath)
       await pipeline(
@@ -94,7 +102,9 @@ async function extractZip({ source, destinationPath, counter }) {
   })
 
   await pipeline(source, unzip.Parse(), transformEntry)
-
+  if (invalidEntries.length) {
+    throw new Error('Invalid archive entry')
+  }
   log('extracting to ', destinationPath)
 }
 
@@ -131,6 +141,7 @@ async function extractTar({
 }) {
   log('extracting to ', destinationPath)
 
+  const invalidEntries = []
   const extract = tar.extract()
 
   extract.on('entry', (header, stream, next) => {
@@ -139,14 +150,25 @@ async function extractTar({
       stream.on('end', next)
       return
     }
+
     counter?.onEnter(header.size)
-    let basename = header.name
+    let entryPath = header.name
     if (strip > 0) {
       // the path is always POSIX inside the tar. For example, "folder/fake-tool"
-      const parts = basename.split(path.posix.sep).slice(strip)
-      basename = parts.length ? parts.join(path.sep) : basename
+      const parts = entryPath.split(path.posix.sep).slice(strip)
+      entryPath = parts.length ? parts.join(path.sep) : entryPath
     }
-    const destinationFilePath = path.join(destinationPath, basename)
+
+    const destinationFilePath = path.join(destinationPath, entryPath)
+    const resolvedPath = path.resolve(destinationFilePath)
+    if (!resolvedPath.startsWith(path.resolve(destinationPath))) {
+      log('invalid archive entry', entryPath)
+      invalidEntries.push(entryPath)
+      stream.resume()
+      stream.on('end', next)
+      return
+    }
+
     fs.mkdir(path.dirname(destinationFilePath), { recursive: true })
       .then(() => {
         log('extracting', destinationFilePath)
@@ -166,6 +188,9 @@ async function extractTar({
   })
 
   await pipeline(source, decompress, extract)
+  if (invalidEntries.length) {
+    throw new Error('Invalid archive entry')
+  }
   log('extracted to', destinationPath)
 }
 

src/get.fake-tools.test.js

@@ -16,6 +16,8 @@ const {
 jest.mock('./download')
 jest.mock('./tools')
 
+const itIsNotWin32 = process.platform !== 'win32' ? it : it.skip
+
 describe('get', () => {
   let tempDirPath
   let cleanup
@@ -96,6 +98,56 @@ describe('get', () => {
     expect(fs.access(toolPath, fs.constants.X_OK)).resolves.toBeUndefined()
   })
 
+  describe('zip-slip', () => {
+    itIsNotWin32('should error (zip)', async () => {
+      jest
+        .mocked(download)
+        .mockResolvedValue(loadFakeToolByName('zip-slip/evil.zip'))
+      jest.mocked(createToolBasename).mockReturnValue('evil.sh')
+      jest.mocked(getArchiveType).mockReturnValue('zip')
+
+      await expect(
+        getTool({
+          tool: '',
+          version: '',
+          destinationFolderPath: tempDirPath,
+        })
+      ).rejects.toThrow(/invalid archive entry/gi)
+    })
+
+    itIsNotWin32('should error (tar.gz)', async () => {
+      jest
+        .mocked(download)
+        .mockResolvedValue(loadFakeToolByName('zip-slip/evil.tar.gz'))
+      jest.mocked(createToolBasename).mockReturnValue('evil.sh')
+      jest.mocked(getArchiveType).mockReturnValue('gzip')
+
+      await expect(
+        getTool({
+          tool: '',
+          version: '',
+          destinationFolderPath: tempDirPath,
+        })
+      ).rejects.toThrow(/invalid archive entry/gi)
+    })
+
+    itIsNotWin32('should error (tar.bz2)', async () => {
+      jest
+        .mocked(download)
+        .mockResolvedValue(loadFakeToolByName('zip-slip/evil.tar.bz2'))
+      jest.mocked(createToolBasename).mockReturnValue('evil.sh')
+      jest.mocked(getArchiveType).mockReturnValue('bzip2')
+
+      await expect(
+        getTool({
+          tool: '',
+          version: '',
+          destinationFolderPath: tempDirPath,
+        })
+      ).rejects.toThrow(/invalid archive entry/gi)
+    })
+  })
+
   describe('with fake server', () => {
     let server
 

src/progress.js

@@ -1,5 +1,4 @@
 const { EventEmitter } = require('node:events')
-const { Transform } = require('node:stream')
 
 const { createLog } = require('./log')
 
@@ -56,7 +55,8 @@ class ProgressCounter extends EventEmitter {
     let extractedPercentage = 0
     if (this.toExtractBytes) {
       extractedPercentage = Math.trunc(
-        (this.extractedBytes / this.toExtractBytes) * 50
+        (this.extractedBytes / this.toExtractBytes) *
+          (this.toDownloadBytes ? 50 : 100)
       )
     }
 

src/progress.test.js

@@ -0,0 +1,28 @@
+const { ProgressCounter } = require('./progress')
+
+describe('progress', () => {
+  describe('ProgressCounter', () => {
+    it('handles when toDownloadBytes is 0', () => {
+      const counter = new ProgressCounter(0)
+      const onProgress = jest.fn()
+      counter.on('progress', onProgress)
+
+      counter.onDownload(10)
+      counter.onDownload(10)
+
+      expect(onProgress).not.toHaveBeenCalled()
+
+      counter.onEnter(100)
+
+      counter.onExtract(25)
+      counter.onExtract(25)
+      counter.onExtract(25)
+      counter.onExtract(25)
+
+      expect(onProgress).toHaveBeenNthCalledWith(1, { current: 25 })
+      expect(onProgress).toHaveBeenNthCalledWith(2, { current: 50 })
+      expect(onProgress).toHaveBeenNthCalledWith(3, { current: 75 })
+      expect(onProgress).toHaveBeenNthCalledWith(4, { current: 100 })
+    })
+  })
+})