Closed Bug 1180088 Opened 5 years ago Closed 4 years ago

Fix security checks so that they work without appid by tagging a child process with a package identifier.

Categories

(Core :: DOM: Core & HTML, defect, P1)

defect

Tracking

()

RESOLVED FIXED
FxOS-S10 (30Oct)
feature-b2g 2.5+
Tracking Status
firefox44 --- fixed

People

(Reporter: pauljt, Assigned: hchang)

References

Details

Attachments

(1 file, 3 obsolete files)

We need to change security checks that currently are done in the parent process. Currently many of them are heavily based on app-ids and installed apps. This may need to be changed when moving to a signed package model.
Priority: -- → P1
blocking-b2g: --- → 2.5+
Hi Paul,
Who do you think is the right person to take this bug?
Should we ask Henry or Kan-Ru?
Flags: needinfo?(ptheriault)
Assignee: nobody → ptheriault
Flags: needinfo?(ptheriault)
Priority: P1 → P2
Depends on: 1191740
Reference bug: bug 1178533.

Bug 1178533 will not deal with the parent side permission check, which means AppProcessAssertPermission would fail for signed content even the permission is granted in child side. This bug might be the one to address this issue.
See Also: → 1178533
Assignee: ptheriault → hchang
Status: NEW → ASSIGNED
I don't have a very clear and generic idea to this issue but at least we need to fix the AssertAppProcessPermission-against-a-signed-content failure issue. [1] is a experimental fix by creating a custom mozIApplication and associating it with a TabContext. My gut feeling is [1] is not way too far from the final solution. Bug 1191740 is adding the originAttribute to TabContext so that part of [1] will be done by then. However, I feel like it's difficult to remove all the use of appId in TabContext in one jump. For the signed content, we may need to still use its owner frame's appId as its appId but return a origin-based mozIApplication in like GetOwnOrContainingApp().


[1] https://github.com/elefant/gecko-dev/commit/8e5c74c67ad4313e45d1f17c08c978249fc1a253
Target Milestone: --- → FxOS-S9 (16Oct)
Blocks: TV_Gecko_P2
Did I miss something? This bug is blocking TV Gecko?
Flags: needinfo?(jocheng)
No longer blocks: TV_Gecko_P2
Flags: needinfo?(jocheng)
Attached patch Bug118088-Part1.diff (obsolete) — Splinter Review
Hi Bobby, Kanru,

To fix the parent process permission assertion issue, we need TabContext to have the signed package origin to look up the permission table. Bug 1191740 has added originAttribute to TabContext. However, we still require at least two things to be done:

1) To update TabContext::mOriginAttributes.mSignedPkg:

When we are creating TabContext, the package identifier is still unknown. Since TabContext is meant to be immutable after construction, we totally have no chance to update the "signedPkg" of OriginAttributes.

2) To have the signed packaged content origin (no suffix):

We need the origin attributes as well as the "originNoSuffix" to look up the permission table. It's easy to add an "origin" field to TabContext. However, from what I understand, TabContext should have only the invariant information. A tab can be used to load content in different origin. If TabContext has origin, we need to update TabContext every time we go across the origin boundary.

---------------------------------------------------------------------

Thankfully in new security model, the signed packaged content would be loaded in a separate process, which means the TabContext will be used only for the signed packaged content in the same origin. Based on this assumption, adding "origin" to TabContext makes more sense to me. So it solves issue (2).

To solve issue (1), at the time we are trying to reload an URL in a newly created process [1], we can set the TabContext with package identifier. Great! Besides, we also need to create the TabContext with the signed package origin. 

I just attached a patch to

1) Create a TabContext with package identifier and the package origin in [1]
2) Deal with the TabContext IPC part

TabContext::SignedPkgOriginNoSuffix would be used in AppProcessChecker [2] to look up the permission table. Before I go further to do this, I'd like to get your feedback since you are the potential reviewer :)

Thanks!

[1] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsFrameLoader.cpp#287
[2] https://dxr.mozilla.org/mozilla-central/source/dom/ipc/AppProcessChecker.cpp#127
Flags: needinfo?(kchen)
Flags: needinfo?(bobbyholley)
Depends on: 1216062
No longer depends on: 1191740
Attached patch patch for review (obsolete) — Splinter Review
Attachment #8674136 - Attachment is obsolete: true
Flags: needinfo?(kchen)
Flags: needinfo?(bobbyholley)
Comment on attachment 8675608 [details] [diff] [review]
patch for review

Hi Kanru, Bobby,

I did more tests and the approach I proposed seems to work very well. With the patch applied 'tcp-socket' assertion is passed on parent side ([1] would pass).

