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;-)
Attachment:
0x7B172BEA.asc
Description: application/pgp-keys
Attachment:
signature.asc
Description: OpenPGP digital signature