1296981 - Property edition in object inspector is vulnerable to JS injection

Status: RESOLVED INVALID

(DevTools :: Storage Inspector, defect, P3)

Description

[Oriol Brufau [:Oriol]]

Somebody thought it was enough to wrap a string between quotes in order to escape it:

    "\"" + aItem._nameString + "\"";

Then that value is used in eval calls. That's horrendous.

 1. Inspect `({"a\nb": 0})`
 2. Click the the property value `0` to edit it
 3. Write `1` and press enter key
 4. Result: the value does not change because of
      SyntaxError: unterminated string literal
 5. Click the property name `ab` to rename it
 6. Write `foo` and press enter key
 7. Result: the property is not renamed because of
      SyntaxError: unterminated string literal
 8. Inspect `({"ab": 0})`
 9. Click the the property value `0` to edit it
10. Write `1, console.log('this should not run')`
11. Result: the value changes to `1` and `this should not run` is logged to the console
12. Click the property name `ab` to rename it
13. Write `", console.log('this should not run'), "foo` and press enter key
14. Result: the property is renamed to `foo` and `this should not run` is logged to the console

So wrong. The solution seems using `JSON.stringify`. I hope it has been implemented to only return strings which are valid JS, otherwise we will have to escape U+2028 and U+2029 manually.

I am already replacing the wrapping quotes with `JSON.stringify` as part of bug 996691, but we will need to ensure it's fixed and add tests.

Comment 1

[Oriol Brufau [:Oriol]]

Basically, there are 3 issues.

 - When renaming a property, the old name can inject JS. This is being fixed in bug 996691.

 - When renaming a property, the new name can inject JS. The fix is simple, in jsterm.js

      let newSymbolicName =
            variableObject.ownerView.symbolicName + '["' + newName + '"]';

   should be

      let newSymbolicName =
            variableObject.ownerView.symbolicName + '[' + escapeString(newName) + ']';

 - When changing a property value, the new one can inject JS.

   In this case I'm not sure what exactly we should allow and what not.

   For example, we could only allow JSON expressions. Then, we should validate that
   JSON.parse does not throw any error. But probably this is too restrictive.

   So I think we should allow more general expressions. To validate them I would check that
   the following does not throw

        Function("return " + code + ";");
        Function("(" + code + ")");

   And then, in simpleValueEvalMacro, wrap the code inside parentheses:

        return aPrefix + aItem.symbolicName + "=(" + aCurrentString + ")";

   I think that's enough, with Function("(" + code + ")") we ensure that the expression is
   valid wrapped inside parentheses, and with Function("return " + code + ";") we ensure
   that the expression isn't using a closing parenthesis to escape the wrapping parentheses.

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

Moving to the DOM panel as it's VariableView related and only the DOM panel uses the VariableView now

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

erratum: this should have been moved to the storage inspector, which is the last consumer of the variable view

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

The test case from comment #0 is not valid for the Storage panel. Also, the VariablesView should be refactored on top of React, so this won't be fixed.

Honza