diff --git a/.changeset/dirty-pears-allow.md b/.changeset/dirty-pears-allow.md new file mode 100644 index 0000000000..4b488ad79d --- /dev/null +++ b/.changeset/dirty-pears-allow.md @@ -0,0 +1,5 @@ +--- +'@backstage/backend-common': patch +--- + +Added a second validation to the `dir()` method of ZIP archive responses returned from `readTree()` that ensures that extracted files do not fall outside the target directory. diff --git a/packages/backend-common/src/reading/__fixtures__/mallory.zip b/packages/backend-common/src/reading/__fixtures__/mallory.zip new file mode 100644 index 0000000000..e45bd798b3 Binary files /dev/null and b/packages/backend-common/src/reading/__fixtures__/mallory.zip differ diff --git a/packages/backend-common/src/reading/tree/ZipArchiveResponse.test.ts b/packages/backend-common/src/reading/tree/ZipArchiveResponse.test.ts index f1915cf99b..427c237802 100644 --- a/packages/backend-common/src/reading/tree/ZipArchiveResponse.test.ts +++ b/packages/backend-common/src/reading/tree/ZipArchiveResponse.test.ts @@ -28,6 +28,9 @@ const archiveDataCorrupted = fs.readFileSync( const archiveDataWithExtraDir = fs.readFileSync( resolvePath(__filename, '../../__fixtures__/mock-with-extra-root-dir.zip'), ); +const archiveWithMaliciousEntry = fs.readFileSync( + resolvePath(__filename, '../../__fixtures__/mallory.zip'), +); describe('ZipArchiveResponse', () => { beforeEach(() => { @@ -35,6 +38,7 @@ describe('ZipArchiveResponse', () => { '/test-archive.zip': archiveData, '/test-archive-with-extra-root-dir.zip': archiveDataWithExtraDir, '/test-archive-corrupted.zip': archiveDataCorrupted, + '/test-archive-malicious.zip': archiveWithMaliciousEntry, '/tmp': mockFs.directory(), }); }); @@ -167,4 +171,22 @@ describe('ZipArchiveResponse', () => { 'invalid comment length. expected: 55. found: 0', ); }); + + it('should throw on entries with a path outside the destination dir', async () => { + const stream = fs.createReadStream('/test-archive-malicious.zip'); + + const res = new ZipArchiveResponse(stream, '', '/tmp', 'etag'); + await expect(res.files()).rejects.toThrow( + 'invalid relative path: ../side.txt', + ); + }); + + it('should throw on entries that attempt to write outside destination dir', async () => { + const stream = fs.createReadStream('/test-archive-malicious.zip'); + + const res = new ZipArchiveResponse(stream, '', '/tmp', 'etag'); + await expect(res.dir()).rejects.toThrow( + 'invalid relative path: ../side.txt', + ); + }); }); diff --git a/packages/backend-common/src/reading/tree/ZipArchiveResponse.ts b/packages/backend-common/src/reading/tree/ZipArchiveResponse.ts index 1a6658b0c2..4100af3d57 100644 --- a/packages/backend-common/src/reading/tree/ZipArchiveResponse.ts +++ b/packages/backend-common/src/reading/tree/ZipArchiveResponse.ts @@ -25,6 +25,7 @@ import { ReadTreeResponseFile, } from '../types'; import { streamToBuffer } from './util'; +import { resolveSafeChildPath } from '../../paths'; /** * Wraps a zip archive stream into a tree response reader. @@ -187,10 +188,10 @@ export class ZipArchiveResponse implements ReadTreeResponse { const dirname = platformPath.dirname(entryPath); if (dirname) { - await fs.mkdirp(platformPath.join(dir, dirname)); + await fs.mkdirp(resolveSafeChildPath(dir, dirname)); } return new Promise(async (resolve, reject) => { - const file = fs.createWriteStream(platformPath.join(dir, entryPath)); + const file = fs.createWriteStream(resolveSafeChildPath(dir, entryPath)); file.on('finish', resolve); content.on('error', reject);