Repository: lirantal/lockfile-lint (js)

Issue:
Verify that the resolved URL references fetches the exact package it's nested under in the lockfile
<!--
Thank you for suggesting an idea to make this project better!

Please fill in as much of the template below as you're able.
-->

**Is your feature request related to a problem? Please describe.**

Yes. I realized that it's not very hard for an attacker to publish a package to a trusted repository, and therefore simply verifying that all packages are pulled from a trusted repository does not drastically increase the difficulty of performing an attack via lockfile modification. Instead of modifying the lockfile to resolve a dependency to a Github repo, they could simply modify it to point to a malicious package that they've published on a trusted repository.

Example of attack:
```
ms@^2.1.1:
  version "2.1.2"
  resolved "https://registry.yarnpkg.com/malicious-package/-/malicious-package-2.1.2.tgz#d09d1f357b443f493382a8eb3ccd183872ae6009"
  integrity sha512-sGkPx+VjMtmA6MX27oA4FBFELFCZZ4S4XqeGOXCv68tT+jb3vk/RyaKWP0PTKyWtmLSM0b+adUTEvbs1PEaH2w==
```

**Describe the solution you'd like**

An additional check that would increase the difficulty significantly of an attack would be to check that the package being pulled from the trusted repository has the same name as that specified in the lockfile. That way, the attacker would be forced to compromise the legitimate package itself.

The solution I'm proposing is to add an additional verifier that checks that the package name in the lockfile key matches the package name in the resolved URL.

**Describe alternatives you've considered**
N/A


Pull request description:
feat(cli arg): validate package names
<!--- Provide a general summary of your changes in the Title above -->

## Description

Instead of modifying the lockfile to resolve a dependency to a Github repo, they could simply modify it to point to a malicious package that they've published on a trusted repository.

Example of attack:
```
ms@^2.1.1:
  version "2.1.2"
  resolved "https://registry.yarnpkg.com/malicious-package/-/malicious-package-2.1.2.tgz#d09d1f357b443f493382a8eb3ccd183872ae6009"
  integrity sha512-sGkPx+VjMtmA6MX27oA4FBFELFCZZ4S4XqeGOXCv68tT+jb3vk/RyaKWP0PTKyWtmLSM0b+adUTEvbs1PEaH2w==
```

<!--- Describe your changes in detail -->

## Types of changes

<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->

- [ ] Bug fix (non-breaking change which fixes an issue)
- [x] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)

## Related Issue

<!--- This project only accepts pull requests related to open issues -->
<!--- If suggesting a new feature or change, please discuss it in an issue first -->
<!--- If fixing a bug, there should be an issue describing it with steps to reproduce -->
<!--- Please link to the issue here: -->

Fixes #113 

Sorry I didn't wait for discussion before implementing this 😅 

## Motivation and Context

<!--- Why is this change required? What problem does it solve? -->

It makes it much harder for an attacker to point a legitimate dependency to a repository they control (even if on an official repository).

## How Has This Been Tested?

<!--- Please describe in detail how you tested your changes. -->
<!--- Include details of your testing environment, and the tests you ran to -->
<!--- see how your change affects other areas of the code, etc. -->
<!--- Unit tests are expected for all changes. -->

I've extended the existing tests to cover the new functionality. 

## Screenshots (if appropriate):

## Checklist:

<!--- Go over all the following points, and put an `x` in all the boxes that apply. -->
<!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! -->

- [ ] I have updated the documentation (if required).
- [x] I have read the **CONTRIBUTING** document.
- [x] I have added tests to cover my changes.
- [x] All new and existing tests passed.
- [ ] I added a picture of a cute animal cause it's fun


Patch:
diff --git a/packages/lockfile-lint-api/index.js b/packages/lockfile-lint-api/index.js
index 300d4e5..b6e8b97 100644
--- a/packages/lockfile-lint-api/index.js
+++ b/packages/lockfile-lint-api/index.js
@@ -2,6 +2,7 @@
 
 const ValidateHost = require('./src/validators/ValidateHost')
 const ValidateHttps = require('./src/validators/ValidateHttps')
+const ValidatePackageNames = require('./src/validators/ValidatePackageNames')
 const ValidateScheme = require('./src/validators/ValidateScheme')
 const ValidateUrl = require('./src/validators/ValidateUrl')
 const ParseLockfile = require('./src/ParseLockfile')
