Subject: [MMUSIC] WGLC on draft-ietf-mmusic-sdp-uks-03

From: Flemming Andreasen <[email-masked]>
Date: Mon, 7 Jan 2019 09:08:53 -0500

Greetings MMUSIC

This is to announce a 2 week WGLC on the draft:

     https://www.ietf.org/id/draft-ietf-mmusic-sdp-uks-03.txt

as Proposed Standard. Please review and provide any comments you may 
have on the document by Monday, January 21, 2019. Comments should be 
sent to the document authors and the MMUSIC WG list. If you review the 
document but do not have any comments, please send a note to that effect 
as well.

Thanks

-- Flemming (MMUSIC co-chair)

---

From: Magnus Westerlund <[email-masked]>
Date: Wed, 16 Jan 2019 12:31:47 +0000

Hi,

I have reviewed this document and I don't think this is yet ready for publications. There appears to be issues that appears to question the possibility for interoperable implementation.

1. Section 3.2:

   The "extension_data" for the "external_id_hash" extension contains a
   "ExternalIdentityHash" struct, described below using the syntax
   defined in [TLS13]:

I assume it is the presentation language in Section 3 of RFC 8446 that you are referring to? Can you be a bit more explicit in the reference to point to the actual section?

2. Section 3.2:

"   A WebRTC identity assertion is provided as a JSON [JSON] object that
   is encoded into a JSON text."

That doesn't match what is written in Sections 7.4 and 7.4.1 of draft-ietf-rtcweb-security-arch-17.

What is provided as an JSON is the fingerprint, not the Identity assertion (see 7.4)

What 7.4.1 says about identity assertions are:

   Once an IdP has generated an assertion, it is attached to the SDP
   offer/answer message.  This is done by adding a new 'identity'
   attribute to the SDP.  The sole contents of this value are a
   Base64-encoded [RFC4648] identity assertion.

Thus, they appear to be only Base64 encoded binary blobs.

3. Section 3.2:

"The SDP "identity" attribute includes the base64 [BASE64] encoding of
   the same octets that were input to the hash."

I don't understand this sentence. What was included into which hash. Please rewrite to be descriptive of which information to retrieve or at least be explicit to reference the relevant component of the identity attribute.

Can you also be more explicit that the SDP "identity" attribute is defined in draft-ietf-rtcweb-security-arch?

4. Section 3.2:

  "The "external_id_hash"
   extension is validated by performing base64 decoding on the value of
   the SDP "identity" attribute, hashing the resulting octets using SHA-
   256, and comparing the results with the content of the extension."

To my understanding the binary blob that is Base64 encoded and put in the SDP Identity attribute after the IdP has taken the fingerprint and the user identity assertion is IdP specific and really a binary blob that only the IdP specific validator can check. So why is the validation of a binary blob done on the binary blob rather than the base64 encoding? Risk for non canonical encoding? I don't quite see that as the IdP will produce the assertion and even the base64 encoding happens once before being provided in SDP and can thus the hash can be run on the Base64.

5. Section 3.2:
   "Where a PASSPoRT is used, the compact form of the PASSPoRT MUST be
   expanded into the full form.  The base64 encoding used in the
   Identity (or 'y') header field MUST be decoded then used as input to
   SHA-256."

Also here there appear to be some mismatch between what RFC 8224 defines as a full passport object. See Section 4.1 of RFC 8224:

  After these two JSON objects, the header and the payload, have been
   constructed and base64-encoded, they must each be hashed and signed
   per [RFC8225], Section 6.  The header, payload, and signature
   components comprise a full PASSporT object.

6. As currently written but questioned above, if the object to hash is a JSON object, are the delimitation of the object given, such that one knows if any final CRLF are to be included or not.

7. Section 3.2:

Identity bindings in either form might be provided by only one peer.
   An endpoint that does not produce an identity binding MUST generate
   an empty "external_id_hash" extension in its ClientHello.

What is an empty "external_id_hash" extension? Is that with 0 bytes of hash data?

Cheers

Magnus

On 2019-01-07 15:09, Flemming Andreasen wrote:
Greetings MMUSIC

as Proposed Standard. Please review and provide any comments you may have on the document by Monday, January 21, 2019. Comments should be sent to the document authors and the MMUSIC WG list. If you review the document but do not have any comments, please send a note to that effect as well.

Thanks

--

Magnus Westerlund

----------------------------------------------------------------------
Network Architecture & Protocols, Ericsson Research
----------------------------------------------------------------------
Ericsson AB                 | Phone  +46 10 7148287
Torshamnsgatan 23           | Mobile +46 73 0949079
SE-164 80 Stockholm, Sweden | mailto: [email-masked]<mailto:[email-masked]>
----------------------------------------------------------------------

---

From: Martin Thomson <[email-masked]>
Date: Thu, 17 Jan 2019 13:24:51 +1100

Hi Magnus, thanks for reviewing.

PR for all the changes here:
https://github.com/martinthomson/sdp-uks/pull/6

And responses inline.

On Wed, Jan 16, 2019, at 23:31, Magnus Westerlund wrote:
> I assume it is the presentation language in Section 3 of RFC 8446 that 
> you are referring to? Can you be a bit more explicit in the reference to 
> point to the actual section?

