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

[Magnus Westerlund <[e-mail]>]

Hi Martin,

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


On 2019-01-17 03:25, Martin Thomson wrote:
> 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:
>
>> 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.
> 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.

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.


>
>> 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.
> I made an attempt.  Take a look at the PR to see if that helps at all.

Yes, I think this is clear.


>> 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.
> 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.

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."


>
>> 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.
> 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.

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.


>
>> 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.

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.

>
>
>> 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?
> Correct.  Updated in the PR.
>

Thanks

Magnus Westerlund 

----------------------------------------------------------------------
Network Architecture & Protocols, Ericsson Research
----------------------------------------------------------------------
Ericsson AB                 | Phone  [phone]
Torshamnsgatan 23           | Mobile [phone]
SE-164 80 Stockholm, Sweden | mailto: [e-mail]
----------------------------------------------------------------------