@@ -10,6 +11,7 @@ module.exports = {
   ParseLockfile,
   ValidateHost,
   ValidateHttps,
+  ValidatePackageNames,
   ValidateScheme,
   ValidateUrl
 }
diff --git a/packages/lockfile-lint-api/src/validators/ValidatePackageNames.js b/packages/lockfile-lint-api/src/validators/ValidatePackageNames.js
new file mode 100644
index 0000000..3cf347b
--- /dev/null
+++ b/packages/lockfile-lint-api/src/validators/ValidatePackageNames.js
@@ -0,0 +1,56 @@
+'use strict'
+
+const {URL} = require('url')
+
+module.exports = class ValidatePackageNames {
+  constructor ({packages} = {}) {
+    if (typeof packages !== 'object') {
+      throw new Error('expecting an object passed to validator constructor')
+    }
+
+    this.packages = packages
+  }
+
+  validate () {
+    let validationResult = {
+      type: 'success',
+      errors: []
+    }
+
+    for (const [packageName, packageMetadata] of Object.entries(this.packages)) {
+      if (!('resolved' in packageMetadata)) {
+        continue
+      }
+
+      try {
+        const packageResolvedURL = new URL(packageMetadata.resolved)
+        const path = packageResolvedURL.pathname
+        const packageNameFromResolved = path.split('/-/')[0].slice(1)
+
+        // Remove versioning info from packageName. The @ sign is the delimiter, but could also be the
+        // first character of a scoped package name. We handle this edge-case here.
+        const nameOnly = packageName.startsWith('@')
+          ? `@${packageName.slice(1).split('@')[0]}`
+          : packageName.split('@')[0]
+
+        const expectedURLBeginning = `${packageResolvedURL.origin}/${nameOnly}`
+
+        const isPassing = packageMetadata.resolved.startsWith(expectedURLBeginning)
+        if (!isPassing) {
+          validationResult.errors.push({
+            message: `detected resolved URL for package with a different name: ${nameOnly}\n    expected: ${nameOnly}\n    actual: ${packageNameFromResolved}\n`,
+            package: nameOnly
+          })
+        }
+      } catch (error) {
+        // swallow error (assume that the version is correct)
+      }
+    }
+
+    if (validationResult.errors.length !== 0) {
+      validationResult.type = 'error'
+    }
+
+    return validationResult
+  }
+}
diff --git a/packages/lockfile-lint/bin/lockfile-lint.js b/packages/lockfile-lint/bin/lockfile-lint.js
index be75848..a5c4fd5 100755
--- a/packages/lockfile-lint/bin/lockfile-lint.js
+++ b/packages/lockfile-lint/bin/lockfile-lint.js
@@ -40,6 +40,7 @@ let validators = []
 const supportedValidators = new Map([
   ['allowed-hosts', 'validateHosts'],
   ['validate-https', 'validateHttps'],
+  ['validate-package-names', 'ValidatePackageNames'],
   ['allowed-schemes', 'validateSchemes'],
   ['allowed-urls', 'validateUrls']
 ])
diff --git a/packages/lockfile-lint/src/config.js b/packages/lockfile-lint/src/config.js
index fe619d4..2dfefde 100644
--- a/packages/lockfile-lint/src/config.js
+++ b/packages/lockfile-lint/src/config.js
@@ -45,6 +45,13 @@ module.exports = (argv, exitProcess = false, searchFrom = process.cwd()) => {
         type: 'boolean',
         describe: 'validates the use of HTTPS as protocol schema for all resources'
       },
