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

Security vulnerability from file-type inner dependency #11268

Open
alfaproject opened this issue Jul 23, 2022 · 5 comments
Open

Security vulnerability from file-type inner dependency #11268

alfaproject opened this issue Jul 23, 2022 · 5 comments

Comments

@alfaproject
Copy link

alfaproject commented Jul 23, 2022

This package depends on archive-type and decompress which seem abandoned and they depend on really old versions of file-type triggering our CI audit checks:

┌───────────────┬──────────────────────────────────────────────────────────────┐
│ moderate      │ file-type vulnerable to Infinite Loop via malformed MKV file │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ file-type                                                    │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ >=16.5.4                                                     │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ @the-mill/infra-serverless                                   │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ @the-mill/infra-serverless > serverless > decompress >       │
│               │ decompress-tar > file-type                                   │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://www.npmjs.com/advisories/1081704                     │
└───────────────┴──────────────────────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ moderate      │ file-type vulnerable to Infinite Loop via malformed MKV file │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ file-type                                                    │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ >=16.5.4                                                     │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ @the-mill/infra-serverless                                   │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ @the-mill/infra-serverless > serverless > decompress >       │
│               │ decompress-tarbz2 > file-type                                │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://www.npmjs.com/advisories/1081704                     │
└───────────────┴──────────────────────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ moderate      │ file-type vulnerable to Infinite Loop via malformed MKV file │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ file-type                                                    │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ >=16.5.4                                                     │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ @the-mill/infra-serverless                                   │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ @the-mill/infra-serverless > serverless > decompress >       │
│               │ decompress-targz > file-type                                 │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://www.npmjs.com/advisories/1081704                     │
└───────────────┴──────────────────────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ moderate      │ file-type vulnerable to Infinite Loop via malformed MKV file │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ file-type                                                    │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ >=16.5.4                                                     │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ @the-mill/infra-serverless                                   │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ @the-mill/infra-serverless > serverless > decompress >       │
│               │ decompress-unzip > file-type                                 │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://www.npmjs.com/advisories/1081704                     │
└───────────────┴──────────────────────────────────────────────────────────────┘
@wilgnerl
Copy link

I'm also having this security vulnerability issue. Is it safe to continue using the tool?

@pgrzesik
Copy link
Contributor

Thanks for reporting @alfaproject.

The vulnerability is marked as moderate, it's also used in very fringe use cases in the Framework and I don't see a real scenario where it could really be exploited in a meaningful way. It should be safe to still use Framework before we find an alternative for decompress which seems to be abandoned.

@sleepwithcoffee
Copy link
Contributor

hi @pgrzesik @medikoo

Can we remove decompose altogether and only use node-tar? (we already have it in our dependencies)

Sample code to modify lib/plugins/aws/invoke-local/index.js:

// Workaround for https://github.com/serverless/serverless/issues/6028
// Java zip/jar implementaion doesn't preserve file permissions in a zip file.
// https://bugs.openjdk.java.net/browse/JDK-6194856
// So we follow Java's way in java.util.zip.ZipEntry#isDirectory() to detect directory.
const artifactFiles = [];
await tar.x({
  file: artifact,
  cwd: destination,
  filter: (file) => !file.endsWith('/'),
  onentry: entry => artifactFiles.push(entry.path),
});

// Wrong permissions are used for rust artifacts in the following stages:
// 1. Artifact creation: https://github.com/softprops/serverless-rust/pull/115
// 2. Artifact extraction: seems like an issue in package
// As a workaround, we are making sure the rust executable has the correct permissions
await Promise.all(
  artifactFiles
    .filter((file) => file === 'bootstrap')
    .map((file) => fsp.chmod(path.join(destination, file), '755'))
);

and below:

layerProgress.notice(`Unzipping ${layer.Ref}`);
await tar.x({ file: targetLayer.package.artifact, cwd: layerArtifactContentPath })
return layerArtifactContentPath;

Please also note that decompress is also being used by @serverless/utils and @serverless/dashboard-plugin so updates to these packges also required

@medikoo
Copy link
Contributor

medikoo commented Mar 14, 2023

@sleepwithcoffee Case with decompress is that it supports not only tar. It's fine to get rid of it assuming we will support exact same set of archivers

@sleepwithcoffee
Copy link
Contributor

@medikoo you mentioned a valid point. It's a shame that we dont have any better alternative to decompose at this point to handle multiple types of compression.

I am also hesitant to edit this code since it is not covered by any tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants