Closed Bug 937404 Opened 6 years ago Closed 6 years ago

Possible buffer size miscalculation in JS::NotableStringInfo

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28
Tracking Status
firefox25 --- unaffected
firefox26 --- fixed
firefox27 --- fixed
firefox28 --- fixed
firefox-esr17 --- unaffected
firefox-esr24 --- unaffected
b2g18 --- unaffected
b2g-v1.1hd --- unaffected
b2g-v1.2 --- fixed

People

(Reporter: jimb, Assigned: jimb)

Details

(Keywords: sec-audit, Whiteboard: [qa-])

Attachments

(3 files, 2 obsolete files)

The move constructor for NotableStringInfo doesn't initialize the bufferSize member. However, it seems that bufferSize is only ever expected to be live in NotableStringInfo's copy constructor, which itself doesn't seem to be used.

This seems to have been the case since the code was added; Waldo is trying out a patch that deletes the copy constructor on Nightly, Aurora, Beta, and Release to make sure.
Keywords: sec-audit
esr24 (and by extension b2g18/esr17) and mozilla-release don't have NotableStringInfo at all, nor any move-construction stuff, and only if moves are used ubiquitously would this malign constructor not be used.

Both aurora and beta have compiled the JS engine and are churning away on the rest of Gecko still, no certain conclusions yet.
Both aurora/beta compiled, so it looks as though the constructor's unused on aurora/beta, so I think we're safe here.

Nonetheless, to be absolutely sure (and to catch any platforms I can't/didn't compile on), I think we should land patches to delete the back constructor on trunk and on all branches.  jimb, you agree?  I can post the aurora/beta backports (not directly applicable but simple backports) for review/approval if you agree.
Yes, let's delete it since it's 1) unsafe, and 2) unused.
Attached patch Trunk patchSplinter Review
It seems to me that if we're killing the copy constructor that uses bad data, and that's the only use of the data, we should kill the bad data entirely as well.  If it's not there at all, *no one* can misuse it.  :-)  Super-minor delta to the previous patch.
Attachment #830520 - Attachment is obsolete: true
Attachment #8334124 - Flags: review?(jimb)
Attached patch Beta patch (obsolete) — Splinter Review
Attachment #8334129 - Attachment is obsolete: true
Attachment #8334124 - Flags: review?(jimb) → review+
I'm told over IRC that the trunk patch will need s/JS_ValueToString/JS::ToString/ to work, just to note.  Trying to round up all the patches here for pushing tonight, uploaded to bug, but may not have time -- we'll see, if I do I'll obsolete everything here and leave just the obvious targeted patches up.
Er.  Wrong bug for comment 9, sorry.  :-(
Comment on attachment 8334127 [details] [diff] [review]
Aurora version of previous patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 893222
User impact if declined: in theory none -- fairly sure we're removing dead code -- but if this code actually *isn't* dead somehow, it's potentially sec-critical; removing the code conclusively proves safety both into the future, and retroactively
Testing completed (on m-c, etc.): this fix is correct if it builds, so no meaningful testing required beyond landing it on m-c (done an hour ago, built fine locally before that, ultimate results will arrive in a few hours)
Risk to taking this patch (and alternatives if risky): basically none, could back out or fix the underlying issue if this isn't actually dead code
String or IDL/UUID changes made by this patch: N/A
Attachment #8334127 - Flags: approval-mozilla-aurora?
Comment on attachment 8334131 [details] [diff] [review]
Beta patch, qref'd

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 893222
User impact if declined: in theory none -- fairly sure we're removing dead code -- but if this code actually *isn't* dead somehow, it's potentially sec-critical; removing the code conclusively proves safety both into the future, and retroactively
Testing completed (on m-c, etc.): this fix is correct if it builds, so no meaningful testing required beyond landing it on m-c (done an hour ago, built fine locally before that, ultimate results will arrive in a few hours)
Risk to taking this patch (and alternatives if risky): basically none, could back out or fix the underlying issue if this isn't actually dead code
String or IDL/UUID changes made by this patch: N/A
Attachment #8334131 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/35f1c196e7d3 fixed on central
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Attachment #8334131 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8334127 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Opening -- this was removing unsafe code, but the unsafe code was dead, so no actual danger, as demonstrated by the success in removing it everywhere without issue.
Group: core-security
I don't think this needs QA verification. If anyone thinks that's a mistake please remove the [qa-] whiteboard tag and add the verifyme keyword.
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.