[Per App Network Traffic Metering] Collect per app traffic in UDP Socket API

RESOLVED FIXED in Firefox 40

Status

defect
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: schien, Assigned: dragana)

Tracking

unspecified
2.2 S10 (17apr)
ARM
Gonk (Firefox OS)
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(feature-b2g:2.2r+, firefox40 fixed, b2g-v2.2 wontfix, b2g-v2.2r fixed, b2g-master fixed)

Details

(Whiteboard: [red-tai])

Attachments

(2 attachments, 11 obsolete attachments)

38.95 KB, patch
dragana
: review+
Details | Diff | Splinter Review
39.08 KB, patch
Details | Diff | Splinter Review
No description provided.

Comment 1

6 years ago
I'll take this one :)
Assignee: nobody → jshih

Updated

6 years ago

Comment 2

5 years ago
In case it's useful, this was the bug where we did this for TCP sockets: bug 855951.
I did a quick skim on the patch in bug 855951. It uses the appID of first mozbrowser iframe for that content process. Is that still correct for the case we run multiple app on the same process?
Hi Vicent, have any engineer resource who can take over @jshih's unfinished work?
Flags: needinfo?(vchang)
(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #3)
> I did a quick skim on the patch in bug 855951. It uses the appID of first
> mozbrowser iframe for that content process. Is that still correct for the
> case we run multiple app on the same process?

Likely not. I don't know the code for the UDP socket api, but can't you get the principal of window using the api? that would give you the app id.
We have a new hire on board on Sep. 15. Maybe he can start from this bug? Do we have expected schedule for this?
Flags: needinfo?(vchang)
Assignee

Updated

5 years ago
Assignee: johnshih.bugs → dd.mozilla
Assignee

Comment 7

5 years ago
Posted patch fix v1 (obsolete) — Splinter Review
Attachment #8513467 - Flags: review?(jduell.mcbugs)
Assignee

Updated

5 years ago
Status: NEW → ASSIGNED

Comment 8

5 years ago
Comment on attachment 8513467 [details] [diff] [review]
fix v1

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

Looks good modulo the issue of how to get the appID for the socket in UDPSocket.

::: dom/network/UDPSocket.cpp
@@ +388,5 @@
>    }
>  
>    if (aLocalAddress.IsEmpty()) {
> +    rv = sock->Init(aLocalPort, /* loopback = */ false,
> +                    nsIScriptSecurityManager::UNKNOWN_APP_ID,

So I assume we always create a UDP socket within a browser/docshell context (it's a DOMEventTargetHelper, for instance), so presumably there's something we can access that will give us a valid appID here instead of UNKNOWN_APP_ID.  But I'm not sure what it is.  I looked at Websockets and TCP sockets, but websockets have gotten complicated by worker support, and TCP is written in JS, so they didn't give me an answer.

Honza, IIRC you reviewed the UDP socket stuff.  Do you know how we can get an appID here?  I.e. how do we get a nsILoadContext from this code.

::: dom/network/UDPSocketParent.cpp
@@ +151,5 @@
>      return rv;
>    }
>  
>    if (aHost.IsEmpty()) {
> +    rv = sock->Init(aPort, false, aAddressReuse, GetAppId(),

OK, so on the parent the ParentChannel here will set the nsUDPSocket's appID correctly.  But we need to also handle the case where we don't use e10s, and just create the UDPSocket on the parent. We should get the appID in the UDPSocket code and then (if we're using e10s) verify it here (and kill the child if they don't match).

::: netwerk/base/src/nsUDPSocket.cpp
@@ +258,5 @@
>  
>  nsUDPSocket::nsUDPSocket()
>    : mLock("nsUDPSocket.mLock")
>    , mFD(nullptr)
> +  , mAppId(NECKO_NO_APP_ID)

generally better to init to NECKO_UNKNOWN_APP_ID than NO_APP (that's 0, and is used by the parent process).  Not that it matters much since we require an appID to be passed in during Init()
Attachment #8513467 - Flags: review?(jduell.mcbugs) → review-

Comment 9

5 years ago
Honza, see previous comment.
Flags: needinfo?(honzab.moz)
I don't think I would be the right person to ask about this.  Either UDPSocket must always be created with reference to its window or the context must be obtained some other way (no idea how) - Jonas?
Flags: needinfo?(honzab.moz)
Assignee

Comment 11

5 years ago
Hi Jonas,
Do you know how to get appID in UDPSocket (see comment 8)
Flags: needinfo?(jonas)
What we should do is the following.

When the UDPSocket API is used to open a udp port we currently send an ipdl message from the child process to the parent process about which port etc should be opened.

As part of sending that ipdl message we should also send the nsIPrincipal of the page which is trying to open the port.

In the parent process, after we have received the principal, we should:

1. Call AssertAppPrincipal [1] to verify that the principal is valid for the given process.
2. Call into nsIPermissionManager to verify that the given nsIPrincipal has permission to use the
   UDPSocket API.

This is how we should do all security checks these days, but a lot of code still uses other mechanisms. If the UDPSocket API doesn't use this approach, it's worth changing it since it makes the next part easier.

Since you now have an nsIPrincipal, you can easily get the appid from that principal. Then you can attribute any network consumption to that app.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/ipc/AppProcessChecker.h#102
Flags: needinfo?(jonas)
Assignee

Comment 13

5 years ago
thank you Jonas.
Just one question: Does everything that is calling UDPSocket have a nsIPrincipal? Can I return an error if they do not or we accept calls without principal too?
Flags: needinfo?(jonas)
Assignee

Updated

5 years ago
Flags: needinfo?(jonas)
Assignee

Comment 14

5 years ago
media/mtransport/nr_socket_prsock.cpp is also using udp-socket-child.
Everything using the UDPSocket API should have a principal. You should definitely reject any calls in the parent that doesn't receive a principal.
Assignee

Comment 16

5 years ago
media/mtransport/nr_socket_prsock.cpp also uses udp-socket-child and it does not have any nsIPrincipal. I am not familiar with media/mtransport, I think it is use for TURN, ICE. Fixing this to have a principal is a different issue.

For now i will make it without reject in the parent.
As long as we don't remove any other security checks, that sounds ok then.
Assignee

Comment 18

5 years ago
Posted patch bug_935838_v2.patch (obsolete) — Splinter Review
If you do not have time, can you give it to someone who can review part with principals.
Attachment #8513467 - Attachment is obsolete: true
Attachment #8548738 - Flags: review?(jonas)
Assignee

Updated

4 years ago
Attachment #8548738 - Flags: review?(jonas)
Assignee

Comment 19

4 years ago
Posted patch bug_935838_v3.patch (obsolete) — Splinter Review
Attachment #8548738 - Attachment is obsolete: true
Attachment #8555456 - Flags: review?(honzab.moz)
Assignee

Comment 20

4 years ago
Posted patch fix v3 (obsolete) — Splinter Review
Attachment #8555456 - Attachment is obsolete: true
Attachment #8555456 - Flags: review?(honzab.moz)
Attachment #8557094 - Flags: review?(honzab.moz)
Comment on attachment 8557094 [details] [diff] [review]
fix v3

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

You still have to get a review from Jonas on this.

You also must update following consumers:
http://mxr.mozilla.org/mozilla-central/source/dom/push/PushService.jsm#1645
http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/discovery/discovery.js#81
http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/secondscreen/SimpleServiceDiscovery.jsm#147

::: dom/network/UDPSocket.cpp
@@ +387,5 @@
>      return rv;
>    }
>  
> +  nsCOMPtr<nsIGlobalObject> global = do_QueryInterface(GetOwner());
> +  nsCOMPtr<nsIPrincipal> principal = global->PrincipalOrNull();

check before use.

@@ +388,5 @@
>    }
>  
> +  nsCOMPtr<nsIGlobalObject> global = do_QueryInterface(GetOwner());
> +  nsCOMPtr<nsIPrincipal> principal = global->PrincipalOrNull();
> +  uint32_t appId;

move just before first use

@@ +393,5 @@
> +  if (!principal) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  principal->GetAppId(&appId);

check result

@@ +467,5 @@
>    if (NS_FAILED(rv)) {
>      return rv;
>    }
>  
> +  nsCOMPtr<nsIGlobalObject> obj = do_QueryInterface(GetOwner());

use &rv version, check result

@@ +468,5 @@
>      return rv;
>    }
>  
> +  nsCOMPtr<nsIGlobalObject> obj = do_QueryInterface(GetOwner());
> +  nsCOMPtr<nsIPrincipal> principal = obj->PrincipalOrNull();

again, check before use.

::: dom/network/UDPSocketParent.cpp
@@ +74,3 @@
>  {
> +  nsIPrincipal* principal = aPrincipal;
> +  if (principal) {

not sure why you need the local var?

@@ +86,5 @@
> +      return false;
> +    }
> +
> +    uint32_t permission = nsIPermissionManager::DENY_ACTION;
> +    permMgr->TestExactPermissionFromPrincipal(principal, "udp-socket",

the AssertAppProcessPermission doesn't work for you here?

::: dom/network/UDPSocketParent.h
@@ +26,5 @@
>    NS_DECL_NSIUDPSOCKETLISTENER
>  
>    UDPSocketParent();
>  
> +  bool Init(const IPC::Principal& aPrincipal,const nsACString& aFilter);

space
Attachment #8557094 - Flags: review?(honzab.moz) → review-
Assignee

Comment 22

4 years ago
(In reply to Honza Bambas (:mayhemer) from comment #21)
> Comment on attachment 8557094 [details] [diff] [review]
> fix v3
> 
> Review of attachment 8557094 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You still have to get a review from Jonas on this.
> 
> You also must update following consumers:
> http://mxr.mozilla.org/mozilla-central/source/dom/push/PushService.jsm#1645
> http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/discovery/
> discovery.js#81
> http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/secondscreen/
> SimpleServiceDiscovery.jsm#147

Do you know, probably are all NO_APP_ID (system app)?

> 
> ::: dom/network/UDPSocketParent.cpp
> @@ +74,3 @@
> >  {
> > +  nsIPrincipal* principal = aPrincipal;
> > +  if (principal) {
> 
> not sure why you need the local var?
> 

I need to transform it from IPC:Principal to nsIPrincipal.

> @@ +86,5 @@
> > +      return false;
> > +    }
> > +
> > +    uint32_t permission = nsIPermissionManager::DENY_ACTION;
> > +    permMgr->TestExactPermissionFromPrincipal(principal, "udp-socket",
> 
> the AssertAppProcessPermission doesn't work for you here?
> 

I was searching around how to permissions, so I wound this one.
I have not know about that one so probably that is what I need

\
Assignee

Comment 23

4 years ago
Posted patch bug_935838_v4.patch (obsolete) — Splinter Review
Attachment #8557094 - Attachment is obsolete: true
Attachment #8558605 - Flags: review?(jonas)
Attachment #8558605 - Flags: review?(honzab.moz)
Comment on attachment 8558605 [details] [diff] [review]
bug_935838_v4.patch

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

Thanks. r=honzab.

According what to use on those 3 places, I'm not sure.  You need to ask either Jonas (already r?) or persons responsible for the respective files.

::: dom/network/UDPSocketParent.cpp
@@ +78,5 @@
> +      if (!ContentParent::IgnoreIPCPrincipal() &&
> +          !AssertAppPrincipal(Manager()->Manager(), principal)) {
> +        return false;
> +      }
> +      principal->GetAppId(&mAppId);

also check result please.
Attachment #8558605 - Flags: review?(honzab.moz) → review+
HI Dragana,

For your information.
Bug 1070944 was landed, which separates network traffic between an app itself and its owned browser
iframe element. You have to add one extra argument |isInBrowser| to SaveNetworkStatsEvent().
You can refer to:
https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpTransaction.cpp#248
Assignee

Comment 26

4 years ago
Posted patch bug_935838_v5.patch (obsolete) — Splinter Review
I have added IsInBrowser parameter.
Honza, do you want to review it again?
Attachment #8558605 - Attachment is obsolete: true
Attachment #8558605 - Flags: review?(jonas)
Attachment #8563273 - Flags: review?(jonas)
Attachment #8563273 - Flags: feedback?(honzab.moz)
Comment on attachment 8563273 [details] [diff] [review]
bug_935838_v5.patch

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

gtg, thanks.
Attachment #8563273 - Flags: feedback?(honzab.moz) → feedback+
Comment on attachment 8563273 [details] [diff] [review]
bug_935838_v5.patch

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

Comments below. But the short of it is that it'll be more future-proof to deal in nsIPrincipal objects, rather than dealing in separate appid/browserflag members.

::: dom/network/UDPSocket.cpp
@@ +412,2 @@
>    if (aLocalAddress.IsEmpty()) {
> +    rv = sock->Init(aLocalPort, /* loopback = */ false, appId,

Why not pass a principal object to this function? Rather than the separate appid/browserflag parameters?

::: dom/network/UDPSocketParent.cpp
@@ +80,5 @@
> +      if (!ContentParent::IgnoreIPCPrincipal() &&
> +          !AssertAppPrincipal(Manager()->Manager(), principal)) {
> +        return false;
> +      }
> +      nsresult rv = principal->GetAppId(&mAppId);

Please keep an mPrincipal member, and just set that to the principal.

That is cleaner and more future proof (in case we add more information to principals) than to explicitly track appid/browserflag.

@@ +90,5 @@
> +        return false;
> +      }
> +    }
> +
> +    if (!AssertAppProcessPermission(Manager()->Manager(), "udp-socket")) {

Instead of doing this, call:

permissionManager->testExactPermissionFromPrincipal(principal, "udp-socket");

The AssertAppProcessPermission function really should be marked as deprecated since we shouldn't use it any more.

Using AssertAppPrincipal (like you do above) and then calling into the permissionManager is better.

@@ +169,5 @@
>    }
>  
>    if (aHost.IsEmpty()) {
> +    rv = sock->Init(aPort, false, aAddressReuse, mAppId, mIsInBrowserElement,
> +                    /* optional_argc = */ 1);

Then forward mPrincipal here instead.

@@ +180,5 @@
>      }
>  
>      mozilla::net::NetAddr addr;
>      PRNetAddrToNetAddr(&prAddr, &addr);
> +    rv = sock->InitWithAddress(&addr, aAddressReuse, mAppId,

Same here.
Attachment #8563273 - Flags: review?(jonas) → review-
Assignee

Comment 29

4 years ago
Posted patch bug_935838_v6.patch (obsolete) — Splinter Review
Attachment #8563273 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Attachment #8570473 - Flags: review?(jonas)
Comment on attachment 8570473 [details] [diff] [review]
bug_935838_v6.patch

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

Someone else needs to review the nsUDPSocket.cpp changes. I don't understand that code well enough.

Looks good otherwise.

::: dom/network/UDPSocketParent.cpp
@@ +70,5 @@
>    uint32_t appId = nsIScriptSecurityManager::UNKNOWN_APP_ID;
> +  if (mPrincipal) {
> +    nsresult rv = mPrincipal->GetAppId(&appId);
> +    if (NS_FAILED(rv)) {
> +      return nsIScriptSecurityManager::UNKNOWN_APP_ID;

Nit: maybe do |appId = nsIScriptSecurityManager::UNKNOWN_APP_ID| so that you only have one return statement?

Alternatively fully embrace multiple return statements and start the function with

if (!mPrincipal)
  return nsIScriptSecurityManager::UNKNOWN_APP_ID;

...
Attachment #8570473 - Flags: review?(jonas) → review+
Assignee

Updated

4 years ago
Attachment #8570473 - Flags: review?(honzab.moz)
Comment on attachment 8570473 [details] [diff] [review]
bug_935838_v6.patch

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

::: dom/network/UDPSocketParent.cpp
@@ +76,2 @@
>    }
>    return appId;

same as Jonas suggests:

if (!..)
  return err

if (!..)
  return err

*result = ..
return ok

is my prefered way too.

::: netwerk/test/TestUDPSocket.cpp
@@ +277,5 @@
> +    do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID, &rv);
> +  NS_ENSURE_SUCCESS(rv, -1);
> +  nsCOMPtr<nsIPrincipal> systemPrincipal;
> +  rv = secman->GetSystemPrincipal(getter_AddRefs(systemPrincipal));
> +  NS_ENSURE_SUCCESS(rv, -1);

some blank line spacing is always good to do..
Attachment #8570473 - Flags: review?(honzab.moz) → review+
Assignee

Updated

4 years ago
Keywords: checkin-needed
This fails to apply:

renamed 935838 -> bug_935838_v7.patch
applying bug_935838_v7.patch
patching file netwerk/test/unit/test_udp_multicast.js
Hunk #1 FAILED at 0
Hunk #2 FAILED at 22
2 out of 2 hunks FAILED -- saving rejects to file netwerk/test/unit/test_udp_multicast.js.rej

could you take a look, thanks!
Flags: needinfo?(dd.mozilla)
Keywords: checkin-needed
Assignee

Comment 34

4 years ago
Posted patch bug_935838_v7.patch (obsolete) — Splinter Review
I have re-based it.
Attachment #8573092 - Attachment is obsolete: true
Flags: needinfo?(dd.mozilla)
Attachment #8573948 - Flags: review+
Assignee

Updated

4 years ago
Keywords: checkin-needed
Assignee

Comment 37

4 years ago
Posted patch bug_935838_v8.patch (obsolete) — Splinter Review
Fix a check.
Attachment #8573948 - Attachment is obsolete: true
Attachment #8575853 - Flags: review?(honzab.moz)
Comment on attachment 8575853 [details] [diff] [review]
bug_935838_v8.patch

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

There is nothing more then a change in the security check code.  Please ask Jonas to check on this.

::: netwerk/test/unit/xpcshell.ini
@@ +108,5 @@
>  [test_bug412945.js]
>  [test_bug414122.js]
>  [test_bug427957.js]
>  [test_bug429347.js]
> +#[test_bug455311.js]

either remove the test or add a "skip-if" clause and a comment.
Attachment #8575853 - Flags: review?(jonas)
Attachment #8575853 - Flags: review?(honzab.moz)
Attachment #8575853 - Flags: review+
Assignee

Comment 40

4 years ago
(In reply to Honza Bambas (:mayhemer) from comment #39)
> Comment on attachment 8575853 [details] [diff] [review]
> bug_935838_v8.patch
> 
> Review of attachment 8575853 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> There is nothing more then a change in the security check code.  Please ask
> Jonas to check on this.
> 
> ::: netwerk/test/unit/xpcshell.ini
> @@ +108,5 @@
> >  [test_bug412945.js]
> >  [test_bug414122.js]
> >  [test_bug427957.js]
> >  [test_bug429347.js]
> > +#[test_bug455311.js]
> 
> either remove the test or add a "skip-if" clause and a comment.


sorry, I was jut testing something did not wanted to leave it in.
Assignee

Comment 41

4 years ago
Posted patch bug_935838_v8.patch (obsolete) — Splinter Review
Attachment #8575853 - Attachment is obsolete: true
Attachment #8575853 - Flags: review?(jonas)
Attachment #8582049 - Flags: review?(jonas)
Comment on attachment 8582049 [details] [diff] [review]
bug_935838_v8.patch

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

Looks great! Just a minor nit.

::: dom/network/UDPSocketParent.cpp
@@ +74,1 @@
>    uint32_t appId = nsIScriptSecurityManager::UNKNOWN_APP_ID;

Nit: You don't have to set appId to a default value here. XPCOM calling convention is that if a getter function returns a success value, it must always set the result out-param.

@@ +80,1 @@
>    return appId;

Nit: you're mixing styles here. At the beginning of this function you use early-return style. Here you are using "always exit at the end of the function" style.

I'd say just do:

uint32_t appId;
if (!mPrincipal || NS_FAILED(mPrincipal->GetAppId(&appId))) {
  <set appId or do a return>
}
return appId;
Attachment #8582049 - Flags: review?(jonas) → review+
Assignee

Comment 44

4 years ago
Attachment #8582049 - Attachment is obsolete: true
Attachment #8589924 - Flags: review+
Assignee

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3182e5961729
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S10 (17apr)
Bug 1148319 requires this patch to be available on 2.2r.
blocking-b2g: --- → 2.2r?
(In reply to Shian-Yow Wu [:swu] from comment #47)
> Bug 1148319 requires this patch to be available on 2.2r.

Hi Dragana,
Can you help on this?
Blocks: 1148319
blocking-b2g: 2.2r? → ---
feature-b2g: --- → 2.2r+
Flags: needinfo?(dd.mozilla)
Whiteboard: [red-tai][ETA=8/31]
Assignee

Comment 49

4 years ago
This is rebased version  for b2g 2.2r
Flags: needinfo?(dd.mozilla)
Assignee

Comment 50

4 years ago
Comment on attachment 8641044 [details] [diff] [review]
bug_935838_b2g_2.2r.patch rebased

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #):  needed for Bug 1148319
User impact if declined: 
Testing completed: 
Risk to taking this patch (and alternatives if risky): Path is since 38. Some checks on principals are added I do not know how risky this is. 
String or UUID changes made by this patch: nsIUDPSocketChild.idl, nsIUDPSocket.idl
Attachment #8641044 - Flags: approval‑mozilla‑b2g37_v2_2r?
Comment on attachment 8641044 [details] [diff] [review]
bug_935838_b2g_2.2r.patch rebased

B2G Release Management has decided to auto-approve any bugs marked feature-b2g:2.2r+.
Attachment #8641044 - Flags: approval‑mozilla‑b2g37_v2_2r?
Whiteboard: [red-tai][ETA=8/31] → [red-tai]
You need to log in before you can comment on or make changes to this bug.