Re: [secdir] Secdir last call review of draft-ietf-ice-rfc5245bis-16

[Christer Holmberg <[e-mail]>]

Hi Stephen,

Thank You for your review! Please see inline.

>Reviewer: Stephen Farrell
>Review result: Has Issues
>
>(1) I think a bit more work on the security and privacy issues around ICE would improve >the document, and hopefully, ICE implementations.
>
>- There has been (IMO somewhat justified) concern [1] about exposing all possible >addresses, for example where one is from a VPN intended to preserve privacy, say if a >user is aiming to circumvent local censorship or surveillance.  5.1.1.1 has a SHOULD that, >as I read it, says to always include such addresses.  
>
>   [1] https://www.w3.org/wiki/Privacy/IPAddresses
>
>(Note: I'm not claiming [1] is authoritative, it's just a page I found that describes the issue reasonably well.)
>
>There is a bit of text about this (1st para in section 19), but IMO it's a bit weak. 
>
>- Did the WG consider REQUIRING or RECOMMENDING agent implementations
>  provide some local interface that allows the host or an application to say "Don't use 
> <this> interface to generate any candidates until I tell you otherwise"?  That might be 
> better than having VPN operators recommending to turn off WebRTC entirely for 
> example. (Note: this is my main comment, the rest are mostly suggested things to think 
> about, if you've not already.)
>
> - Even if the text isn't changed, a reference to [2] would I think be
>  useful. (Assuming [2] is still being worked on in rtcweb, though mind you, I don't like 
> how [2] refers to "consent" as I doubt a random person can really provide meaningful 
> consent for things like this.)
>
>   [2] https://tools.ietf.org/html/draft-ietf-rtcweb-ip-handling-04

I would not want to REQUIRE implementing a local interface for controlling the candidates, as ICE is also used by different types of network entities (gateways etc). But, I am ok to RECOMMEND.

What about:

OLD:

   "Individual implementations may also have implementation-specific rules for controlling which
   addresses are revealed."

NEW:

   "Individual implementations may also have implementation-specific rules for controlling which
   addresses are revealed. It is RECOMMENDED that applications interacting with human users provide 
   a user interfaces that allows the users to control which interfaces are used to generate candidates, or 
   when to use candidates generated for the interfaces.

   [REF-to- draft-ietf-rtcweb-ip-handling] provide additional information and
   Requirements regarding the handing of IP addresses for WebRTC applications."

> - Separately [1] also describes another potential privacy issue with
>  STUN urls. I think it'd be worthwhile noting such issues that have been found with deployments of 
> ICE here, as any protocol using ICE and accepting non locally configured STUN/TURN URLs may have 
> issues like that. For example, a recent paper [3] also describes some attacks (in section 4.2 mostly) that 
> could (I guess) be mounted against any ICE agent and not only WebRTC clients. That'd be worth a reference 
> and maybe thinking about which attacks are generic ICE issues and don't only affect WebRTC.
>
>   [3] https://doi.org/10.1145/3019612.3019844

I am struggling to figure out exactly what to do here... I really hope we don't have to perfom a study on [3], and see what is general, and what is WebRTC-specific. Are we even allowed to reference [3], as it's not freely available?

Some help and guidance would be appreciated :)

---

> - I also wondered if I can fingerprint a host based on how it does ICE?  
>  I  suspect that might work, but haven't tried to figure out details.
>
> - Could I probe an internal network based on feeding it candidates to
>  check? E.g. checking timing of reactions.  If I could that seems noteworthy.

Something like?

"Based on the types of candidates provided by the peer, and the results of the connectivity tests performed 
Against those candidates, an agent might be able to determine characteristics of the peer network."

---

> (2) 5.3: "...MUST contain ... <foo> bits of randomness" Don't you need to say when these values MUST be different and when they're ok to be re-used? I forget if STUN/TURN do that. 

Section 7.2.2 says:

"A connectivity check Binding request MUST utilize the STUN short-term credential mechanism."

RFC 5389 (STUN) says:

      "Short-Term Credential:  A temporary username and associated password
      that represent a shared secret between client and server.  Short-
      term credentials are obtained through some kind of protocol
      mechanism between the client and server, preceding the STUN
      exchange.  A short-term credential has an explicit temporal scope,
      which may be based on a specific amount of time (such as 5
      minutes) or on an event (such as termination of a SIP dialog).
      The specific scope of a short-term credential is defined by the
      application usage."

