CVE-2019-10773: Improper Link Resolution Before File Access ('Link Following') (CWE-59)
value: In Yarn before 1.21.1, the package install functionality can be abused to generate arbitrary symlinks on the host filesystem by using specially crafted "bin" keys. Existing files could be overwritten depending on the current user permission set.
CWE description: The product attempts to access a file based on the filename, but it does not properly prevent that filename from identifying a link or shortcut that resolves to an unintended resource.
CVSS v3 base score: 7.8
Published: 2019-12-16T20:15Z
Repository: https://github.com/yarnpkg/yarn
Language: Other
Files changed: __tests__/__snapshots__/normalize-manifest.js.snap, __tests__/fixtures/normalize-manifest/dangerous bin name/actual.json	, __tests__/fixtures/normalize-manifest/dangerous bin name/expected.json	, __tests__/fixtures/normalize-manifest/dangerous bin values/actual.json	, __tests__/fixtures/normalize-manifest/dangerous bin values/expected.json	, __tests__/fixtures/normalize-manifest/empty bin string/expected.json	, src/reporters/lang/en.js, src/util/normalize-manifest/fix.js, src/util/normalize-manifest/util.js
Commit date: 2019-12-11 09:39:33 +0100
Commit message: Fixes bin overwrites (#7755)

* Fixes potential file overwrite at install time

* Fixes the regexp

* Adds warnings

* Fixes overzealous removals

* Fixes test

* Adds tests

Diff: diff --git a/__tests__/__snapshots__/normalize-manifest.js.snap b/__tests__/__snapshots__/normalize-manifest.js.snap
index c5a78707..eb903b98 100644
--- a/__tests__/__snapshots__/normalize-manifest.js.snap
+++ b/__tests__/__snapshots__/normalize-manifest.js.snap
@@ -58,10 +58,28 @@ exports[`contributors array expand: contributors array expand 1`] = `
 Array [
   "No license field",
 ]
 `;
 
+exports[`dangerous bin name: dangerous bin name 1`] = `
+Array [
+  "foo: Invalid bin entry for \\"/tmp/foo\\" (in \\"foo\\").",
+  "foo: Invalid bin entry for \\"../tmp/foo\\" (in \\"foo\\").",
+  "foo: Invalid bin entry for \\"tmp/../../foo\\" (in \\"foo\\").",
+  "foo: No license field",
+]
+`;
+
+exports[`dangerous bin values: dangerous bin values 1`] = `
+Array [
+  "foo: Invalid bin entry for \\"foo\\" (in \\"foo\\").",
+  "foo: Invalid bin entry for \\"bar\\" (in \\"foo\\").",
+  "foo: Invalid bin entry for \\"baz\\" (in \\"foo\\").",
+  "foo: No license field",
+]
+`;
+
 exports[`dedupe all trivial dependencies: dedupe all trivial dependencies 1`] = `
 Array [
   "No license field",
 ]
 `;
@@ -90,10 +108,11 @@ Array [
 ]
 `;
 
 exports[`empty bin string: empty bin string 1`] = `
 Array [
+  "foo: Invalid bin field for \\"foo\\".",
   "foo: No license field",
 ]
 `;
 
 exports[`engines array: engines array 1`] = `
diff --git a/__tests__/fixtures/normalize-manifest/dangerous bin name/actual.json b/__tests__/fixtures/normalize-manifest/dangerous bin name/actual.json
new file mode 100644
index 00000000..d70ea69e
--- /dev/null
+++ b/__tests__/fixtures/normalize-manifest/dangerous bin name/actual.json	
@@ -0,0 +1,9 @@
+{
+    "name": "foo",
+    "version": "",
+    "bin": {
+        "/tmp/foo": "main.js",
+        "../tmp/foo": "main.js",
+        "tmp/../../foo": "main.js"
+    }
+}
diff --git a/__tests__/fixtures/normalize-manifest/dangerous bin name/expected.json b/__tests__/fixtures/normalize-manifest/dangerous bin name/expected.json
new file mode 100644
index 00000000..80ce110c
--- /dev/null
+++ b/__tests__/fixtures/normalize-manifest/dangerous bin name/expected.json	
@@ -0,0 +1,5 @@
+{
+    "name": "foo",
+    "version": "",
+    "bin": {}
+}
diff --git a/__tests__/fixtures/normalize-manifest/dangerous bin values/actual.json b/__tests__/fixtures/normalize-manifest/dangerous bin values/actual.json
new file mode 100644
index 00000000..a8e335ab
--- /dev/null
+++ b/__tests__/fixtures/normalize-manifest/dangerous bin values/actual.json	
@@ -0,0 +1,9 @@
+{
+    "name": "foo",
+    "version": "",
+    "bin": {
+        "foo": "../../../../../foo",
+        "bar": "/hello/world",
+        "baz": "./foo/bar/../../../../../../foo"
+    }
+}
diff --git a/__tests__/fixtures/normalize-manifest/dangerous bin values/expected.json b/__tests__/fixtures/normalize-manifest/dangerous bin values/expected.json
new file mode 100644
index 00000000..80ce110c
--- /dev/null
+++ b/__tests__/fixtures/normalize-manifest/dangerous bin values/expected.json	
@@ -0,0 +1,5 @@
+{
+    "name": "foo",
+    "version": "",
+    "bin": {}
+}
diff --git a/__tests__/fixtures/normalize-manifest/empty bin string/expected.json b/__tests__/fixtures/normalize-manifest/empty bin string/expected.json
index 7c894d5f..15a2224e 100644
--- a/__tests__/fixtures/normalize-manifest/empty bin string/expected.json	
+++ b/__tests__/fixtures/normalize-manifest/empty bin string/expected.json	
@@ -1,5 +1,4 @@
 {
   "name": "foo",
-  "version": "",
-  "bin": ""
+  "version": ""
 }
diff --git a/src/reporters/lang/en.js b/src/reporters/lang/en.js
index 100c1692..5175666d 100644
--- a/src/reporters/lang/en.js
+++ b/src/reporters/lang/en.js
@@ -24,11 +24,12 @@ const messages = {
   internalServerErrorRetrying: 'There appears to be trouble with the npm registry (returned $1). Retrying...',
   clearedCache: 'Cleared cache.',
   couldntClearPackageFromCache: "Couldn't clear package $0 from cache",
   clearedPackageFromCache: 'Cleared package $0 from cache',
   packWroteTarball: 'Wrote tarball to $0.',
-
+  invalidBinField: 'Invalid bin field for $0.',
+  invalidBinEntry: 'Invalid bin entry for $1 (in $0).',
   helpExamples: '  Examples:\n$0\n',
   helpCommands: '  Commands:\n$0\n',
   helpCommandsMore: '  Run `$0` for more information on specific commands.',
   helpLearnMore: '  Visit $0 to learn more about Yarn.\n',
 
diff --git a/src/util/normalize-manifest/fix.js b/src/util/normalize-manifest/fix.js
index 95d19262..7d9796de 100644
--- a/src/util/normalize-manifest/fix.js
+++ b/src/util/normalize-manifest/fix.js
@@ -1,19 +1,21 @@
 /* @flow */
 
 import {MANIFEST_FIELDS} from '../../constants';
 import type {Reporter} from '../../reporters/index.js';
-import {isValidLicense} from './util.js';
+import {isValidBin, isValidLicense} from './util.js';
 import {normalizePerson, extractDescription} from './util.js';
 import {hostedGitFragmentToGitUrl} from '../../resolvers/index.js';
 import inferLicense from './infer-license.js';
 import * as fs from '../fs.js';
 
 const semver = require('semver');
 const path = require('path');
 const url = require('url');
 
+const VALID_BIN_KEYS = /^[a-z0-9_-]+$/i;
+
 const LICENSE_RENAMES: {[key: string]: ?string} = {
   'MIT/X11': 'MIT',
   X11: 'MIT',
 };
 
@@ -157,10 +159,28 @@ export default (async function(
     // Remove scoped package name for consistency with NPM's bin field fixing behaviour
     const name = info.name.replace(/^@[^\/]+\//, '');
     info.bin = {[name]: info.bin};
   }
 
+  // Validate that the bin entries reference only files within their package, and that
+  // their name is a valid file name
+  if (typeof info.bin === 'object' && info.bin !== null) {
+    const bin: Object = info.bin;
+    for (const key of Object.keys(bin)) {
+      const target = bin[key];
+      if (!VALID_BIN_KEYS.test(key) || !isValidBin(target)) {
+        delete bin[key];
+        warn(reporter.lang('invalidBinEntry', info.name, key));
+      } else {
+        bin[key] = path.normalize(target);
+      }
+    }
+  } else if (typeof info.bin !== 'undefined') {
+    delete info.bin;
+    warn(reporter.lang('invalidBinField', info.name));
+  }
+
   // bundleDependencies is an alias for bundledDependencies
   if (info.bundledDependencies) {
     info.bundleDependencies = info.bundledDependencies;
     delete info.bundledDependencies;
   }
diff --git a/src/util/normalize-manifest/util.js b/src/util/normalize-manifest/util.js
index 73f90488..7d30f610 100644
--- a/src/util/normalize-manifest/util.js
+++ b/src/util/normalize-manifest/util.js
@@ -1,15 +1,22 @@
 /* @flow */
 
 import type {PersonObject} from '../../types.js';
 
+const path = require('path');
 const validateLicense = require('validate-npm-package-license');
 
+const PARENT_PATH = /^\.\.([\\\/]|$)/;
+
 export function isValidLicense(license: string): boolean {
   return !!license && validateLicense(license).validForNewPackages;
 }
 
+export function isValidBin(bin: string): boolean {
+  return !path.isAbsolute(bin) && !PARENT_PATH.test(path.normalize(bin));
+}
+
 export function stringifyPerson(person: mixed): any {
   if (!person || typeof person !== 'object') {
     return person;
   }
 

Vulnerable code:   "version": "",
  "bin": ""

import {isValidLicense} from './util.js';
Fixed code: exports[`dangerous bin name: dangerous bin name 1`] = `
Array [
  "foo: Invalid bin entry for \\"/tmp/foo\\" (in \\"foo\\").",
  "foo: Invalid bin entry for \\"../tmp/foo\\" (in \\"foo\\").",
  "foo: Invalid bin entry for \\"tmp/../../foo\\" (in \\"foo\\").",
  "foo: No license field",
]
`;

exports[`dangerous bin values: dangerous bin values 1`] = `
Array [
  "foo: Invalid bin entry for \\"foo\\" (in \\"foo\\").",
  "foo: Invalid bin entry for \\"bar\\" (in \\"foo\\").",
  "foo: Invalid bin entry for \\"baz\\" (in \\"foo\\").",
  "foo: No license field",
]
`;

  "foo: Invalid bin field for \\"foo\\".",
{
    "name": "foo",
    "version": "",
    "bin": {
        "/tmp/foo": "main.js",
        "../tmp/foo": "main.js",
        "tmp/../../foo": "main.js"
    }
}
{
    "name": "foo",
    "version": "",
    "bin": {}
}
{
    "name": "foo",
    "version": "",
    "bin": {
        "foo": "../../../../../foo",
        "bar": "/hello/world",
        "baz": "./foo/bar/../../../../../../foo"
    }
}
{
    "name": "foo",
    "version": "",
    "bin": {}
}
  "version": ""
  invalidBinField: 'Invalid bin field for $0.',
  invalidBinEntry: 'Invalid bin entry for $1 (in $0).',
import {isValidBin, isValidLicense} from './util.js';
const VALID_BIN_KEYS = /^[a-z0-9_-]+$/i;

  // Validate that the bin entries reference only files within their package, and that
  // their name is a valid file name
  if (typeof info.bin === 'object' && info.bin !== null) {
    const bin: Object = info.bin;
    for (const key of Object.keys(bin)) {
      const target = bin[key];
      if (!VALID_BIN_KEYS.test(key) || !isValidBin(target)) {
        delete bin[key];
        warn(reporter.lang('invalidBinEntry', info.name, key));
      } else {
        bin[key] = path.normalize(target);
      }
    }
  } else if (typeof info.bin !== 'undefined') {
    delete info.bin;
    warn(reporter.lang('invalidBinField', info.name));
  }

const path = require('path');
const PARENT_PATH = /^\.\.([\\\/]|$)/;

export function isValidBin(bin: string): boolean {
  return !path.isAbsolute(bin) && !PARENT_PATH.test(path.normalize(bin));
}