Closed Bug 1637682 Opened 3 months ago Closed 3 months ago

A few Metadata fields are not serialized and deserialized, thus they may be undefined

Categories

(Core :: Javascript: WebAssembly, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla78
Tracking Status
firefox78 --- fixed

People

(Reporter: bbouvier, Assigned: lth)

References

(Blocks 1 open bug)

Details

(Keywords: sec-low, Whiteboard: [adv-main78+r])

Attachments

(1 file)

Thanks to dmajor who asked about this on slack.

A few fields in Metadata may not be serialized or deserialized: for instance, bigIntEnabled isn't serialized and deserialized here, leading to undefined behavior here.

Classifying as sec bug for now (I'm probably being paranoid), but I'd like that we take time thinking about what the consequences might be. v128enabled suffers from the same issue, as far as i can tell.

OK, good find. It's problematic that we don't have a system that catches these mistakes for us. We need to fix the problem for BigInt soonish because that rides the FF78 train.

The clang-10 landing (bug 1616692) bounced because of a ubsan complaint about this (I'm not sure why clang-9's sanitizers don't catch it). I'm very eager to re-land clang-10, since compiler updates bitrot really fast and I'm always catching up with new test failures. I had been planning on adding a suppression for this error while it's investigated, but, seeing that this is potentially a security bug... is that an ok thing to do?

This is at most a nightly-only concern since bigint and simd are nightly-only at the moment. Simd is even off by default. As for bigint, it's a new feature no content uses, and it's a boolean flag, so if it should have been false but is read as true we're just enabling code that isn't exercised, and if it should have been true but is read as false then we're at most throwing errors some places we shouldn't be, and we'll see those bug reports. So suppressing it should be just fine.

It's problematic that we don't have a system that catches these mistakes for us.

The right way is to put these raw fields in the MetadataCacheablePod, which are serialized and deserialized for free.

we're at most throwing errors some places we shouldn't be

Are we sure it might not cause MOZ_CRASHes? Even so, we could unhide this bug and suppress it then, because mozcrashes are safe.

Yeah, I can look into preparing a patch to move the fields tomorrow.

In general, the values of these switches that are exposed to about:config must be assumed to change at any moment, so if there's a MOZ_CRASH or other crash then that's a latent bug I think. I think we've generally been careful about this, we read switches once when compilation starts and then propagate those values, and we try to gate things at high levels (instructions that validate) but not low (specific code generation strategies), though of course bugs happen.

With that logic in mind, it sort of doesn't make a difference whether the fields are serialized, so long as we don't deserialize them but instead get current values...

Assignee: nobody → lhansen
Status: NEW → ASSIGNED

omitsBoundsChecks falls into this category too. It was already properly serialized/deserialized but can be moved to the POD.

Serialized/deserialized properties that are POD should be in the
CacheablePod structure so that we don't have to worry about adding
extra code to serialize/deserialize them; it's so easy to forget.

Nightly-only and probably without any real user impact => sec-low

Keywords: sec-low

On Benjamin's request

Group: core-security, javascript-core-security
Attachment #9148254 - Attachment description: Bug 1637682 - Make sure to serialize/deserialize all code properties. r?bbouvier → Bug 1637682 - Make sure to serialize/deserialize all code properties. r=bbouvier

Oops, forgot to update serializedSize() too.

Flags: needinfo?(lhansen)
Pushed by lhansen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/99ddf2c70b4f
Make sure to serialize/deserialize all code properties.  r=bbouvier
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla78
Backout by csabou@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/db5f5d127e24
Backed out changeset bc5e947dda0a for wasm related bustage CLOSED TREE

(In reply to Pulsebot from comment #14)

Backout by csabou@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/db5f5d127e24
Backed out changeset bc5e947dda0a for wasm related bustage CLOSED TREE

Note that this is a delayed reporting of the first backout; the second landing is still present.

Thanks a lot for the quick response on this, Benjamin and Lars!

Blocks: clang-10
You need to log in before you can comment on or make changes to this bug.