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" <stephen.farrell@xxxxxxxxx> wrote: > >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-rfc524 >>>>>5b >>>>> 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;-)