[rtcweb] AD Review: draft-ietf-rtcweb-security-arch

[Adam Roach <[e-mail]>]

[sorry for the re-send to the authors; I forgot to copy the WG on my 
first email]

This is my AD review for draft-ietf-rtcweb-security-arch. Several of the
comments below are marked [DISCUSS], which indicates issues that either 
require
discussion among the working group participants to resolve an ambiguity, or
issues that I would like to see addressed before asking for IETF review. I
think all of these should be easy to fix. I expect we'll talk about 
several of
them during the RTCWEB meeting next week.

---------------------------------------------------------------------------

[DISCUSS]

Per the discussion around Cluster 238 dependencies, please reference RFC 
8445
instead of RFC 5245.

---------------------------------------------------------------------------

Please fix the following nits:

   == The document seems to contain a disclaimer for pre-RFC5378 work, 
but was
      first submitted on or after 10 November 2008.  The disclaimer is 
usually
      necessary only for documents that revise or obsolete older RFCs, 
and that
      take significant amounts of text from those RFCs.  If you can 
contact all
      authors of the source material and they are willing to grant the BCP78
      rights to the IETF Trust, you can and should remove the disclaimer.
      Otherwise, the disclaimer is needed and you can ignore this comment.
      (See the Legal Provisions document at
      https://trustee.ietf.org/license-info for more information.)

   == Unused Reference: 'RFC5234' is defined on line 1799, but no explicit
      reference was found in the text

(This latter nit should probably be fixed by referencing RFC 5234 from §5)

---------------------------------------------------------------------------

I note that the early security review had some DISCUSS-level concerns about
the language in section 4.1. I don't see any discussion of this on the 
mailing
list, and the cited language is still in the document. See
https://mailarchive.ietf.org/arch/msg/rtcweb/aqSxsEtE-f4tVlT8je5HxwjOeho

Please ensure this review is addressed along with any other IETF last-call
comments.

---------------------------------------------------------------------------

§3.1:

 >  Note that merely being authenticated does not make these entities
 >  trusted.  For instance, just because we can verify that
 >  https://www.evil.org/

Note that evil.org has been delegated by ICANN and appears to be in
active use. Please consider "https://www.evil.example/" or
"https://evil.example.org/" instead.

---------------------------------------------------------------------------

§4:

 >  of signaling.  Specifically, Alice and Bob have relationships with
 >  some Identity Provider (IdP) that supports a protocol (such as OpenID
 >  Connect) that can be used to demonstrate their identity to other

Consider adding a citation for Open ID Connect.

---------------------------------------------------------------------------

§4.1:

 >  In this example, we show Alice and Bob using a separate
 >  identity service, though the identity service may be the same entity
 >  as the calling service or there may be no identity service at all.

It seems to me that there should be some discussion here about the 
diminished
guarantees of non-interceptability of media when the identity service 
and the
calling service are under the same domain of control.

---------------------------------------------------------------------------

§4.1:

 >  Once the PeerConnection is created, the calling service JS needs to
 >  set up some media.  Because this is an audio/video call, it creates a
 >  MediaStream with two MediaStreamTracks

Consider adding a citation to JSEP for the formal definition of these 
terms when
they first appear.

---------------------------------------------------------------------------

§4.1:

 >  In the current W3C API, once some streams have been added, Alice's
 >  browser + JS generates a signaling message [I-D.ietf-rtcweb-jsep]
 >  containing:
 >
 >  o  Media channel information
 >
 >  o  Interactive Connectivity Establishment (ICE) [RFC5245] candidates

I understand that this is intended as a high-level summary of behavior, 
but this
second bullet is pretty uncommon in the general case due to the use of 
Trickle
ICE. I suggest removing or caveating it.


---------------------------------------------------------------------------

§4.1:

 >  [RFC5246].  The signaling server processes the message from Alice's
 >  browser, determines that this is a call to Bob and sends a signaling
 >  message to Bob's browser (again, the format is currently undefined).

The phrasing here ("currently undefined") makes this sound like a to-do 
of sorts
for the working group. I believe it would be clearer to describe it as
"implementation-defined" or something similar to that.

Nit: "...a call to Bob, and sends..."

---------------------------------------------------------------------------

§4.1:

 >  This allows
 >  the browser to display a trusted element in the browser chrome
 >  indicating that a call is coming in from Alice.

"browser chrome" is a very specific term of art that readers of this 
document
may be unfamiliar with. Please either define it or rephrase along the 
lines of
"...trusted element in a privileged part of the browser's user interface..."

---------------------------------------------------------------------------

§4.3:

 >  If Alice and Bob authenticated via their IdPs,
 >  then they also know that the signaling service is not mounting a man-
 >  in-the-middle attack on their traffic.

It seems that this should be caveated to point out that an IdP that colludes
with the signaling server could enable the signaling service to mount an
active MITM attack on their traffic.

---------------------------------------------------------------------------

§5.1.2:

 >  When an answerer receives an offer that contains an 'identity'
 >  attribute, the answerer can use the the attribute information to

Nit: s/the the/the/

---------------------------------------------------------------------------

[DISCUSS]

§5.1.4:

 >  Offer processing of an 'identity' attribute is the same as that
 >  described in Section 5.1.2.

Many of the protocols that are used inter-domain don't have an easy way to
"reject" an answer (and it's not a well-defined operation in RFC 3264 
either).
It would be far less ambiguous to instead be explicit that the session is
terminated; e.g.:

    When an offerer receives an answer that contains an 'identity'
    attribute, the offerer can use the attribute information to
    contact the IdP, and verify the identity of the peer.  If the
    identity verification fails, the offerer MUST terminate the
    session.

---------------------------------------------------------------------------

[DISCUSS]

§5.1.5:

This section omits any discussion of what the clients are supposed to do 
if an
updated fingerprint fails validation. Presumably, if the fingerprint in 
an offer
fails, the answerer can reject it, and the offerer can roll back the 
session. If
the fingerprint in an answer fails, I think the only recovery is to 
terminate
the session. It's not clear whether we want these to be symmetrical (both
terminate the session), but I think the procedure needs to be clearly 
spelled
out regardless of whether that's true (or, if we want to leave it up to the
implementation whether to reject the offer or terminate the session, we 
should
say so explicitly).