I was hoping to write a mochitest (call navigator.mozTCPSocket.open and the process shouldn't crash.) for this patch but failed because MOZ_CHILD_PERMISSIONS is not defined on browser. I am not sure if the mochitest infrastructure on B2G is able to do the test. Need to do further investigation.

So, could you have a review on my patch? Thanks!

[1] https://dxr.mozilla.org/mozilla-central/source/dom/network/TCPSocketParent.cpp#151
[2] https://dxr.mozilla.org/mozilla-central/source/dom/ipc/AppProcessChecker.cpp#9
Attachment #8675608 - Flags: review?(kchen)
Attachment #8675608 - Flags: review?(bobbyholley)
Comment on attachment 8675608 [details] [diff] [review]
patch for review

Review of attachment 8675608 [details] [diff] [review]:
-----------------------------------------------------------------

I'm pretty busy at the moment. Kanru, are you comfortable doing this review?
Attachment #8675608 - Flags: review?(bobbyholley)
Priority: P2 → P1
Target Milestone: FxOS-S9 (16Oct) → FxOS-S10 (30Oct)
Comment on attachment 8675608 [details] [diff] [review]
patch for review

Review of attachment 8675608 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsIFrameLoader.idl
@@ +57,3 @@
>     * Throws an exception with non-remote frames.
>     */
> +  void switchProcessAndLoadURI(in nsIURI aURI, in ACString aPackageId);

The second parameter is not optional so the test dom/base/test/test_frameLoader_switchProcess.html needs update.

::: dom/ipc/AppProcessChecker.cpp
@@ +126,5 @@
>  
> +// Do a origin-based permission check for signed packaged content.
> +static bool
> +CheckSignedPkgPermission(const nsACString& aOrigin, const char* aPermission)
> +{

Despite the name, this function only checks the permission based on the origin right? Change the function name to CheckOriginPermission or really pass OriginAttributes as a parameter and warn if the origin passed in is not signed pkg.

::: dom/ipc/PTabContext.ipdlh
@@ +38,5 @@
>    // The ID of the app containing this app/browser frame, if applicable.
>    uint32_t frameOwnerAppId;
> +
> +  // The origin without originAttribute suffix for a signed package.
> +  nsCString signedPkgOriginNoSuffix;

Add a comment saying that it's empty string for other content.

::: dom/ipc/TabContext.h
@@ +113,5 @@
> +  /**
> +   * Returns the origin associated with the tab (w/o suffix) if this tab owns 
> +   * a signed packaged content.
> +   */
> +  const nsACString& SignedPkgOriginNoSuffix() const;

nit: trailing whitespace

Add a comment saying that the return value is empty string if the tab is not a signed package. Even better, return a Maybe<nsCString>

@@ +178,5 @@
> +   * The signed package origin without suffix. Since the signed packaged
> +   * web content is always loaded in a separate process, it makes sense
> +   * that we store this immutable value in TabContext.
> +   */
> +  nsCString mSignedPkgOriginNoSuffix;

Ditto, use Maybe<nsCString> or add a comment saying that it's empty for others.
Attachment #8675608 - Flags: review?(kchen) → review+
Thanks Kanru!

Beside the patch, I am working on two other things:

1) Figuring out if we can have a mochitest test case on B2G. (on browser is unlikely cuz the parent process permission assertion is only for B2G).

2) If we navigate from "about:blank" to a signed package, nsFrameLoader::SwitchProcessAndLoadURL wouldn't be called due to [1]. A followup bug is be needed to address this issue. The solution might be using a better way to determine if it's the newly created process for loading signed package.

[1] http://hg.mozilla.org/mozilla-central/file/9605da94e75d/dom/ipc/TabParent.cpp#l512

(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #10)
> Comment on attachment 8675608 [details] [diff] [review]
> patch for review
> 
> Review of attachment 8675608 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/nsIFrameLoader.idl
> @@ +57,3 @@
> >     * Throws an exception with non-remote frames.
> >     */
> > +  void switchProcessAndLoadURI(in nsIURI aURI, in ACString aPackageId);
> 
> The second parameter is not optional so the test
> dom/base/test/test_frameLoader_switchProcess.html needs update.
> 
> ::: dom/ipc/AppProcessChecker.cpp
> @@ +126,5 @@
> >  
> > +// Do a origin-based permission check for signed packaged content.
> > +static bool
> > +CheckSignedPkgPermission(const nsACString& aOrigin, const char* aPermission)
> > +{
> 
> Despite the name, this function only checks the permission based on the
> origin right? Change the function name to CheckOriginPermission or really
> pass OriginAttributes as a parameter and warn if the origin passed in is not
> signed pkg.
> 
> ::: dom/ipc/PTabContext.ipdlh
> @@ +38,5 @@
> >    // The ID of the app containing this app/browser frame, if applicable.
> >    uint32_t frameOwnerAppId;
> > +
> > +  // The origin without originAttribute suffix for a signed package.
> > +  nsCString signedPkgOriginNoSuffix;
> 
> Add a comment saying that it's empty string for other content.
> 
> ::: dom/ipc/TabContext.h
> @@ +113,5 @@
> > +  /**
> > +   * Returns the origin associated with the tab (w/o suffix) if this tab owns 
> > +   * a signed packaged content.
> > +   */
> > +  const nsACString& SignedPkgOriginNoSuffix() const;
> 
> nit: trailing whitespace
> 
> Add a comment saying that the return value is empty string if the tab is not
> a signed package. Even better, return a Maybe<nsCString>
> 
> @@ +178,5 @@
> > +   * The signed package origin without suffix. Since the signed packaged
> > +   * web content is always loaded in a separate process, it makes sense
> > +   * that we store this immutable value in TabContext.
> > +   */
> > +  nsCString mSignedPkgOriginNoSuffix;
> 
> Ditto, use Maybe<nsCString> or add a comment saying that it's empty for
> others.
This bug is part of New Security Model, which is aimed to offer a developer-mode prototype in 2.5 (default pref-off). Should not be a blocker for 2.5.
blocking-b2g: 2.5+ → ---
feature-b2g: --- → 2.5+
Attached patch patch (r+'d) (obsolete) — Splinter Review
Attachment #8677428 - Attachment is obsolete: true
Attachment #8675608 - Attachment is obsolete: true
Blocks: 1214572
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/94ca3a0db572
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Nice job! Thanks Henry and Kan-Ru!
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.