I'll take this one :)

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?

(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?

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()

Honza, see previous comment.

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?

Hi Jonas, Do you know how to get appID in UDPSocket (see comment 8)

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

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?

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.

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.

If you do not have time, can you give it to someone who can review part with principals.

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

(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 \

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.

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

I have added IsInBrowser parameter. Honza, do you want to review it again?

Comment on attachment 8563273 [details] [diff] [review] bug_935838_v5.patch Review of attachment 8563273 [details] [diff] [review]: ----------------------------------------------------------------- gtg, thanks.

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.

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; ...

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..

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!

I have re-based it.

Backed out for making test_udpsocket.html perma-timeout on B2G mochitest runs. https://hg.mozilla.org/integration/mozilla-inbound/rev/468eb0cd72c5 https://treeherder.mozilla.org/logviewer.html#?job_id=7375195&repo=mozilla-inbound

Fix a check.

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.

(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.

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;

Bug 1148319 requires this patch to be available on 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?

This is rebased version for b2g 2.2r

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

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+.