Move nICEr stun local address discovery to an IPC call to enable future sandbox restrictions on content process

RESOLVED FIXED in Firefox 55

Status

()

enhancement
P1
normal
Rank:
19
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mjf, Assigned: mjf)

Tracking

(Blocks 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(4 attachments)

Assignee

Description

2 years ago
No description provided.
Assignee

Updated

2 years ago
Assignee: nobody → mfroman
Rank: 19
Priority: -- → P1
Assignee

Comment 1

2 years ago
Jason, I'm looking for feedback on this new IPC protocol for discovering local stun addrs.  In previous conversations with Patrick he suggested that our new code live under media/mtransport since that will be the primary (probably only) use case.  I kept it as managed by PNecko since it is a networking related IPC.  I appreciate any feedback you have on this.
Attachment #8844965 - Flags: feedback?(jduell.mcbugs)

Comment 2

2 years ago
Comment on attachment 8844965 [details]
Proposed ipdl file for stun addr IPC call managed by PNecko

>include protocol PNecko;
>
>using struct StunAddr from "mozilla/net/StunAddr.h";
>using StunAddrArray from "mozilla/net/PStunAddrsParams.h";
>
>include "mozilla/net/StunAddrMessageUtils.h";
>
>namespace mozilla {
>namespace net {
>
>async protocol PStunAddrsRequest
>{
>  manager PNecko;

As far as I know it's fine for this to be managed by PNecko.  


>parent:
>  // Note: channels are opened during construction, so no open method here:
>  // see PNecko.ipdl

Is this comment still valid or is it a vestige of cut-and-paste?


>  async GetStunAddrs(bool allowLoopback,
>                     bool tcpEnabled,
>                     bool allowLinkLocal);
>
>  async __delete__();
>
>child:
>  async OnStunAddrsAvailable(StunAddrArray addrs);
>};
>
>} // namespace net
>} // namespace mozilla
Attachment #8844965 - Flags: feedback?(jduell.mcbugs) → feedback+
Assignee

Comment 3

2 years ago
Thanks, Jason.  Yes, good catch on the comment.  I've removed it.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 7

2 years ago
mozreview-review
Comment on attachment 8848206 [details]
Bug 1345511 - pt 1 - nICEr changes to support stun addr gathering from main process.

https://reviewboard.mozilla.org/r/121142/#review123120

::: media/mtransport/third_party/nICEr/src/ice/ice_ctx.c:700
(Diff revision 1)
> -static int nr_ice_get_local_addresses(nr_ice_ctx *ctx)
> +int nr_ice_get_local_addresses(nr_ice_ctx *ctx,
> +                               nr_local_addr* stun_addrs, int stun_addr_ct)

I think this would be easier to follow if this function were renamed to nr_ice_set_local_addresses, and make it barf if none are supplied (and maybe make it barf if ctx->local_addrs is already set?), and have it not call nr_stun_find_local_addresses.

Then, down in nr_ice_gather, we check ctx->local_addrs, and if they are not set, it calls nr_stun_find_local_addresses, and then calls nr_ice_set_local_addresses with the result.
Attachment #8848206 - Flags: review?(docfaraday)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 11

2 years ago
mozreview-review
Comment on attachment 8848207 [details]
Bug 1345511 - pt 2 - add IPC mechanism for getting stun addrs on main process.

https://reviewboard.mozilla.org/r/121144/#review123442

Pretty close, but I really think we should avoid serializing wild pointers. Let's serialize nullptr instead, just to cover our butts. Some other nits too.

::: media/mtransport/ipc/NrIceStunAddrMessageUtils.h:18
(Diff revision 2)
> +    aMsg->WriteBytes((void*)aParam.localAddr(),
> +                     aParam.SerializationBufferSize());

Let's prevent the localAddr_->addr->addr wild pointer from being serialized here somehow. Make a function that returns a copy with it nulled out or something. I don't think an extra copy is going to hurt us.

::: media/mtransport/ipc/StunAddrsRequestParent.cpp:7
(Diff revision 2)
> +#include "../runnable_utils.h"
> +#include "nsNetUtil.h"
> +
> +#include "../nricectx.h"
> +#include "../nricemediastream.h" // needed only for including nricectx.h
> +#include "../nricestunaddr.h"

Let's use mtransport/ as the prefix on these.

::: media/mtransport/ipc/StunAddrsRequestParent.cpp:29
(Diff revision 2)
> +  // InitializeGlobals is done on the main thread in the content process, so
> +  // we'll do the same here since IPC happens on main thread.
> +  NrIceCtx::InitializeGlobals(allowLoopback, tcpEnabled, allowLinkLocal);

Oof. That's gross. I think the problem here is we are handling policy on addresses in two places:

In nr_stun_get_addrs we are filtering out loopback, link local, and we are also removing duplicates. This function is running on the parent process.

In nr_ice_set_local_addresses, we are handling things like default route only, forcing specific interfaces, and prioritization. This function is running on the content process.

I think the design would be cleaner if the policy logic in nr_stun_get_addrs were moved to nr_ice_set_local_addresses. That would mean the parent process wouldn't need to be configured with these policy bits.

Of course, I still think we'd be stuck with doing global init, so we would have the logging system on the parent process.

::: media/mtransport/nricestunaddr.h:16
(Diff revision 2)
> +
> +namespace mozilla {
> +
> +class NrIceStunAddr {
> +public:
> +  NrIceStunAddr(); // needed for IPC serialization

Needed for deserialization, right?

::: media/mtransport/nricestunaddr.h:22
(Diff revision 2)
> +  nr_local_addr* localAddr() { return localAddr_; }
> +  const nr_local_addr* localAddr() const { return localAddr_; }

Return references?

::: media/mtransport/nricestunaddr.h:31
(Diff revision 2)
> +  // in media/mtransport/ipc/NrIceStunAddrMessagUtils.h
> +  size_t SerializationBufferSize() const;
> +  nsresult Deserialize(const char* buffer, size_t buffer_size);
> +
> +private:
> +  nr_local_addr* localAddr_;

Any particular reason this can't just be

nr_local_addr localAddr_;

::: media/mtransport/nricestunaddr.cpp:60
(Diff revision 2)
> +  if (buffer_size != sizeof(nr_local_addr)) {
> +    MOZ_MTLOG(ML_ERROR, "Failed trying to deserialize NrIceStunAddr, "
> +                        "input buffer length (" << buffer_size <<
> +                        ") does not match required length ("
> +                        << sizeof(nr_local_addr) << ")");
> +    return NS_ERROR_FAILURE;

A MOZ_ASSERT would probably be appropriate here?

::: media/mtransport/nricestunaddr.cpp:66
(Diff revision 2)
> +  }
> +
> +  nr_local_addr* from_addr =
> +      const_cast<nr_local_addr*>((const nr_local_addr*)buffer);
> +
> +  if (nr_local_addr_copy(localAddr_, from_addr)) {

You probably need a comment here about from_addr->addr->addr being invalid, and nr_local_addr_copy fixing it.

::: media/mtransport/nricestunaddr.cpp:69
(Diff revision 2)
> +      const_cast<nr_local_addr*>((const nr_local_addr*)buffer);
> +
> +  if (nr_local_addr_copy(localAddr_, from_addr)) {
> +    MOZ_MTLOG(ML_ERROR, "Failed trying to deserialize NrIceStunAddr, "
> +                        "could not copy nr_local_addr.");
> +    return NS_ERROR_FAILURE;

Probably should assert here too.
Attachment #8848207 - Flags: review?(docfaraday) → review-

Comment 12

2 years ago
mozreview-review
Comment on attachment 8848206 [details]
Bug 1345511 - pt 1 - nICEr changes to support stun addr gathering from main process.

https://reviewboard.mozilla.org/r/121142/#review123454

This looks pretty good to me. I have a comment on part 2 that may change this patch though, so reserving judgement until then.

::: media/mtransport/third_party/nICEr/src/ice/ice_ctx.c:726
(Diff revision 2)
> +//     if (!stun_addr_ct) {
> +//       /* First, gather all the local addresses we have */
> +//       if((r=nr_stun_find_local_addresses(local_addrs,MAXADDRS,&addr_ct))) {
> +//         r_log(LOG_ICE,LOG_ERR,"ICE(%s): unable to gather local addresses, trying default route",ctx->label);
> +//       }
> +//     } else {
> +//       addr_ct = MIN(stun_addr_ct, MAXADDRS);
> +//       r_log(LOG_ICE, LOG_DEBUG, "ICE(%s): copy %d pre-fetched stun addrs", ctx->label, addr_ct);
> +//       for (i=0; i<addr_ct; ++i) {
> +//         if (r=nr_local_addr_copy(&local_addrs[i], &stun_addrs[i])) {
> +//           ABORT(r);
> +//         }
> +//       }
> +//     }

Comment needs to go.

::: media/mtransport/third_party/nICEr/src/ice/ice_ctx.c:817
(Diff revision 2)
> +//     if ((r=nr_ice_get_local_addresses(ctx, NULL, 0)))
> +//       ABORT(r);

Remove comment.
Attachment #8848206 - Flags: review?(docfaraday)

Comment 13

2 years ago
mozreview-review
Comment on attachment 8848208 [details]
Bug 1345511 - pt 3 - start using IPC call for stun addrs in PCMedia.

https://reviewboard.mozilla.org/r/121146/#review123458

I'm mainly concerned about the RefPtr issue, but there are some nits too.

::: media/mtransport/nricectx.h:238
(Diff revision 2)
> +  // static GetStunAddrs for use in main process to support
> +  // future sandboxing restrictions

The word "future" here is going to be out of date, and nobody will fix it (probably). Also, s/main/parent/.

::: media/mtransport/nricectx.cpp:526
(Diff revision 2)
> +nsTArray<NrIceStunAddr>
> +NrIceCtx::GetStunAddrs()

Maybe add a /* static */ comment before the definition.

::: media/mtransport/nricectx.cpp:550
(Diff revision 2)
> +  nr_local_addr* local_addrs;
> +  local_addrs = new nr_local_addr[addrs.Length()];

Pretty sure you can use a stack array here, and avoid using the heap.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.h:463
(Diff revision 2)
> +    explicit StunAddrsHandler(PeerConnectionMedia *pcm) :
> +      pcm_(pcm) {}
> +    void OnStunAddrsAvailable(
> +        const mozilla::net::NrIceStunAddrArray& addrs) override;
> +   private:
> +    PeerConnectionMedia* pcm_;

Hmm. Why are we not using a RefPtr like ProtocolProxyQueryHandler?
Attachment #8848208 - Flags: review?(docfaraday) → review-

Comment 14

2 years ago
Comment on attachment 8848207 [details]
Bug 1345511 - pt 2 - add IPC mechanism for getting stun addrs on main process.

The PNecko/Necko{Child|Parent} parts looks fine.
Attachment #8848207 - Flags: review?(jduell.mcbugs) → feedback+

Comment 15

2 years ago
mozreview-review
Comment on attachment 8848208 [details]
Bug 1345511 - pt 3 - start using IPC call for stun addrs in PCMedia.

https://reviewboard.mozilla.org/r/121146/#review123686

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:310
(Diff revision 2)
> +                                  new StunAddrsHandler(this));
> +    mStunAddrsRequest->SendGetStunAddrs();
> +  } else {
> +    // No content process, so don't need to hold up the ice event queue
> +    // until completion of stun address discovery. We can let the
> +    // discovery of stun addresses happen normally.

"normally" suggest the other way is wrong. How about "discovery of stun addresses happen without IPC in the same process"?
Assignee

Comment 16

2 years ago
mozreview-review-reply
Comment on attachment 8848206 [details]
Bug 1345511 - pt 1 - nICEr changes to support stun addr gathering from main process.

https://reviewboard.mozilla.org/r/121142/#review123454

> Comment needs to go.

Gah - sorry!  Too much hurrying.
Assignee

Comment 17

2 years ago
mozreview-review-reply
Comment on attachment 8848207 [details]
Bug 1345511 - pt 2 - add IPC mechanism for getting stun addrs on main process.

https://reviewboard.mozilla.org/r/121144/#review123442

> Any particular reason this can't just be
> 
> nr_local_addr localAddr_;

Yes, because the IPC-related code doesn't have access to the actual definition of nr_local_addr.  There were so many hoops to jump through there to export the headers from down in nICEr and have it build on all the platforms (windows in particular caused much pain).  I'm open to suggestions, but this seemed like the easiest way to keep nICEr details encapsulated.
Assignee

Comment 18

2 years ago
mozreview-review-reply
Comment on attachment 8848208 [details]
Bug 1345511 - pt 3 - start using IPC call for stun addrs in PCMedia.

https://reviewboard.mozilla.org/r/121146/#review123458

> Pretty sure you can use a stack array here, and avoid using the heap.

Windows builds complained on try when I attempted that before.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 22

2 years ago
mozreview-review
Comment on attachment 8848206 [details]
Bug 1345511 - pt 1 - nICEr changes to support stun addr gathering from main process.

https://reviewboard.mozilla.org/r/121142/#review124680
Attachment #8848206 - Flags: review?(docfaraday) → review+

Comment 23

2 years ago
mozreview-review
Comment on attachment 8848207 [details]
Bug 1345511 - pt 2 - add IPC mechanism for getting stun addrs on main process.

https://reviewboard.mozilla.org/r/121144/#review124686

::: media/mtransport/ipc/NrIceStunAddrMessageUtils.h:19
(Diff revisions 2 - 3)
>  struct ParamTraits<mozilla::NrIceStunAddr>
>  {
>    static void Write(Message* aMsg, const mozilla::NrIceStunAddr &aParam)
>    {
> -    aMsg->WriteBytes((void*)aParam.localAddr(),
> -                     aParam.SerializationBufferSize());
> +    const size_t bufSize = aParam.SerializationBufferSize();
> +    char* buffer = new char[bufSize];

We need to delete[] after, right?
Attachment #8848207 - Flags: review?(docfaraday) → review+

Comment 24

2 years ago
mozreview-review
Comment on attachment 8848208 [details]
Bug 1345511 - pt 3 - start using IPC call for stun addrs in PCMedia.

https://reviewboard.mozilla.org/r/121146/#review124690
Attachment #8848208 - Flags: review?(docfaraday) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Keywords: checkin-needed

Comment 29

2 years ago
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e0a12ec4de38
pt 1 - nICEr changes to support stun addr gathering from main process. r=bwc
https://hg.mozilla.org/integration/autoland/rev/af9901df3d48
pt 2 - add IPC mechanism for getting stun addrs on main process. r=bwc
https://hg.mozilla.org/integration/autoland/rev/6fe853638e4d
pt 3 - start using IPC call for stun addrs in PCMedia. r=bwc
Keywords: checkin-needed

Comment 30

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e0a12ec4de38
https://hg.mozilla.org/mozilla-central/rev/af9901df3d48
https://hg.mozilla.org/mozilla-central/rev/6fe853638e4d
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1350055

Updated

2 years ago
Attachment #8848207 - Flags: review?(jduell.mcbugs) → review+
Depends on: 1350568
Duplicate of this bug: 1322506

Updated

2 years ago
Blocks: 1375122
You need to log in before you can comment on or make changes to this bug.