+      n: {
+        alias: ['validate-package-names'],
+        type: 'boolean',
+        describe:
+          "validates that the resource URL specifies the same package name as that listed as the lockfile entry's key",
+        implies: 'allowed-hosts'
+      },
       e: {
         alias: 'empty-hostname',
         type: 'boolean',
diff --git a/packages/lockfile-lint/src/main.js b/packages/lockfile-lint/src/main.js
index cb09276..d1957c8 100644
--- a/packages/lockfile-lint/src/main.js
+++ b/packages/lockfile-lint/src/main.js
@@ -4,6 +4,7 @@ const debug = require('debug')('lockfile-lint')
 const {
   ValidateHostManager,
   ValidateHttpsManager,
+  ValidatePackageNamesManager,
   ValidateSchemeManager,
   ValidateUrlManager
 } = require('../src/validators')
@@ -11,6 +12,7 @@ const {
 const validatorFunctions = new Map([
   ['validateHosts', ValidateHostManager],
   ['validateHttps', ValidateHttpsManager],
+  ['ValidatePackageNames', ValidatePackageNamesManager],
   ['validateSchemes', ValidateSchemeManager],
   ['validateUrls', ValidateUrlManager]
 ])
diff --git a/packages/lockfile-lint/src/validators/index.js b/packages/lockfile-lint/src/validators/index.js
index 9e53ced..e69bb29 100644
--- a/packages/lockfile-lint/src/validators/index.js
+++ b/packages/lockfile-lint/src/validators/index.js
@@ -4,6 +4,7 @@ const {
   ValidateHost,
   ParseLockfile,
   ValidateHttps,
+  ValidatePackageNames,
   ValidateScheme,
   ValidateUrl
 } = require('lockfile-lint-api')
@@ -12,6 +13,7 @@ const debug = require('debug')('lockfile-lint')
 module.exports = {
   ValidateHostManager,
   ValidateHttpsManager,
+  ValidatePackageNamesManager,
   ValidateSchemeManager,
   ValidateUrlManager
 }
@@ -78,6 +80,25 @@ function ValidateHttpsManager ({path, type, validatorValues, validatorOptions})
   return validator.validate()
 }
 
+function ValidatePackageNamesManager ({path, type, validatorValues, validatorOptions}) {
+  debug(
+    `validate-package-names-manager invoked with validator options: ${JSON.stringify(
+      validatorValues
+    )}`
+  )
+
+  const options = {
+    lockfilePath: path,
+    lockfileType: type
+  }
+
+  const parser = new ParseLockfile(options)
+  const lockfile = parser.parseSync()
+  const validator = new ValidatePackageNames({packages: lockfile.object})
+
+  return validator.validate()
+}
+
 function ValidateUrlManager ({path, type, validatorValues, validatorOptions}) {
   debug(`validate-url-manager invoked with validator options: ${JSON.stringify(validatorValues)}`)
 


Test patch:
diff --git a/packages/lockfile-lint-api/__tests__/validators.packageNames.test.js b/packages/lockfile-lint-api/__tests__/validators.packageNames.test.js
new file mode 100644
index 0000000..5aa3d97
--- /dev/null
+++ b/packages/lockfile-lint-api/__tests__/validators.packageNames.test.js
@@ -0,0 +1,81 @@
+const ValidatePackageNames = require('../src/validators/ValidatePackageNames')
+
+describe('Validator: PackageName', () => {
+  it('validator should throw an error when provided a string', () => {
+    expect(() => new ValidatePackageNames('ss')).toThrowError()
+  })
+
+  it('validator should throw an error when provided null', () => {
+    expect(() => new ValidatePackageNames(null)).toThrowError()
+  })
+
+  it('validator should throw an error when provided array', () => {
+    expect(() => new ValidatePackageNames(['a'])).toThrowError()
+  })
+
+  it('validator should fail if a resolved URL for a different package is found', () => {
+    const failedPackage = 'meow'
+    const maliciousPackage = 'malicious'
+    const mockedPackages = {
+      '@babel/code-frame@^0.16.0': {
+        resolved: 'https://registry.npmjs.org/@babel/code-frame/-/code-frame-7.0.0.tgz'
+      },
+      'meow@1.0.0': {
+        resolved: `https://registry.npmjs.org/${maliciousPackage}/-/meow-4.0.1.tgz`
+      },
+      '@babel/generator@3.2.1': {
+        resolved: 'https://registry.npmjs.org/@babel/generator/-/generator-7.4.4.tgz'
+      }
+    }
+
+    const validator = new ValidatePackageNames({packages: mockedPackages})
+    const expectedValidationResult = {
+      type: 'error',
+      errors: [
+        {
+          message: `detected resolved URL for package with a different name: ${failedPackage}\n    expected: ${failedPackage}\n    actual: ${maliciousPackage}\n`,
+          package: failedPackage
+        }
+      ]
+    }
+    expect(validator.validate()).toEqual(expectedValidationResult)
+  })
+
+  it('validator should succeed if all resource URLs are for correct packages', () => {
+    const mockedPackages = {
+      '@babel/code-frame@^0.16.0': {
+        resolved: 'https://registry.npmjs.org/@babel/code-frame/-/code-frame-7.0.0.tgz'
+      },
+      'meow@1.0.0': {
+        resolved: 'https://registry.npmjs.org/meow/-/meow-4.0.1.tgz'
+      },
+      '@babel/generator@3.2.1': {
+        resolved: 'https://registry.npmjs.org/@babel/generator/-/generator-7.4.4.tgz'
+      }
+    }
+
+    const validator = new ValidatePackageNames({packages: mockedPackages})
+    expect(validator.validate()).toEqual({
+      type: 'success',
+      errors: []
+    })
+  })
+
+  it('validator should succeed if package has no `resolved` field', () => {
+    const mockedPackages = {
+      '@babel/code-frame@^0.16.0': {
+        resolved: 'https://registry.npmjs.org/@babel/code-frame/-/code-frame-7.0.0.tgz'
+      },
+      meow: {},
+      '@babel/generator@3.2.1': {
+        resolved: 'https://registry.npmjs.org/@babel/generator/-/generator-7.4.4.tgz'
+      }
+    }
+
+    const validator = new ValidatePackageNames({packages: mockedPackages})
+    expect(validator.validate()).toEqual({
+      type: 'success',
+      errors: []
+    })
+  })
+})
diff --git a/packages/lockfile-lint/__tests__/cli.test.js b/packages/lockfile-lint/__tests__/cli.test.js
index 8286a87..9975604 100644
--- a/packages/lockfile-lint/__tests__/cli.test.js
+++ b/packages/lockfile-lint/__tests__/cli.test.js
@@ -80,6 +80,30 @@ describe('CLI tests', () => {
     })
   })
 
