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