Make ThrowRangeError() and ThrowTypeError() type-safe

RESOLVED FIXED in Firefox 43

Status

()

Core
DOM
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

(Blocks: 1 bug, {sec-audit, sec-want})

Trunk
mozilla43
sec-audit, sec-want
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 fixed)

Details

(Whiteboard: [adv-main43-])

Attachments

(8 attachments, 7 obsolete attachments)

1.50 KB, patch
peterv
: review+
Details | Diff | Splinter Review
2.46 KB, patch
peterv
: review+
Details | Diff | Splinter Review
1.54 KB, patch
peterv
: review+
Details | Diff | Splinter Review
2.26 KB, patch
peterv
: review+
Details | Diff | Splinter Review
3.00 KB, patch
peterv
: review+
Details | Diff | Splinter Review
6.29 KB, patch
peterv
: review+
Details | Diff | Splinter Review
4.16 KB, patch
peterv
: review+
Details | Diff | Splinter Review
1.17 KB, patch
peterv
: review+
Details | Diff | Splinter Review
(Assignee)

Description

2 years ago
ErrorResult::ThrowErrorWithMessage (and the methods that call it) use var args, which means they don't do any type checking of their arguments. As seen in bug 1194562, this can result in buffer overflows that are not detected except in ASan builds. It seems like with some template programming we should be able to statically check these arguments.

I can take a look if nobody else is champing at the bit to work on this. I'm filing this as a sec bug because it may turn up more instances of things like bug 1194562.
(Assignee)

Updated

2 years ago
Assignee: nobody → continuation
(Assignee)

Updated

2 years ago
Depends on: 1194562
(Assignee)

Updated

2 years ago
Depends on: 1196339
Depends on: 1197012
(Assignee)

Comment 1

2 years ago
Created attachment 8651295 [details] [diff] [review]
part 1 - Update comment on Errors.msg.

This is still in progress, but I figured I might as well upload what I have so far. This depends on the definition of a constant from bug 1196391.
(Assignee)

Comment 2

2 years ago
Created attachment 8651296 [details] [diff] [review]
part 2 - Statically assert that DOM error messages don't have too many arguments.
(Assignee)

Comment 3

2 years ago
Created attachment 8651297 [details] [diff] [review]
part 3 - Add helper for getting the number of arguments.
(Assignee)

Comment 4

2 years ago
Created attachment 8651298 [details] [diff] [review]
part 4 - Enforce number of arguments more consistently.
(Assignee)

Comment 5

2 years ago
Created attachment 8651299 [details] [diff] [review]
part 5 - Hoist out inner part of throw error with message.

this will enable us to move it to the header in the next part, for templatification
(Assignee)

Comment 6

2 years ago
Created attachment 8651300 [details] [diff] [review]
part 6 - Inline throwerror with message into the header.
(Assignee)

Comment 7

2 years ago
Created attachment 8651301 [details] [diff] [review]
part 7 - Use variadic templates instead of var args.
(Assignee)

Comment 8

2 years ago
Here's a summary of the changes:

1. Passing an argument of the incorrect type will now cause a compilation error, rather than some kind of weird type confusion problem.

2. Specifying an error message with too many arguments with cause a compilation error (static_assert).

3. Passing too many arguments will cause an assertion in debug builds. In release builds, the additional arguments will be ignored (which is what happens currently).

4. Not passing in enough errors will cause a fatal assertion in both and debug and opt builds. This is to prevent a stack buffer overflow when the JS engine attempts to read arguments that are not there.

5. If we deserialize an ErrorResult, and the message either has too many or too messages, we fail the deserialize (which kills the child process or whatever.). The ErrNum deserialization enforces that the result is a valid enum, and the static_assert enforces that the number of arguments for valid enums is < 10. This is to prevent a hostile child process from causing a stack buffer overflow.

6. Dynamically check that ThrowInvalidThis takes no more than 2 arguments, which is how many we pass in. Again, this prevents a stack buffer overflow if used incorrectly.
(Assignee)

Updated

2 years ago
Depends on: 1196391
(Assignee)

Updated

2 years ago
Depends on: 1197889
(Assignee)

Updated

2 years ago
Blocks: 1197889
No longer depends on: 1197889
(Assignee)

Updated

2 years ago
Blocks: 1197893
(Assignee)

Comment 9

2 years ago
I'm narrowing the scope of this a bit and I filed some followup bugs.
Summary: Statically check the arguments to ErrorResult::ThrowErrorWithMessage → Make ThrowRangeError() and ThrowTypeError() type-safe

Updated

2 years ago
Group: core-security → dom-core-security
(Assignee)

Comment 10

2 years ago
Created attachment 8656777 [details] [diff] [review]
part 1 - Add JS_EXN_TYPE to comment in Errors.msg.

There are a lot of patches here, but they are mostly pretty simple, except part 7.
Attachment #8651295 - Attachment is obsolete: true
Attachment #8651296 - Attachment is obsolete: true
Attachment #8651297 - Attachment is obsolete: true
Attachment #8651298 - Attachment is obsolete: true
Attachment #8651299 - Attachment is obsolete: true
Attachment #8651300 - Attachment is obsolete: true
Attachment #8651301 - Attachment is obsolete: true
Attachment #8656777 - Flags: review?(peterv)
(Assignee)

