Property edition in object inspector is vulnerable to JS injection

NEW
Unassigned

Status

()

Firefox
Developer Tools: Object Inspector
2 years ago
2 years ago

People

(Reporter: Oriol, Unassigned)

Tracking

unspecified
Points:
---

Firefox Tracking Flags

(firefox51 affected)

Details

(Reporter)

Description

2 years ago
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.
(Reporter)

Updated

2 years ago
Version: 51 Branch → unspecified
(Reporter)

Comment 1

2 years ago
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.
You need to log in before you can comment on or make changes to this bug.