---------------------------------------------------------------------------

[DISCUSS]

§6.2 seems somewhat schizophrenic on how insecure HTTP is handled. For 
example:

 >  Because HTTP origins cannot be securely established against network
 >  attackers, implementations MUST NOT allow the setting of permanent
 >  access permissions for HTTP origins.  Implementations MUST refuse all
 >  permissions grants for HTTP origins.

My reading of this is that one sentence places restrictions on how 
devices can
be accessed from insecure pages, while the next one forbids such access
altogether. If that's not the intention of these respective sentences, then
they probably need some clarification.

---------------------------------------------------------------------------

§6.2:

 >     this may not be necessary in systems that are non-windows-based
 >     but that have good notifications support, such as phones.]

Presumably, this means "non-window-based" rather than 
"non-Windows-based". The
current phrasing implies the second, as the adjectival form of nouns are
typically singular.

---------------------------------------------------------------------------

§6.3:

 >  While continuing consent is required, the ICE [RFC5245]; Section 10

To make sure it doesn't get overlooked, I want to highlight that this 
changes to
"[RFC8445] Section 11".

---------------------------------------------------------------------------

§6.3:

 >  The current WG consensus is to use ICE
 >  Binding Requests for continuing consent freshness.

This phrasing won't age well.

---------------------------------------------------------------------------

§6.3:

 >  A separate document will profile
 >  the ICE timers to be used; see [RFC7675].

The future tense here is confusing.

---------------------------------------------------------------------------

§6.4:

 >     the user to optimize performance by reconfiguring to allow non-
 >     turn candidates during an active call if the user decides they no

Nit: "non-TURN"

---------------------------------------------------------------------------

[DISCUSS]

§6.5:

 >   All implementations MUST implement DTLS 1.0

I understand that it's still a work in progress, but the current text in
draft-ietf-tls-oldversions-deprecate §9 would seem to counter-indicate this
recommendation:

    o  Implementations MUST NOT negotiate DTLS version 1.0 [RFC4347].

Unless I misunderstand the situation, one of these two documents needs to
change. Right?

---------------------------------------------------------------------------

[DISCUSS]