Comment 11

2 years ago
Created attachment 8656778 [details] [diff] [review]
part 2 - Statically assert that DOM error messages don't have more arguments than the JS engine supports.

This makes it so that we do not need to dynamically enforce this
constraint in ErrorResult::ThrowErrorWithMessage().

This uses a new JS API constant, JS::MaxNumErrorArguments, defined in bug 1196391.
Attachment #8656778 - Flags: review?(peterv)
(Assignee)

Comment 12

2 years ago
Created attachment 8656779 [details] [diff] [review]
part 3 - Add helper for getting the number of error arguments.

This helper is useful later.
Attachment #8656779 - Flags: review?(peterv)
(Assignee)

Comment 13

2 years ago
Created attachment 8656782 [details] [diff] [review]
part 4 - Enforce number of arguments more consistently.

The new check in ErrorResult::ReportErrorWithMessage() shouldn't be
needed and is just to protect against the possibility of another way
to construct messages being added.

Of particular note, the additional check in ErrorResult::DeserializeMessage() prevents a hostile child process from causing a buffer overflow in our process. We do not need to check against the max number of arguments, because it is statically enforced that all valid enums of this type have less arguments than that, and the IPC deserialize code for enums dynamically enforces that this is a valid enum.
Attachment #8656782 - Flags: review?(peterv)
(Assignee)

Comment 14

2 years ago
Created attachment 8656784 [details] [diff] [review]
part 5 - Hoist out inner part of ErrorResult::ThrowErrorWithMessage() into a helper.

This will enable us to move it to the header later, which is needed to
turn it into a template.
Attachment #8656784 - Flags: review?(peterv)
(Assignee)

Comment 15

2 years ago
Created attachment 8656785 [details] [diff] [review]
part 6 - Inline ErrorResult throw error methods into the header.

This is needed to turn these into templates later.
Attachment #8656785 - Flags: review?(peterv)
(Assignee)

Comment 16

2 years ago
Created attachment 8656786 [details] [diff] [review]
part 7 - Use variadic templates instead of var args for ThrowTypeError() and ThrowRangeError().

This enables type checking of these arguments.

The MOZ_RELEASE_ASSERT in the case with no arguments will become superfluous with bug 1197893 but I left it here for now.
Attachment #8656786 - Flags: review?(peterv)
(Assignee)

Comment 17

2 years ago
Created attachment 8656787 [details] [diff] [review]
part 8 - Check that ThrowInvalidThis is passing in enough arguments.

We have to permit passing in too many arguments because sometimes the
error is MSG_METHOD_THIS_UNWRAPPING_DENIED which only takes one
argument.

This is another error message function that should be checked.
Attachment #8656787 - Flags: review?(peterv)
Attachment #8656777 - Flags: review?(peterv) → review+
Attachment #8656778 - Flags: review?(peterv) → review+
Attachment #8656779 - Flags: review?(peterv) → review+
Attachment #8656782 - Flags: review?(peterv) → review+
Attachment #8656784 - Flags: review?(peterv) → review+
Attachment #8656785 - Flags: review?(peterv) → review+
Attachment #8656786 - Flags: review?(peterv) → review+
Attachment #8656787 - Flags: review?(peterv) → review+
Inlining the error throwing code makes me a little sad, but don't see a good way around it :-(.
(Assignee)

Comment 19

2 years ago
(In reply to Peter Van der Beken [:peterv] from comment #18)
> Inlining the error throwing code makes me a little sad, but don't see a good
> way around it :-(.

Yeah, it is unfortunate this requires templating.

The blocking bugs have all been fixed, so I think this can be unhidden now.
Group: dom-core-security

Comment 20

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/37ceff90f964
https://hg.mozilla.org/integration/mozilla-inbound/rev/505d11fcad96
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd02797d06af
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3075fbc955d
https://hg.mozilla.org/integration/mozilla-inbound/rev/fba22ced664c
https://hg.mozilla.org/integration/mozilla-inbound/rev/8fbf291b2fdc
https://hg.mozilla.org/integration/mozilla-inbound/rev/eae3e7c3dbbd
https://hg.mozilla.org/integration/mozilla-inbound/rev/461b0e3e3071
https://hg.mozilla.org/mozilla-central/rev/37ceff90f964
https://hg.mozilla.org/mozilla-central/rev/505d11fcad96
https://hg.mozilla.org/mozilla-central/rev/bd02797d06af
https://hg.mozilla.org/mozilla-central/rev/f3075fbc955d
https://hg.mozilla.org/mozilla-central/rev/fba22ced664c
https://hg.mozilla.org/mozilla-central/rev/8fbf291b2fdc
https://hg.mozilla.org/mozilla-central/rev/eae3e7c3dbbd
https://hg.mozilla.org/mozilla-central/rev/461b0e3e3071
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox43: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Whiteboard: [adv-main43-]
You need to log in before you can comment on or make changes to this bug.