Subject: Secdir last call review of draft-ietf-ice-rfc5245bis-16

From: Stephen Farrell <[email-masked]>
Date: Fri, 26 Jan 2018 08:49:00 -0800

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

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

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

Nits:

I took at 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.

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

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

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

- 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:-) In any case saying "referred to as the tie-breaker
value" twice seems odd. 

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

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

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

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

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

---

From: Christer Holmberg <[email-masked]>
Date: Sun, 28 Jan 2018 12:57:24 +0000

Hi Stephen,

Thank You for your review! Please see inline.

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

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

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

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

---

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

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.

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

---

>  and responding agents." s/is/are/ I guess?

Yes. I will fix as suggested.

---

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

---

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

---

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

---

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

---

>  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

---

From: Stephen Farrell <[email-masked]>
Date: Tue, 30 Jan 2018 11:18:11 +0000

Hiya,

Sorry for the slowish response, more inline...

On 28/01/18 12:57, Christer Holmberg wrote:
>> 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.
>> 
>> (Note: I'm not claiming [1] is authoritative, it's just a page I
>> found that describes the issue reasonably well.)
>> 
>> - 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.)
>> 
> 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.

Fair enough.

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

That's a fine improvement from my POV, esp if folks will
really provide such interfaces. I'm not sure "interacting
with humans" is quite right though, maybe it'd be better
to add a reference to the problem (via [1] or [rtcweb-ip])
and to then say "Implementations where such issues can
arise are RECOMMENDED to..." I'd also suggest maybe
s/user interface/interface/ there, as e.g. a VPN client
might want to use an API provided by an ICE implementation
so the UI might be part of the VPN client and not the ICE
implementation. So, overall I'd suggest something like
this:

"Individual implementations may also have implementation-specific
rules for controlling which addresses are revealed. For example,
[REF-to- draft-ietf-rtcweb-ip-handling] provides additional
information about the privacy aspects of revealing IP addresses
via ICE for WebRTC applications. ICE implementations where such
issues can arise are RECOMMENDED to provide a programmatic or
user interface that provides control over which network interfaces
are used to generate candidates."

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

Sure, we can reference academic papers behind paywalls, and
there's often another version available that's not. (Not sure
in this case.)

> Some help and guidance would be appreciated :)

I guess about as much as can be done with this -bis effort is
to note where any leakages have been found. There's been some
recent traffic on the W3C WebRTC list about that I think, (in
the thread about CSP iirc) so maybe a trawl of that thread
will suggest some text?

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

Maybe:

"Based on the types of candidates provided by the peer, and the
results of the connectivity tests performed against those candidates,
the peer might be able to determine characteristics of the local
network, e.g. if different timings are apparent to the peer. In
the limit the peer might be able to probe the local network."

That said - I'm not claiming this is possible for-sure, I just
bet it would be:-)

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

Not sure I agree - maybe I wasn't clear in my comment though,
so let me try rephrase it.

STUN doesn't define the duration for which short-term creds
are OK to be used. ICE does define a session concept therefore
ICE ought to say if short term STUN creds MUST/MUST-NOT/MAY
(or whatever) be re-used in different ICE sessions. Even
within an ICE session I guess in theory one could require
different short-term STUN credentials every single time, or
not. So I do think there's something to be said here.

> Do you have a suggestion for a better word?

I think what you want is to say "unguessable, with at
least 128 bits of random number generator output used
to generate each" or something like that? It's a hard
one to get right, but even what you have isn't that
bad (I did say this was a nit:-)

The rest below are all fine,
Thanks,
S.

> 
> ========================
> 
> 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
>>
>>
>> 
> 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.
> 
>> 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?
> 
>> 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).
> 
>> 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.
> 
>> - 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 
> "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 :)
> 
> ---
> 

-- 
PGP key change time for me.
New-ID 7B172BEA; old-ID 805F8DA2 expires Jan 24 2018.
NewWithOld sigs in keyservers.
Sorry if that mucks something up;-)

---

From: Christer Holmberg <[email-masked]>
Date: Tue, 30 Jan 2018 13:53:47 +0000

Hi,

>are used to generate candidates."

Looks good :)

---

>the thread about CSP iirc) so maybe a trawl of that thread

I did follow that discussion. I will take a second look.

---

>That said - I'm not claiming this is possible for-sure, I just

The text looks good.

---

>not. So I do think there's something to be said here.

Section 9 says that an agent must create a new username fragment and
password for every new ICE session:

     "To restart ICE, an agent MUST change both the password and the
      username fragment for the data stream(s) being restarted.²

Since I also uses the username to identity the ICE session, the values
cannot be changed within a session.

There is no text on when a value can be re-used - apart from the fact that
they cannot be re-used when an ICE restart takes place.

My assumption has been that the randomness criteria are assumed to ensure
that two agents don¹t choose the same values for the same session, and
that values are unlikely to be re-used within a short period of time.

We could say that agents shall verify that the same value is not re-used
as long as packets from a previous ICE session may still be present in the
network. But, that is not really security related, but more to make sure
ICE works.

---

>bad (I did say this was a nit:-)

What about?

     "Username Fragment and Password:  Values used to perform connectivity
      checks. The values MUST be unguessable, with at least 128 bits of
random 
      number generator output used to generate the password, and at least
24 
      bits output to generate the username fragment."

Thanks! :)

Regards,

Christer

---

From: Stephen Farrell <[email-masked]>
Date: Tue, 30 Jan 2018 14:32:29 +0000

Hiya,

Those look close-enough or fine to me. For the
recent WebRTC thread, if you find something, that'll
be a good thing. If not, and nobody suggests some
text, then I guess at least we did try:-) And FWIW I'm
fine with that, as I'm not up to speed on non-WebRTC
uses of ICE, so don't know if the same leakage issues
can arise in reality, outside of WebRTC.

Thanks,
S.

---

From: Christer Holmberg <[email-masked]>
Date: Wed, 31 Jan 2018 14:39:12 +0000

Hi,

I’ve updated the pull request with changes based on Stephen’s sec-dir
review.

I am still looking whether there is something that could be added
regarding WebRTC IP leakage.

https://github.com/ice-wg/rfc5245bis/pull/57

I have not yet committed any changes based on Magnus’ trv-dir review.

Regards,

Christer

On 30/01/18 16:32, "Stephen Farrell" <[email-masked]> wrote:

>>>>>https://tools.ietf.org/rfcdiff?url1=rfc5245&url2=draft-ietf-ice-rfc524
>>>>>5b
>>>>> is-16.txt
>>>>>
>>>>>
>>>>>