diff --git a/package-lock.json b/package-lock.json index 1d43fbf3..7575e429 100644 --- a/package-lock.json +++ b/package-lock.json @@ -6664,9 +6664,9 @@ } }, "node_modules/ip": { - "version": "2.0.0", - "resolved": "https://registry.npmjs.org/ip/-/ip-2.0.0.tgz", - "integrity": "sha512-WKa+XuLG1A1R0UWhl2+1XQSi+fZWMsYKffMZTTYsiZaUD8k2yDAj5atimTUD2TZkyCkNEeYE5NhFZmupOGtjYQ==", + "version": "2.0.1", + "resolved": "https://registry.npmjs.org/ip/-/ip-2.0.1.tgz", + "integrity": "sha512-lJUL9imLTNi1ZfXT+DU6rBBdbiKGBuay9B6xGSPVjUeQwaH1RIGqef8RZkUtHioLmSNpPR5M4HVKJGm1j8FWVQ==", "dev": true }, "node_modules/is-array-buffer": { diff --git a/packages/artifact/RELEASES.md b/packages/artifact/RELEASES.md index 9e14a6b6..7c30fe79 100644 --- a/packages/artifact/RELEASES.md +++ b/packages/artifact/RELEASES.md @@ -113,4 +113,8 @@ ### 2.1.1 -- Updated `isGhes` check to include `.ghe.com` and `.ghe.localhost` as accepted hosts \ No newline at end of file +- Updated `isGhes` check to include `.ghe.com` and `.ghe.localhost` as accepted hosts + +### 2.1.2 + +- Updated the stream extract functionality to use `unzip.Parse()` instead of `unzip.Extract()` for greater control of unzipping artifacts diff --git a/packages/artifact/__tests__/download-artifact.test.ts b/packages/artifact/__tests__/download-artifact.test.ts index 1c0c9b0d..e961532a 100644 --- a/packages/artifact/__tests__/download-artifact.test.ts +++ b/packages/artifact/__tests__/download-artifact.test.ts @@ -121,6 +121,16 @@ const mockGetArtifactFailure = jest.fn(() => { } }) +const mockGetArtifactMalicious = jest.fn(() => { + const message = new http.IncomingMessage(new net.Socket()) + message.statusCode = 200 + message.push(fs.readFileSync(path.join(__dirname, 'fixtures', 'evil.zip'))) // evil.zip contains files that are formatted x/../../etc/hosts + message.push(null) + return { + message + } +}) + describe('download-artifact', () => { describe('public', () => { beforeEach(setup) @@ -170,6 +180,51 @@ describe('download-artifact', () => { expect(response.downloadPath).toBe(fixtures.workspaceDir) }) + it('should not allow path traversal from malicious artifacts', async () => { + const downloadArtifactMock = github.getOctokit(fixtures.token).rest + .actions.downloadArtifact as MockedDownloadArtifact + downloadArtifactMock.mockResolvedValueOnce({ + headers: { + location: fixtures.blobStorageUrl + }, + status: 302, + url: '', + data: Buffer.from('') + }) + + const mockHttpClient = (HttpClient as jest.Mock).mockImplementation( + () => { + return { + get: mockGetArtifactMalicious + } + } + ) + + await expect( + downloadArtifactPublic( + fixtures.artifactID, + fixtures.repositoryOwner, + fixtures.repositoryName, + fixtures.token + ) + ).rejects.toBeInstanceOf(Error) + + expect(downloadArtifactMock).toHaveBeenCalledWith({ + owner: fixtures.repositoryOwner, + repo: fixtures.repositoryName, + artifact_id: fixtures.artifactID, + archive_format: 'zip', + request: { + redirect: 'manual' + } + }) + + expect(mockHttpClient).toHaveBeenCalledWith(getUserAgentString()) + expect(mockGetArtifactMalicious).toHaveBeenCalledWith( + fixtures.blobStorageUrl + ) + }) + it('should successfully download an artifact to user defined path', async () => { const customPath = path.join(testDir, 'custom') diff --git a/packages/artifact/__tests__/fixtures/evil.zip b/packages/artifact/__tests__/fixtures/evil.zip new file mode 100644 index 00000000..d345590f Binary files /dev/null and b/packages/artifact/__tests__/fixtures/evil.zip differ diff --git a/packages/artifact/package-lock.json b/packages/artifact/package-lock.json index 550cbb01..d25b1524 100644 --- a/packages/artifact/package-lock.json +++ b/packages/artifact/package-lock.json @@ -1,12 +1,12 @@ { "name": "@actions/artifact", - "version": "2.1.1", + "version": "2.1.2", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@actions/artifact", - "version": "2.1.1", + "version": "2.1.2", "license": "MIT", "dependencies": { "@actions/core": "^1.10.0", diff --git a/packages/artifact/package.json b/packages/artifact/package.json index a5f6ad10..35f088e6 100644 --- a/packages/artifact/package.json +++ b/packages/artifact/package.json @@ -1,6 +1,6 @@ { "name": "@actions/artifact", - "version": "2.1.1", + "version": "2.1.2", "preview": true, "description": "Actions artifact lib", "keywords": [ diff --git a/packages/artifact/src/internal/download/download-artifact.ts b/packages/artifact/src/internal/download/download-artifact.ts index dc54f6fe..09890c29 100644 --- a/packages/artifact/src/internal/download/download-artifact.ts +++ b/packages/artifact/src/internal/download/download-artifact.ts @@ -1,4 +1,7 @@ import fs from 'fs/promises' +import * as stream from 'stream' +import {createWriteStream} from 'fs' +import * as path from 'path' import * as github from '@actions/github' import * as core from '@actions/core' import * as httpClient from '@actions/http-client' @@ -44,6 +47,11 @@ async function streamExtract(url: string, directory: string): Promise { await streamExtractExternal(url, directory) return } catch (error) { + if (error.message.includes('Malformed extraction path')) { + throw new Error( + `Artifact download failed with unretryable error: ${error.message}` + ) + } retryCount++ core.debug( `Failed to download artifact after ${retryCount} retries due to ${error.message}. Retrying in 5 seconds...` @@ -78,6 +86,8 @@ export async function streamExtractExternal( } const timer = setTimeout(timerFn, timeout) + const createdDirectories = new Set() + createdDirectories.add(directory) response.message .on('data', () => { timer.refresh() @@ -89,8 +99,51 @@ export async function streamExtractExternal( clearTimeout(timer) reject(error) }) - .pipe(unzip.Extract({path: directory})) - .on('close', () => { + .pipe(unzip.Parse()) + .pipe( + new stream.Transform({ + objectMode: true, + transform: async (entry, _, callback) => { + const fullPath = path.normalize(path.join(directory, entry.path)) + if (!directory.endsWith(path.sep)) { + directory += path.sep + } + if (!fullPath.startsWith(directory)) { + reject(new Error(`Malformed extraction path: ${fullPath}`)) + } + + core.debug(`Extracting artifact entry: ${fullPath}`) + if (entry.type === 'Directory') { + if (!createdDirectories.has(fullPath)) { + createdDirectories.add(fullPath) + await resolveOrCreateDirectory(fullPath).then(() => { + entry.autodrain() + callback() + }) + } else { + entry.autodrain() + callback() + } + } else { + if (!createdDirectories.has(path.dirname(fullPath))) { + createdDirectories.add(path.dirname(fullPath)) + await resolveOrCreateDirectory(path.dirname(fullPath)).then( + () => { + entry.autodrain() + callback() + } + ) + } + + const writeStream = createWriteStream(fullPath) + writeStream.on('finish', callback) + writeStream.on('error', reject) + entry.pipe(writeStream) + } + } + }) + ) + .on('finish', async () => { clearTimeout(timer) resolve() })