Closed Bug 1346759 Opened 3 years ago Closed 9 months ago

Consider secure prngs when creating NullPrincipals to allow string comparison of serialized NullPrincipals

Categories

(Core :: Security: CAPS, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox67 --- fixed

People

(Reporter: ckerschb, Assigned: jkt)

References

(Depends on 2 open bugs, Blocks 2 open bugs)

Details

(Keywords: sec-want, Whiteboard: [adv-main67-])

Attachments

(1 file, 2 obsolete files)

As discussed within Bug 1343279, in particular comment 27, 28 and 29 [1] we should consider to _not_ rely on pointer comparison [2] of NullPrincipals because principals might have been serialized.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1343279#c27
[2] https://dxr.mozilla.org/mozilla-central/source/caps/nsNullPrincipal.cpp#144
Keywords: sec-want
What's the perf impact?
The patch in question just changes the comparison, afaict.  It does not change the prng used.
> The patch in question just changes the comparison, afaict.

It does, however I was told the e10s changes already did this to make these thread safe. Perhaps I'm misunderstanding that though.
The single test failure appears to be a race condition caused by calling isInstallAllowed() in resource://gre/modules/addons/XPIInstall.jsm twice in the updated case.
The relevant code is nsUUIDGenerator::GenerateUUIDInPlace and does the following:

Windows: CoCreateGuid.  I don't believe this is cryptographically secure in any way, nor is it really using a prng, afaict.
Mac: CFUUIDCreate.  I can't quickly find information on this.
If HAVE_ARC4RANDOM is defined (which is when? I don't think it is on Linux...): use arc4random
Else: use random()

So unless I am missing something, we are not using a csprng for this stuff.
My understanding is the uuid crate in rust uses only a csprng so I moved the code to using that. It will need some work to neaten it up I suspect however I think this might be a viable solution so long as talos is happy.

Try run here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0a376baca47f211517f7ec8f338d9980f03ce44
Assignee: nobody → jkt
Comment on attachment 9025726 [details]
Bug 1346759 - Use URI comparison for null principals instead of pointer comparison.

:ckerschb would you be able to provide any initial feedback on this patch?
Attachment #9025726 - Attachment description: Bug 1346759 - Use normal comparison for null principals instead of ptr comparison. ffi uuid and xpi debugging → Bug 1346759 - Use normal comparison for null principals instead of ptr comparison. ffi uuid and xpi debugging
Attachment #9025726 - Flags: feedback?(ckerschb)
Attachment #9025726 - Attachment description: Bug 1346759 - Use normal comparison for null principals instead of ptr comparison. ffi uuid and xpi debugging → Bug 1346759 - Use normal comparison for null principals instead of ptr comparison. ffi uuid and xpi debugging
Comment on attachment 9025726 [details]
Bug 1346759 - Use URI comparison for null principals instead of pointer comparison.

Yeah, that looks pretty good. Thanks for working on this, wanted to have that landed a long time ago!
Attachment #9025726 - Attachment description: Bug 1346759 - Use normal comparison for null principals instead of ptr comparison. ffi uuid and xpi debugging → Bug 1346759 - Use normal comparison for null principals instead of ptr comparison. ffi uuid and xpi debugging
Attachment #9025726 - Flags: feedback?(ckerschb) → feedback+
Comment on attachment 9025726 [details]
Bug 1346759 - Use URI comparison for null principals instead of pointer comparison.

:heycam other than the feedback :ckerschb gave is there anything I need to think about further from an oxidation standpoint when landing this? I know the Rust is super simple but I haven't done ffi before or anything in Firefox.
Flags: needinfo?(cam)
Attachment #9025726 - Attachment description: Bug 1346759 - Use normal comparison for null principals instead of ptr comparison. ffi uuid and xpi debugging → Bug 1346759 - Use normal comparison for null principals instead of ptr comparison. ffi uuid and xpi debugging
Commented in Phabricator.
Flags: needinfo?(cam)
Hey :kmag,

This code changes how we compare principals from pointer comparison to string comparison.

In toolkit/mozapps/extensions/AddonManager.jsm we have the following code:

> else if (!aBrowser.contentPrincipal || !aInstallingPrincipal.subsumes(aBrowser.contentPrincipal)) {

My current assumption is that null principals are being serialized across the process boundary and not passing this subsumes( code where as in my test run it is returning true.

To make the failing test (toolkit/mozapps/extensions/test/xpinstall/browser_datauri.js) pass I changed to:

> } else if (aInstallingPrincipal.isNullPrincipal || !aBrowser.contentPrincipal || !aInstallingPrincipal.subsumes(aBrowser.contentPrincipal)) {

---

So I could instead add "Harness.installBlockedCallback = install_blocked;" to toolkit/mozapps/extensions/test/xpinstall/browser_datauri.js to make it pass.

This is because it is still failing the following check:
> if (!this.isInstallAllowed(aMimetype, aInstallingPrincipal)) {


Are there other concerning changes you can think of in the addon code to making this change?
Flags: needinfo?(kmaglione+bmo)
Attachment #9025726 - Attachment description: Bug 1346759 - Use normal comparison for null principals instead of ptr comparison. ffi uuid and xpi debugging → Bug 1346759 - Use URI comparison for null principals instead of pointer comparison.
Depends on: 1508877
Blocks: 1521876

Bug 1346759 - Update uuid crate to 0.7.2

Attachment #9042081 - Attachment is obsolete: true
Attached file Update uuid to 0.7.2 (obsolete) —
Depends on: 1525970
Attachment #9042108 - Attachment is obsolete: true
Pushed by jkingston@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ef4325327e46
Use URI comparison for null principals instead of pointer comparison. r=ckerschb,bholley
Status: NEW → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

Removing ni as I spoke with :zombie on irc with :aswan. The agreement was that the code was fine however it might make sense to move it to isInstallAllowed however that error produced is different.

Flags: needinfo?(kmaglione+bmo)
Whiteboard: [adv-main67-]
Blocks: 1443925
You need to log in before you can comment on or make changes to this bug.