§6.5:

 >     *  The "security characteristics" MUST indicate the cryptographic
 >        algorithms in use (For example: "AES-CBC".)  However, if Null
 >        ciphers are used, that MUST be presented to the user at the
 >        top-level UI.

Text earlier in this section forbids the negotiation of null cipher 
suites. I
suspect this is legacy text?

---------------------------------------------------------------------------

§7:

 >  Recently, a number of Web-based identity technologies (OAuth,

Consider citing RFC 6749.

---------------------------------------------------------------------------

§7.1:

 >  Authoritative:   IdPs which have verifiable control of some section
 >     of the identity space.  For instance, in the realm of e-mail, the
 >     operator of "example.com" has complete control of the namespace
 >     ending in "@example.com".  Thus, "[e-mail]" is whoever
 >     the operator says it is.  Examples of systems with authoritative
 >     identity providers include DNSSEC, RFC 4474, and Facebook Connect

Please update to point to RFC 8224. Also, please add this an informative
reference.

Also, consider citing RFC 4035 (or something more appropriate -- it's 
not really
my area) for "DNSSEC"

---------------------------------------------------------------------------

§7.2:

 >  In order to provide security without trusting the calling site, the
 >  PeerConnection component of the browser must interact directly with
 >  the IdP.  The details of the mechanism are described in the W3C API
 >  specification, but the general idea is that the PeerConnection

Please provide a citation to:

[webrtc-identity]
     Identity for WebRTC 1.0. Adam Bergkvist; Daniel Burnett; Cullen 
Jennings;
     Anant Narayanan; Bernard Aboba; Taylor Brandstetter. W3C. W3C Candidate
     Recommendation. URL: 
https://w3c.github.io/webrtc-identity/identity.html

---------------------------------------------------------------------------

§7.2:

 >  component downloads JS from a specific location on the IdP dictated

Please expand "JS" on first use.

---------------------------------------------------------------------------

§7.7:

 >  The application can then load the provided URL to enable the user to
 >  enter credentials.  The communication between the application and the
 >  IdP is described in [webrtc-api].

All information involving the IdP has been moved from the [webrtc-api] 
document
into the [webrtc-identity] document I point to above. Please update 
accordingly.

---------------------------------------------------------------------------

§7.7:

 >  For instance, an OAuth-based protocol will likely require using the
 >  IdP as an oracle, whereas with a signature-based scheme might be able
 >  to verify the assertion without contacting the IdP, provided that it
 >  has cached the relevant public key.

This sentence is ungrammatical in a way that isn't trivial to fix. Please
re-work.

---------------------------------------------------------------------------
[DISCUSS]

§8.1:

 >  The identity provided from the IdP to the RP browser MUST consist of
 >  a string representing the user's identity.  This string is in the
 >  form "<user>@<domain>", where "user" consists of any character except
 >  '@', and domain is an internationalized domain name [RFC5890].

This needs to specify whether the domain name is encoded as an A-label 
or as a
U-label. Although either would work, I suspect that U-label is what is 
intended.

---------------------------------------------------------------------------

§8.1:

 >  Sites that have identities that do not fit into the RFC822 style (for

Please either make RFC822 a citation or designate this using different
terminology (e.g., "<user>@<domain>").

---------------------------------------------------------------------------
[DISCUSS]

§8.1:

 >  instance, identifiers that are simple numeric values, or values that
 >  contain '@' characters) SHOULD convert them to this form by escaping
 >  illegal characters and appending their IdP domain (e.g.,
 >  [e-mail]), thus ensuring that they are
 >  authoritative for the identity.

The form of escaping here needs to either be normatively defined, or this
document needs to be extremely clear that the receiving party cannot 
rely on the
ability to un-escape user portions. The example implies the use of URI-style
percent encoding -- if this is meant to be normative, please cite RFC 
3986 §2.1.

If no canonical escaping mechanism is intended, this should probably use 
a term
other than "escaping," as the reciprocal operation of "unescaping" becomes
impossible. Something like "transformed" seems more appropriate.

---------------------------------------------------------------------------

§9.2:

 >  Such services SHOULD instruct the browser to use separate
 >  DTLS keys for each call and also to use TURN throughout the call.

The phrasing here ("instruct") seems a bit odd, as this is required by this
document to be the default mode of behavior. Something involving 
"...SHOULD not
override..." would be less confusing.