1741201 - Out-of-bounds write due to integer overflow [@ ObjectStoreAddOrPutRequestOp::DoDatabaseWork]

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect, P1)

Description

[Christian Holler (:decoder)]

In order to flatten clone data, we resize an nsCString here:

https://searchfox.org/mozilla-central/rev/3c8a7970944daaf917b547dffc0790bcd37cadc1/dom/indexedDB/ActorsParent.cpp#19764

The problem here is that flatCloneData.SetLength takes uint32_t while cloneDataSize is size_t. So this can be overflowed to make flatCloneData small and then cause an out-of-bounds write when calling ReadBytes which takes the length as size_t again.

Found with -Wshorten-64-to-32 with some custom filters on top of it + manual code inspection.

Comment 1

[Jens Stutte [:jstutte]]

If I read that right the nsCString is just used as a fixed-size char buffer. We can probably easily avoid this?

Comment 2

[Jari Jalkanen]

(In reply to Jens Stutte [:jstutte] from comment #1)

If I read that right the nsCString is just used as a fixed-size char buffer. We can probably easily avoid this?

Yes, this is not hard to change.

Comment 3

[Jari Jalkanen]

Attachment: Bug 1741201 - Increase intermediate byte storage maximum capacity. r=#dom-storage-reviewers

Comment 4

[Jari Jalkanen]

Comment on attachment 9250910 [details]
Bug 1741201 - Increase intermediate byte storage maximum capacity. r=#dom-storage-reviewers

Security Approval Request

• How easily could an exploit be constructed based on the patch?:
  • This branch can only be entered when maximum compressed size is less than max(int32_t) IndexedDatabaseManager.cpp#175 . The preferences can adjust the value but the type constrains it, ActorsParent.cpp#19480 .
  • The total size of JSStructuredCloneData is limited by the 512 MB message size cap as a whole. Internally, it uses smaller buckets but these are not related to the messages.
  • This code is executed is before the data is stored so the sqlite3 limitations do not apply.
  • The message size cap is enforced by MessageLink.cpp
  • To exploit the finding, the content process would need to be compromized
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: All
• If not all supported branches, which bug introduced the flaw?: Bug 994190
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: It is essentially a four line change, they are not hard to create and the dependency has existed since bug 1235261 . The risk is low because we are just replacing a container with another..
• How likely is this patch to cause regressions; how much testing does it need?: Regressions are unlikely because we are just replacing an old container with another old container.

Comment 5

[Jari Jalkanen]

First question in the sec-approval request updated after feedback from [:janv]

Comment 6

[Jens Stutte [:jstutte]]

Hi Daniel, did this sec-approval slip through? Thanks!

Comment 7

[Daniel Veditz [:dveditz]]

I imagine sec-approvals for sec-high bugs were held up because it was Release Candidate week and nothing could be uplifted.

Comment 8

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Comment on attachment 9250910 [details]
Bug 1741201 - Increase intermediate byte storage maximum capacity. r=#dom-storage-reviewers

Precisely. Approved to land and uplift.

Comment 9

[Frederik Braun [:freddy]]

Clearing Tom's needinfo, while he's away for the week. Happy to help in his stead if there are additional unresolved questions.

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Increase intermediate byte storage maximum capacity. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/3382e4da79289240c4c2fe718b461f7a58bce553
https://hg.mozilla.org/mozilla-central/rev/3382e4da7928

Comment 11

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7d2a42441d81

Approved for 96.0b4

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/a582903d0821

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Comment 14

[Jari Jalkanen]

(In reply to Release mgmt bot [:sylvestre / :calixte / :marco for bugbug] from comment #13)

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Done