Closed Bug 1578453 Opened 2 years ago Closed 2 years ago

IndexGetKeyRequestOp::GetResponse and ObjectStoreGetKeyRequestOp::GetResponse always return a 0 response size in the getAll case

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla71
Tracking Status
firefox71 --- fixed

People

(Reporter: sg, Assigned: sg)

Details

Attachments

(1 file)

In https://searchfox.org/mozilla-central/source/dom/indexedDB/ActorsParent.cpp#24923, mResponse is swapped with a newly initialized, thus empty array, and then the size of its elements in calculated, which is always 0. This must be done either before swapping, or on the target array.

The same problem exists in IndexGetKeyRequestOp::GetResponse.

The effect of this is that the check for the maximum message size with always succeed.

I am not sure if this is a security issue, since I don't know why the limit on the message size exists. I marked it as a security issue for now to be sure.

Flags: needinfo?(htsai)

Before this, the response size was always calculated as 0, since it used the swapped-out array.
In addition, make use of std::accumulate to increase idiomacy.

Blocks: eviltraps
Status: NEW → ASSIGNED

Hi Freddy, we discussed this bug in our meeting that this is a eviltraps bug, instead of a security one. Do you think you can help us to unhide this? thanks!

Flags: needinfo?(htsai) → needinfo?(fbraun)

I'm confused. How is this an evil trap? It does not seem like this is annoying to users or even useful to websites that want to scare a user?
However, if you're sure this isn't a security bug, I'm happy to unhide.

Flags: needinfo?(fbraun)

As said before, I was not sure if this kind of bug is a security bug. After discussion in the team, it turned out that the effect would be as follows:

  • The response size for queries querying all keys in an object store or index is always reported as 0.
  • Normally, responses exceeding the maximum IPC message limit are caught within ActorsParent.cpp and turned into a IDB error, resulting is JS exception for the caller.
  • If a query were issued that resulted in a response size exceeding the maximum IPC message limit, this would instead tried to be passed to the IPC layer, which would detect this and crash the parent process.
Flags: needinfo?(fbraun)

I agree with Freddy that this isn't an evil trap. The point of evil trap bugs is DOS-style attacks that make it impossible for users to interact with the browser or close/leave a page. This is a very different type of bug.

No longer blocks: eviltraps

(In reply to :Gijs (he/him) from comment #6)

I agree with Freddy that this isn't an evil trap. The point of evil trap bugs is DOS-style attacks that make it impossible for users to interact with the browser or close/leave a page. This is a very different type of bug.

Ok, sorry, then I was misinterpreting something here.

Group: core-security
Priority: -- → P2
Flags: needinfo?(fbraun)
Keywords: checkin-needed

Pushed by btara@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d23a0a0ffa93
Correctly calculate response size in IndexGetKeyRequestOp::GetResponse and ObjectStoreGetKeyRequestOp::GetResponse r=ttung,asuth

Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71
You need to log in before you can comment on or make changes to this bug.