1770154 - Reconcile old/ and new/ NotificationDB.sys.mjs

Status: RESOLVED DUPLICATE of bug 1889930

(Core :: DOM: Notifications, task)

Description

[Andrew McCreight [:mccr8]]

The sandbox escape portion of the pwn2own exploit involves code in dom/notification/old/NotificationDB.jsm. However, on Nightly we use a different implementation, in dom/notification/new/NotificationDB.jsm (note the "new" instead of "old").

As far as I can see, this split occurred in 2019 with bug 1547877, and there have been no substantial changes to either file since then, and I don't see any open bugs talking about NotificationDB.jsm. Aside from the exploit, it seems bad for code to exist in this split state for so long, especially given that there doesn't seem to be any active work to bring them together.

Comment 1

[Andrew McCreight [:mccr8]]

Mossop, it looks like you were a reviewer on bug 1547877, and the author and other reviewer of that patch are no longer at Mozilla, so maybe you could figure out who could look into a plan for this code? Thanks.

Comment 2

[Andrew McCreight [:mccr8]]

It seems like too large of a change to do in a dot release, but technically we could fix the sandbox escape by changing this function in moz.configure to always return true, instead of only returning true in Nightly.

Comment 3

[Andrew McCreight [:mccr8]]

The immediate issue is going to be fixed in bug 1770137, so I'm going to mark this as sec-other. This is more of a long-term code health kind of issue than anything we need to fix soon.

Comment 4

[Jens Stutte [:jstutte]]

Tim, are there any plans to work on this? It blocks us from removing LMDB support entirely from the tree, IIUC.

Comment 5

[Dave Townsend [:mossop]]

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

Tim, are there any plans to work on this? It blocks us from removing LMDB support entirely from the tree, IIUC.

Do we have a bug tracking removal of LMDB? I see at least four other consumers of the LMDB service in tree so wondering what the plan is there.

Comment 6

[Jens Stutte [:jstutte]]

(In reply to Dave Townsend [:mossop] from comment #5)

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

Tim, are there any plans to work on this? It blocks us from removing LMDB support entirely from the tree, IIUC.

Do we have a bug tracking removal of LMDB? I see at least four other consumers of the LMDB service in tree so wondering what the plan is there.

Not yet as bug but as jira ticket for context, however we track the transition to rkv safemode in bug 1771882. Are we missing other consumers?

Comment 7

[Dave Townsend [:mossop]]

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

(In reply to Dave Townsend [:mossop] from comment #5)

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

Tim, are there any plans to work on this? It blocks us from removing LMDB support entirely from the tree, IIUC.

Do we have a bug tracking removal of LMDB? I see at least four other consumers of the LMDB service in tree so wondering what the plan is there.

Not yet as bug but as jira ticket for context, however we track the transition to rkv safemode in bug 1771882. Are we missing other consumers?

Sorry I missed that kvstore.jsm already uses the safe mode. In which case I don't think this bug blocks removing LMDB. The old component uses a JSON file on disk while the new component uses rkv safe mode.

Comment 8

[Jens Stutte [:jstutte]]

(In reply to Dave Townsend [:mossop] from comment #7)

Sorry I missed that kvstore.jsm already uses the safe mode. In which case I don't think this bug blocks removing LMDB. The old component uses a JSON file on disk while the new component uses rkv safe mode.

Thanks, then sorry for the noise here!

Comment 9

[Kagami Rosylight [:saschanaz] (they/them)]

Given that bug 1770137 is now public, should this also be public too?

Comment 10

[Kagami Rosylight [:saschanaz] (they/them)]

Bug 1777973 preferred the "old" JSON implementation over the rkv-based "new" implementation. Does the same apply here?

Comment 11

[Dave Townsend [:mossop]]

(In reply to Kagami [:saschanaz] from comment #10)

Bug 1777973 preferred the "old" JSON implementation over the rkv-based "new" implementation. Does the same apply here?

There are two key differences between the xulstore component and this one:

1. In the case of xulstore the rkv based version included a whole bunch of custom binary code for interacting with the store so there was a lot of additional complexity that could be removed. Here the store appears to be entirely accessed through kvstore.sys.mjs, a shared module in use by a number of other components. The two NotificationDB.sys.mjs modules appear to be very similar in terms of complexity, though I have not dug into the details.
2. The data stored in xulstore is small and non-critical. We decided that it was fine to essentially throw the xulstore data away for our nightly population making the work to switch back trivial. I don't know that the same is true here so switching back to the old component might require writing migration code to import the rkv data into the json store. If we have telemetry on how much nightly users use notifications that might help us understand this better.

Given the migration issue it would be simplest to do the opposite here and promote the new version to release and in the couple of minutes I've had to look I don't see a reason not to do that, but there are three questions that it would be good to answer first:

1. Do we intent to maintain kvstore.sys.mjs for the foreseeable future?
2. Are the two components actually of similar complexity.
3. Are we seeing any signs (bug reports, telemetry, support etc.) that the new or old versions are more prone to bugs?

Comment 12

[Kagami Rosylight [:saschanaz] (they/them)]

(In reply to Dave Townsend [:mossop] from comment #11)

Thanks for the quick answer!

If we have telemetry on how much nightly users use notifications that might help us understand this better.

Sounds like a good work item. Maybe it can be around nsINotificationStorage::Put() and also Get().

Do we intent to maintain kvstore.sys.mjs for the foreseeable future?

At least the change history doesn't say so...

Are we seeing any signs (bug reports, telemetry, support etc.) that the new or old versions are more prone to bugs?

This bug and bug 1770227 mentioned it, but I guess that's it for now.

It's not any urgent right now, just wanted to know the current state. Let me add the telemetry and see what happens.

Comment 13

[Dave Townsend [:mossop]]

(In reply to Kagami [:saschanaz] from comment #12)

(In reply to Dave Townsend [:mossop] from comment #11)

Do we intent to maintain kvstore.sys.mjs for the foreseeable future?

At least the change history doesn't say so...

No changes may just mean there have been no bugs to be fixed. What I'm really getting at is is there some plan to move all the other consumers of kvstore.sys.mjs to something else. If not I'm assuming that if we hit issues with kvstore then we'll fix them as they could impact multiple components.

Comment 14

[Dave Townsend [:mossop]]

(In reply to Kagami [:saschanaz] from comment #12)

(In reply to Dave Townsend [:mossop] from comment #11)

If we have telemetry on how much nightly users use notifications that might help us understand this better.

Sounds like a good work item. Maybe it can be around nsINotificationStorage::Put() and also Get().

I was hoping there might be something present already but it appears not for quite some time. We should be wary that the work involved in deciding on what metrics are important and adding them might outweigh the work involved in just writing the migration code.

Comment 15

[Jens Stutte [:jstutte]]

(In reply to Dave Townsend [:mossop] from comment #13)

(In reply to Kagami [:saschanaz] from comment #12)

(In reply to Dave Townsend [:mossop] from comment #11)

Do we intent to maintain kvstore.sys.mjs for the foreseeable future?

At least the change history doesn't say so...

No changes may just mean there have been no bugs to be fixed. What I'm really getting at is is there some plan to move all the other consumers of kvstore.sys.mjs to something else. If not I'm assuming that if we hit issues with kvstore then we'll fix them as they could impact multiple components.

Yeah, my understanding is that this just wraps rkv so I would not be surprised if there is low activity and it just works as is.

Comment 16

[Kagami Rosylight [:saschanaz] (they/them)]

Harveer removed the rkv based storage in bug 1889930.