Closed Bug 1431439 Opened 2 years ago Closed 2 years ago

Land JS fuzzing target for StructuredCloneReader

Categories

(Core :: JavaScript Engine, enhancement, P2)

All
Linux
enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox-esr52 --- wontfix
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- fixed

People

(Reporter: decoder, Assigned: decoder)

References

(Blocks 1 open bug)

Details

(Keywords: sec-want, Whiteboard: [adv-main61-][post-critsmash-triage])

Attachments

(1 file, 2 obsolete files)

This bug is about landing a fuzzing target for StructuredCloneReader once bug 1431090 landed. We need to make sure to extensively test this before landing in order to not accidentally expose a bug.
Attached patch fuzzing-target-sc.patch (obsolete) — Splinter Review
This patch adds the fuzzing target for StructuredCloneReader, but also changes the way we add libfuzzer's instrumentation flags to the target source code. Local experiments have shown that adding the flags to all of the JS engine (like I implemented it in bug 1431090) causes quite some noise. Instead we should only build the files that we are actually targeting with those flags. This requires taking them out of UNIFIED_SOURCES when a fuzzing build with libfuzzer is requested.
Attachment #8947083 - Flags: review?(sphink)
Comment on attachment 8947083 [details] [diff] [review]
fuzzing-target-sc.patch

Review of attachment 8947083 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/fuzz-tests/testStructuredCloneReader.cpp
@@ +8,5 @@
> +#include "mozilla/ScopeExit.h"
> +#include "jsapi.h"
> +#include "fuzz-tests/tests.h"
> +#include "vm/Interpreter.h"
> +#include "jscntxtinlines.h"

I think our style checker is going to scream at you here. I really should review jorendorff's patch to add a --fixup option to just reorder the includes for you.

@@ +13,5 @@
> +
> +using namespace JS;
> +using namespace js;
> +
> +extern JS::PersistentRootedObject gGlobal;

Pick one way or the other -- either say 'using namespace JS' and leave off the JS:: qualifier, or qualify everywhere. (Or do 'using' in a non-toplevel scope and qualify at the top.)

@@ +15,5 @@
> +using namespace js;
> +
> +extern JS::PersistentRootedObject gGlobal;
> +extern JSContext* gCx;
> +

Can you comment where these two are actually defined?

@@ +33,5 @@
> +
> +    // Make sure to pad the buffer to a multiple of kSegmentAlignment
> +    const size_t kSegmentAlignment = 8;
> +    size_t size_diff = kSegmentAlignment - (size % kSegmentAlignment);
> +    size_t buf_size = size + size_diff;

Probably easier to read as

    size_t buf_size = JS_ROUNDUP(size, kSegmentAlignment);

not to mention that this is always padding (so it adds 8 bytes to an already-aligned size). Unless you want that for some reason.
Attachment #8947083 - Flags: review?(sphink) → review+
Priority: -- → P2
Attached patch fuzzing-target-sc.patch (obsolete) — Splinter Review
Updated patch to address review comments. Green on try, will land as soon as bug 1434384 landed because that is the only remaining unfixed security bug that could be easily found with this patch.
Attachment #8947083 - Attachment is obsolete: true
Attachment #8951229 - Flags: review+
Rebased patch plus some cleanups with flags. Cleanup parts reviewed by :jandem over IRC.
Attachment #8951229 - Attachment is obsolete: true
Attachment #8963063 - Flags: review+
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/4ee5ca2ac54a
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
IIRC, we fuzz the release branches as well. Is this worth a backport to Beta60?
Flags: needinfo?(choller)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #7)
> IIRC, we fuzz the release branches as well. Is this worth a backport to
> Beta60?

We only do so in exceptional cases right now, so no. I would let this ride the trains.
Flags: needinfo?(choller)
Group: javascript-core-security → core-security-release
Whiteboard: [adv-main61-]
Flags: qe-verify-
Whiteboard: [adv-main61-] → [adv-main61-][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.