If anything extra is needed, I think that should be done as an update to STUN.

 > (Also - the phrasing isn't that great - how do I "contain" randomness? But that's just a nit.)

Do you have a suggestion for a better word?

========================

Nits:

> I took a look at the diff [4] between this and 5245, but that wasn't really that useful, so this review is based on
> a read of the draft without comparing it to 5245. Apologies if that causes me to comment on text that's
> unchanged - in such cases, I think it's fine to not heed those particular comments. 
>
>   [4] https://tools.ietf.org/rfcdiff?url1=rfc5245&url2=draft-ietf-ice-rfc5245bis-16.txt
>
> - 2.1: Why "only UDP specified here"? Does that include QUIC which is
>   UDP-based, but not UDP? I assume so.

There is a separate specification, RFC 6544, for TCP based candidates.

Regarding QUIC, I don't know. QUIC wasn't discussed during the work, but how/if the procedures also apply to QUIC without any modifications etc I don't know.

> - 2.3: how the controlling/controlled stuff works isn't clear from
>  this, nor even (as I was reading it) in section 7.3.1.1 - I wasn't clear how the tie-breaker value is initialised until I got to 16.1.
> In any case I think a bit more explanatory text in 2.3 might be good.
> (But if implementers haven't had a problem with this, maybe it's just me.)

In general, one of the tasks of the bis work was to *simplify* section 2, because the previous version was too detailed for non-implementers who just want to figure out what ICE is all about (in order to  actually implement ICE, reading section 2 is obviously not enough). For that reason, my opinion was that section 2 doesn't really need to talk about role conflicts and tie-breaker values, as it's not essential for a high-level description of ICE.

---

> - 5.2: "The procedures in this section is common across the initiating
>  and responding agents." s/is/are/ I guess?

Yes. I will fix as suggested.

---

> - 6.1.1: What is "3PCC"? That note could maybe also do with a
>  reference, as I at least don't get what you're trying to tell me. Ah
> - it's 3rd party call control (mentioned in 7.3.1.1).

I can use "3PCC (3rd Party Call Control)" in section 6.1.1.

As the spec is protocol independent, I don't think there are any references that can be added.

---

> - 16.1: For a given ICE session, are the random numbers in the
>  ICE-CONTROLLED and ICE-CONTROLLING attributes sent by the same agent supposed to have
> the same value? I wasn't clear. If they are the same, I understand how tie-breaking happens. 
> If they can differ, I'm not sure. (But I didn't go back and re-read 7.3.1.1, so it may be ok either 
> way:-) 

It is covered in the 2nd paragraph of section 7.2.5.1, which says that the agent MAY change the value when it switches role.

> In any case saying "referred to as the tie-breaker value" twice seems odd. 

I'll remove it from the ICE-CONTROLLING definition.

---

> - 16.1: (mega-mega-nit:-) the fact that tie-breaker is split over a
>  line-breaker here is why I didn't find this when I was reading
> 2.3/7.3.1.1 - maybe tweak the words so a grep will find it?)

Good catch. I'll fix it.

It would be really cool if search functions could discard line-breaks when searching for strings.

---

> - 17: "looking to deploy ICE" is a bit confusing - do you mean
>  "looking to be nice to ICE"? My assumption is that endpoints deploy ICE agents but 
> network operators don't, though they might deploy STUN/TURN servers I guess. But 
> maybe I'm thinking too much about WebRTC there.

Perhaps something like:

   "This section discusses issues relevant to operators operating networks where ICE will be used by endpoints."

---

> - 18: I wondered if this is still needed, given that 5245 already said
>  it (I didn't check if the text has been updated). Saying this once would seem sufficient 
> to me anyway. I guess leaving it in is the easier path.

I'll leave it :)

---

> - 19: s/DNS-SEC/DNSSEC/ would be more common

I will fix as suggested.

---

> - 19.4.1: 1st sentence if missing a "T" - s/he/The/

I will fix as suggested.

---

Regards,

Christer