Repository: nodeca/js-yaml
Pull request #480 (2019-04-05 15:54:43)

Fix possible code execution in (already unsafe) load()
Object with executable toString() property when used as a map key will execute that function.

This happens only for `load()`, which should not be used with untrusted data anyway. `safeLoad()` is not affected because it can't parse functions.

Example:

```yaml
$ cat test.yaml
{ toString: !<tag:yaml.org,2002:js/function> 'function (){return Date.now()}' } : 1

$ ./bin/js-yaml.js ./test.yaml 
{
  "1553107949161": 1
}
```

After this PR it would return `[object Object]` as if toString wasn't there.

Dates are not affected, and if you for whatever reason are using custom types as keys (which is **not** supported very well), you can change internal class with `Symbol.toStringTag` bypassing added validation check.


Patch:
diff --git a/lib/js-yaml/loader.js b/lib/js-yaml/loader.js
index 433ee21..2815c95 100644
--- a/lib/js-yaml/loader.js
+++ b/lib/js-yaml/loader.js
@@ -30,6 +30,8 @@ var PATTERN_TAG_HANDLE            = /^(?:!|!!|![a-z\-]+!)$/i;
 var PATTERN_TAG_URI               = /^(?:!|[^,\[\]\{\}])(?:%[0-9a-f]{2}|[0-9a-z\-#;\/\?:@&=\+\$,_\.!~\*'\(\)\[\]])*$/i;
 
 
+function _class(obj) { return Object.prototype.toString.call(obj); }
+
 function is_EOL(c) {
   return (c === 0x0A/* LF */) || (c === 0x0D/* CR */);
 }
@@ -287,16 +289,29 @@ function storeMappingPair(state, _result, overridableKeys, keyTag, keyNode, valu
 
   // The output is a plain object here, so keys can only be strings.
   // We need to convert keyNode to a string, but doing so can hang the process
-  // (deeply nested arrays that explode exponentially using aliases) or execute
-  // code via toString.
+  // (deeply nested arrays that explode exponentially using aliases).
   if (Array.isArray(keyNode)) {
+    keyNode = Array.prototype.slice.call(keyNode);
+
     for (index = 0, quantity = keyNode.length; index < quantity; index += 1) {
       if (Array.isArray(keyNode[index])) {
         throwError(state, 'nested arrays are not supported inside keys');
       }
+
+      if (typeof keyNode === 'object' && _class(keyNode[index]) === '[object Object]') {
+        keyNode[index] = '[object Object]';
+      }
     }
   }
 
+  // Avoid code execution in load() via toString property
+  // (still use its own toString for arrays, timestamps,
+  // and whatever user schema extensions happen to have @@toStringTag)
+  if (typeof keyNode === 'object' && _class(keyNode) === '[object Object]') {
+    keyNode = '[object Object]';
+  }
+
+
   keyNode = String(keyNode);
 
   if (_result === null) {


Test patch:
diff --git a/test/issues/0480-date.yml b/test/issues/0480-date.yml
new file mode 100644
index 0000000..3fcac6e
--- /dev/null
+++ b/test/issues/0480-date.yml
@@ -0,0 +1,1 @@
+{ !<tag:yaml.org,2002:timestamp> '2019-04-05T12:00:43.467Z': 123 }
diff --git a/test/issues/0480-fn-array.yml b/test/issues/0480-fn-array.yml
new file mode 100644
index 0000000..2e151bf
--- /dev/null
+++ b/test/issues/0480-fn-array.yml
@@ -0,0 +1,4 @@
+? [
+    123,
+    { toString: !<tag:yaml.org,2002:js/function> 'function (){throw new Error("code execution")}' }
+] : key
diff --git a/test/issues/0480-fn.yml b/test/issues/0480-fn.yml
new file mode 100644
index 0000000..68412be
--- /dev/null
+++ b/test/issues/0480-fn.yml
@@ -0,0 +1,1 @@
+{ toString: !<tag:yaml.org,2002:js/function> 'function (){throw new Error("code execution")}' } : key
diff --git a/test/issues/0480-fn2.yml b/test/issues/0480-fn2.yml
new file mode 100644
index 0000000..6efd250
--- /dev/null
+++ b/test/issues/0480-fn2.yml
@@ -0,0 +1,1 @@
+{ __proto__: { toString: !<tag:yaml.org,2002:js/function> 'function(){throw new Error("code execution")}' } } : key
diff --git a/test/issues/0480.js b/test/issues/0480.js
new file mode 100644
index 0000000..bd9f418
--- /dev/null
+++ b/test/issues/0480.js
@@ -0,0 +1,34 @@
+'use strict';
+
+
+var assert = require('assert');
+var yaml   = require('../../');
+var readFileSync = require('fs').readFileSync;
+
+
+test('Should not execute code when object with toString property is used as a key', function () {
+  var data = yaml.load(readFileSync(require('path').join(__dirname, '/0480-fn.yml'), 'utf8'));
+
+  assert.deepEqual(data, { '[object Object]': 'key' });
+});
+
+test('Should not execute code when object with __proto__ property is used as a key', function () {
+  var data = yaml.load(readFileSync(require('path').join(__dirname, '/0480-fn2.yml'), 'utf8'));
+
+  assert.deepEqual(data, { '[object Object]': 'key' });
+});
+
+test('Should not execute code when object inside array is used as a key', function () {
+  var data = yaml.load(readFileSync(require('path').join(__dirname, '/0480-fn-array.yml'), 'utf8'));
+
+  assert.deepEqual(data, { '123,[object Object]': 'key' });
+});
+
+// this test does not guarantee in any way proper handling of date objects,
+// it just keeps old behavior whenever possible
+test('Should leave non-plain objects as is', function () {
+  var data = yaml.load(readFileSync(require('path').join(__dirname, '/0480-date.yml'), 'utf8'));
+
+  assert.deepEqual(Object.keys(data).length, 1);
+  assert(/2019/.test(Object.keys(data)[0]));
+});