+  test('Linting a file that has incorrect package name in resolved url should return exit code 1', done => {
+    const process = childProcess.spawn('node', [
+      cliExecPath,
+      '--type',
+      'yarn',
+      '--path',
+      '__tests__/fixtures/yarn-incorrect-package-name.lock',
+      '--validate-package-names',
+      '--allowed-hosts',
+      'yarn'
+    ])
+
+    let output = ''
+    process.stderr.on('data', chunk => {
+      output += chunk
+    })
+
+    process.on('close', exitCode => {
+      expect(output.indexOf('detected resolved URL for package with a different name')).not.toBe(-1)
+      expect(exitCode).toBe(1)
+      done()
+    })
+  })
+
   test('Providing conflicting arguments should display an error', done => {
     const process = childProcess.spawn(cliExecPath, [
       '--type',
diff --git a/packages/lockfile-lint/__tests__/config.test.js b/packages/lockfile-lint/__tests__/config.test.js
index f906af2..28e5cb5 100644
--- a/packages/lockfile-lint/__tests__/config.test.js
+++ b/packages/lockfile-lint/__tests__/config.test.js
@@ -81,15 +81,20 @@ describe('config', () => {
         'yarn.lock',
         '--type',
         'npm',
+        '--allowed-hosts',
+        'yarn',
         '--validate-https',
         'true',
         '--empty-hostname',
-        'false'
+        'false',
+        '--validate-package-names',
+        'true'
       ],
       expected: {
         path: 'yarn.lock',
         type: 'npm',
         validateHttps: true,
+        validatePackageNames: true,
         emptyHostname: false
       }
     }
diff --git a/packages/lockfile-lint/__tests__/fixtures/yarn-incorrect-package-name.lock b/packages/lockfile-lint/__tests__/fixtures/yarn-incorrect-package-name.lock
new file mode 100644
index 0000000..ae23874
--- /dev/null
+++ b/packages/lockfile-lint/__tests__/fixtures/yarn-incorrect-package-name.lock
@@ -0,0 +1,15 @@
+# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.
+# yarn lockfile v1
+
+
+debug@^4.1.1:
+  version "4.1.1"
+  resolved "https://registry.yarnpkg.com/debug/-/debug-4.1.1.tgz#3b72260255109c6b589cee050f1d516139664791"
+  integrity sha512-pYAIzeRo8J6KPEaJ0VWOh5Pzkbw/RetuzehGM7QRRX5he4fPHx2rdKMB256ehJCkX+XRQm16eZLqLNS8RSZXZw==
+  dependencies:
+    ms "^2.1.1"
+
+ms@^2.1.1:
+  version "2.1.2"
+  resolved "https://registry.yarnpkg.com/malicious/-/ms-2.1.2.tgz#d09d1f357b443f493382a8eb3ccd183872ae6009"
+  integrity sha512-sGkPx+VjMtmA6MX27oA4FBFELFCZZ4S4XqeGOXCv68tT+jb3vk/RyaKWP0PTKyWtmLSM0b+adUTEvbs1PEaH2w==