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

[Stephen Farrell <[e-mail]>]

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.

On 30/01/18 13:53, Christer Holmberg wrote:
> Hi,
> 
>>>> 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.
>>
>> 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."
> 
> Looks good :)
> 
> ---
> 
>>>
>>>> - 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?
>>
>> 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 did follow that discussion. I will take a second look.
> 
> ---
> 
>>>> - 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:-)
> 
> The text looks good.
> 
> ---
> 
>>>> (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.
>>
>> 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.
> 
> 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.
> 
> ---
> 
>>>
>>>> (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?
>>
>> 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:-)
> 
> 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
> 
> 
>>>
>>> ========================
>>>
>>> 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-rfc5245b
>>>> is-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
>>>
>>
>> -- 
>> 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;-)
> 
> 

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