Sure.  Done.

> Thus, they appear to be only Base64 encoded binary blobs.

That binary blob is a UTF-8 encoded JSON text.  That format is described in Section 7.6 of the security-arch doc.  I admit, having read your comment and gone looking for this, I would have missed this connection.  I think that the security-arch needs some editorial work.  The process is:

1. collect all fingerprint attributes
2. assemble into an object as described in 7.4
3. serialize that object into a JSON text
4. pass that as a string to the IdP (the IdP treats this as opaque)
5. get back a JSON object in the form described in 7.6
6. serialize that object into a UTF-8-encoded JSON text
7. base64 encode THAT to produce the a=identity attribute value

That is what is implemented at least.

This needs to be a JSON object because  the relying party needs to be able to extract the attributes described in 7.6 in order to instantiate the IdP.

> I don't understand this sentence. What was included into which hash. 
> Please rewrite to be descriptive of which information to retrieve or at 
> least be explicit to reference the relevant component of the identity 
> attribute.

I made an attempt.  Take a look at the PR to see if that helps at all.

> To my understanding the binary blob that is Base64 encoded and put in 
> the SDP Identity attribute after the IdP has taken the fingerprint and 
> the user identity assertion is IdP specific and really a binary blob 
> that only the IdP specific validator can check. So why is the validation 
> of a binary blob done on the binary blob rather than the base64 
> encoding? Risk for non canonical encoding? I don't quite see that as the 
> IdP will produce the assertion and even the base64 encoding happens once 
> before being provided in SDP and can thus the hash can be run on the 
> Base64.

The potential for disagreement about padding is what concerned me here.  Padding in base64 is somewhat erratic.  Then there is the domain transform from string (how the headers are ostensibly represented) and octet sequences.  Avoiding another need to describe that transformation (with UTF-8 involved, I would assume), seemed less hazardous.

The value needs to be decoded anyway, so this seemed like the most robust approach.

> Also here there appear to be some mismatch between what RFC 8224 defines 
> as a full passport object. See Section 4.1 of RFC 8224:
> 

I've tried to address that in the same commit above.  The construction of the JWS is - as usual - subject to multiple base64 rounds, so the trick is in identifying the right binary bits to hash.  I think that what I have described is clearer.

> 6. As currently written but questioned above, if the object to hash is a 
> JSON object, are the delimitation of the object given, such that one 
> knows if any final CRLF are to be included or not.

All of these protocol steps are vulnerable to whitespace changes.  PASSPorT rather naively tries to address this point with canonicalization, but this doesn't.  If whitespace is caught by the a=identity attribute or Identity: header field, then that whitespace needs to be hashed.  Do you think that needs to be said explicitly?  I didn't do that, but can if you think that helps.

> What is an empty "external_id_hash" extension? Is that with 0 bytes of 

Correct.  Updated in the PR.

---

From: Magnus Westerlund <[email-masked]>
Date: Thu, 17 Jan 2019 08:50:35 +0000

Hi Martin,

Thanks for the pull request. I have detailed inline comments, but I
think the pull request do address my issues.

> This needs to be a JSON object because  the relying party needs to be able to extract the attributes described in 7.6 in order to instantiate the IdP.

Although I didn't read all the sections in 7, I do agree that security
arch appear to need some clarification so that what you write is clear.

I think the new text is at least clear on what you use as input.

> I made an attempt.  Take a look at the PR to see if that helps at all.

Yes, I think this is clear.

> The value needs to be decoded anyway, so this seemed like the most robust approach.

I will not disagree with you about the potential for BASE64 encoders not
producing the same results if different implementations encoded the same
base material. So from that perspective I am fine. However, I think
there are a point of putting some motivation for the base64 decodings
into this section. Something like. "To avoid base64 encoders producing
different output, base64 data is decoded prior to hashing."

> I've tried to address that in the same commit above.  The construction of the JWS is - as usual - subject to multiple base64 rounds, so the trick is in identifying the right binary bits to hash.  I…

I think including which syntax element from the ABNF for the SDP
attribue to hash makes this much less unclear and I don't see the any
issues any more with this one.

> All of these protocol steps are vulnerable to whitespace changes.  PASSPorT rather naively tries to address this point with canonicalization, but this doesn't.  If whitespace is caught by the a=ide…

I think this is probably fine as long as implementations don't try to be
smart. As long as they hash the full output of the base64 decoder it
works fine for this purpose.

Thanks

Magnus Westerlund 

SE-164 80 Stockholm, Sweden | mailto: [email-masked]

---

From: Martin Thomson <[email-masked]>
Date: Tue, 22 Jan 2019 09:59:59 +1100

> think the pull request do address my issues.

Thanks again Magnus.

I made a few tweaks regarding canonicalization (or lack thereof) and merged the PR.  I'll await the end of the WGLC and the chairs' instruction before publishing again, but there's a preview here:

https://martinthomson.github.io/sdp-uks/draft-ietf-mmusic-sdp-uks.html

> arch appear to need some clarification so that what you write is clear.

FWIW, https://github.com/rtcweb-wg/security-arch/pull/84 attempts to fix that.