1195977 - Make ThrowRangeError() and ThrowTypeError() type-safe

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Andrew McCreight (out of office until 8/21) [:mccr8]]

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.

Comment 1

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Attachment: 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.

Comment 2

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Attachment: part 2 - Statically assert that DOM error messages don't have too many arguments.

Comment 3

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Attachment: part 3 - Add helper for getting the number of arguments.

Comment 4

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Attachment: part 4 - Enforce number of arguments more consistently.

Comment 5

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Attachment: 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

Comment 6

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Attachment: part 6 - Inline throwerror with message into the header.

Comment 7

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Attachment: part 7 - Use variadic templates instead of var args.

Comment 8

[Andrew McCreight (out of office until 8/21) [:mccr8]]

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.

Comment 9

[Andrew McCreight (out of office until 8/21) [:mccr8]]

I'm narrowing the scope of this a bit and I filed some followup bugs.

Comment 10

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Attachment: 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.

Comment 11

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Attachment: 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.

Comment 12

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Attachment: part 3 - Add helper for getting the number of error arguments.

This helper is useful later.

Comment 13

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Attachment: 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.

Comment 14

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Attachment: 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.

Comment 15

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Attachment: part 6 - Inline ErrorResult throw error methods into the header.

This is needed to turn these into templates later.

Comment 16

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Attachment: 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.

Comment 17

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Attachment: 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.

Comment 18

[Peter Van der Beken [:peterv]]

Inlining the error throwing code makes me a little sad, but don't see a good way around it :-(.

Comment 19

[Andrew McCreight (out of office until 8/21) [:mccr8]]

(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.

Comment 20

[Pulsebot]

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

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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