Re: [Gen-art] draft-baeuerle-netnews-cancel-lock-06 and RFCs 5536 & 5322

[Brian E Carpenter <[e-mail]>]

> The question is: in the meantime, what to do about this draft? By 
> default I guess I will simply restate the issue in my genart telechat 
> review and leave it for the IESG discuss it.

That's about all you can do. Of course if the IESG approves it anyway
you can appeal that decision. I'm not aware of any case where a Gen-ART
reviewer has needed to do that, but there's a first time for everything,
and the plenaries are boring if there are no appeals to rehash ;-)

Regards
   Brian

On 19/09/2017 09:14, Paul Kyzivat wrote:
> I'm on tap to do the genart telechat review on 
> draft-baeuerle-netnews-cancel-lock-06. I see that nothing has been done 
> about the ABNF that was my primary concern in my prior wglc review of -05:
> 
> https://mailarchive.ietf.org/arch/msg/gen-art/wXKa5tadbo_xQbbUUCk9RZyB0w8
> 
> I'm told by the authors that Alexey asked that it be submitted this way, 
> but the ABNF *is* wrong, and I can't see how a new draft can be allowed 
> to be published with knowingly wrong ABNF. OTOH, the problem didn't 
> originate with this draft. Rather the problem originated with RFC5536, 
> and this draft has simply built on that mistake. Unfortunately the root 
> of the problem is with RFC5322, since its ABNF is structured in a way 
> that makes it really hard to define extensions such as those in RFC5536 
> and this draft. So the cleanest solution I can see is to revise both 
> 5322 and 5536, and then this draft can build on those changes. That is a 
> rather involved fix but I don't see any other reasonable way.
> 
> This prompted me to submit an erratum (#5116) on RFC5536 regarding this 
> issue. But errata aren't often acted on quickly when they require 
> revising RFCs.
> 
> The question is: in the meantime, what to do about this draft? By 
> default I guess I will simply restate the issue in my genart telechat 
> review and leave it for the IESG discuss it.
> 
> Anybody have a better idea?
> 
> 	Thanks,
> 	Paul
> 
> On 7/14/17 12:58 PM, Paul Kyzivat wrote:
>> Michael,
>>
>> Please see further comments inline.
>>
>> (Sorry - the indenting is getting deep, but I think we are pretty much 
>> done.)
>>
>> On 7/11/17 2:39 PM, Michael Bäuerle wrote:
>>> Paul Kyzivat wrote:
>>>> On 7/7/17 11:47 AM, Michael Bäuerle wrote:
>>>>> Paul Kyzivat wrote:
>>>>>>
>>>>>> [...]
>>>>>> General Comments:
>>>>>>
>>>>>> I have not attempted to validate the security properties of this
>>>>>> document - leaving that to a security review.
>>>>>>
>>>>>> I have also not attempted to verify the operational suitability of 
>>>>>> this
>>>>>> mechanism because I don't have the experience needed to do so.
>>>>>>
>>>>>> Issues:
>>>>>>
>>>>>> Major: 1
>>>>>> Minor: 3
>>>>>> Nits:  0
>>>>>>
>>>>>> (1) MAJOR:
>>>>>>
>>>>>> [Problem with ABNF syntax of header fields]
>>>>>
>>>>> Hi Paul,
>>>>>
>>>>> Julien Élie has proposed a new syntax definition.
>>>>
>>>> I was copied on his mail proposing a change to RFC5536, and that is
>>>> tentative. *This* document would of course require a similar change. Or
>>>> else it could extend that change, via something like
>>>>
>>>>     news-fields =/ cancel-lock / cancel-key
>>>>
>>>> But that can only be done if there is actually a draft that revises 5536
>>>> that can then be referenced. Bottom line is that it seems there needs to
>>>> be some coordination of that fix with this draft.
>>>
>>> It's clear that the Cancel-Lock draft needs to be changed. My comment
>>> was about the potential erratum for RFC 5536 and the question:
>>> Is it (in general) possible to reference a modified ABNF definition in
>>> an erratum?
>>
>> I don't know of any way to do so. Perhaps somebody else can provide a 
>> more definitive answer. You might try asking on rfc-interest.
>>
>>> If this is possible, then the current:
>>>
>>>     fields =/ *( cancel-lock / cancel-key )
>>>
>>> can be changed to:
>>>
>>>     news-fields =/ cancel-lock / cancel-key
>>>
>>> and should then reference the new ABNF from the erratum.
>>> If not, what about a less formal definition like this:
>>>
>>>     The Cancel-Lock and Cancel-Key header fields MUST be treated as
>>>     though it were news header fields as defined in Section 3 of
>>>     [RFC5536].
>>
>> I'm not sure about that. It might fly, or not. Again, I think you need 
>> to ask somebody more authoritative.
>>
>>> Rewriting RFC 5322 and RFC 5536 should be avoided if possible.
>>
>> I understand why you might not want to take on responsibility for that. 
>> But it is the right thing to do. Question is whether it needs to be done 
>> now.
>>
>>>>>> (2) Minor:
>>>>>>
>>>>>> In Section 3.5, step 1 says to hash the key using the algorithm 
>>>>>> from its
>>>>>> scheme. But IIUC the scheme describes the algorithm that has already
>>>>>> been used when constructing the Cancel-Key header.
>>>>>
>>>>> No. There are two different operations that use a hash function.
>>>>>
>>>>> The recommended algorithm described in Section 4 calculates:
>>>>> |
>>>>> | K = HMAC(uid+mid, sec)
>>>>>
>>>>> K is then used for <c-key-string> in the Cancel-Key header field only
>>>>> with Base64 as transfer encoding (no further hash operation is used):
>>>>>
>>>>>      c-key-string = Base64(K)
>>>>>
>>>>> HMAC can be based on a different hash function than the one in <scheme>
>>>>> (because a serving agent doesn't need to know how K was calculated for
>>>>> the check algorithm defined in Section 3.5).
>>>>>
>>>>> A value of e.g. "sha256" for <scheme> in a Cancel-Key header field
>>>>> doesn't mean that SHA256 was used to generate this field, but that the
>>>>> corresponding Cancel-Lock header field (in the original article) 
>>>>> contains
>>>>> the value of <c-key-string> hashed with SHA256 (and finally Base64
>>>>> encoded).
>>>>>
>>>>> The hash function defined by <scheme> is used to generate 
>>>>> <c-lock-string>
>>>>> elements for the Cancel-Lock header field:
>>>>>
>>>>>      c-lock-string = Base64(hash(c-key-string))
>>>>>                             ^^^^
>>>>> This hash function is the one specified with <scheme> (in both matching
>>>>> elements <c-lock> and <c-key> in the Cancel-Lock and Cancel-Key header
>>>>> fields of the two articles involved).
>>>>>> IIUC the serving
>>>>>> agent need not hash the provided key. Rather it only uses the 
>>>>>> scheme to
>>>>>> select the locks to compare the hash against.
>>>>>
>>>>> <scheme> is used for both purposes.
>>>>>
>>>>> Step 1 is defined in Section 3.5 as:
>>>>> |
>>>>> | 1. The <c-key-string> part of the <c-key> element is hashed using
>>>>> |    the algorithm defined by its <scheme> part.
>>>>>
>>>>> This is the operation for <c-lock-string> listed above.
>>>>> The hash operation is required here because if the elements from the
>>>>> Cancel-Key header field would be directly comparable, everybody could
>>>>> forge them (copy from the public original article).
>>>>> The hijack/abuse attack against <c-key-string> elements on their way
>>>>> through the network (as discussed for the review from David Mandelberg)
>>>>> is not the same. The relevant difference is who can *create* valid
>>>>> <c-key-string> elements.
>>>>>
>>>>> In Step 2 <scheme> is used again to skip <c-lock-string> elements that
>>>>> were created using different hash functions. The compare operation from
>>>>> Step 2 (with first operand from Step 1) is:
>>>>>
>>>>>      Base64(hash(c-key-string)) == c-lock-string
>>>>>
>>>>> The Base64 encode part is not explicitly noted in Step 1.
>>>>> Base64 decoding the second operand instead would work too:
>>>>>
>>>>>      hash(c-key-string) == Base64_decode(c-lock-string)
>>>>>
>>>>> To be unambiguous and consistent with the definition of <c-lock-string>
>>>>> from Section 2.1, the words for Step 1 in Section 3.5 maybe should
>>>>> explicitly cover the Base64 encode operation. Suggestion:
>>>>>
>>>>>      1. The <c-key-string> part of the <c-key> element is hashed using
>>>>>         the algorithm defined by its <scheme> part and then Base64
>>>>>         encoded.
>>>>
>>>> Hmm. That wasn't so clear to me, though on rereading I can understand it
>>>> that way.
>>>>
>>>> But in that case, what is the point of passing the scheme around at all?
>>>
>>> The <scheme> part of <c-key> elements serve the following purposes:
>>> - Backward compatibility
>>>    Existing implementations use it this way since many years
>>> - Define the hash algorithm to apply on <c-key-string> elements
>>> - Select corresponding <c-lock-string> elements to compare against
>>>
>>>> IIUC doing so doesn't add any additional security or privacy. You could
>>>> simply pass the hashed value in cancel-key. Then the verifying agent
>>>> wouldn't do any hashing.
>>>
>>> In this case the creator of the Cancel-Key header field would not prove
>>> that he really knows K.
>>>
>>>> This would mean that the cancel-key would be
>>>> checked against locks that used a different hash, but that shouldn't
>>>> hurt anything.
>>>
>>> Yes, it is unlikely that comparing against <c-lock-string> elements,
>>> that used a different hash algorithm, will hurt.
>>>
>>> But changing the general syntax of the header fields would break
>>> backward compatibility to existing implementations. This should be
>>> avoided with highest priority.
>>
>> I studied the draft further. I finally get that it is simpler than I was 
>> expecting it to be. So really it is just that the lock contains 
>> hash(password) while the key contains the password. (The key is just an 
>> algorithmically generated password.) I couldn't believe you would really 
>> be passing the password in the clear, since that would expose it to 
>> on-path attack. But I guess your assumption is that this is really a 
>> one-time password, and that if it is correct then it will immediately be 
>> invalidated, and if it is wrong then it doesn't matter.
>>
>> So I'll withdraw my concerns on this section, except that maybe it could 
>> be clearer. Specifically, section 3 could be more explicit about what 
>> each distinct actor (poster, posting agent, moderator or injecting 
>> agent) does.
>>
>>>> [...]
>>>> I didn't see a response from you on (4).
>>>
>>> Sorry for the delay, I was busy.
>>>
>>> | (4) Minor:
>>> |
>>> | In Section 8.1/8.2 the syntax/semantics of the Owner/Change
>>> | controller is unspecified. But IANA is charged with 
>>> allowing/rejecting change
>>> | requests only from the "same" owner. How is IANA expected to verify
>>> | this?
>>>
>>> If it is an experimental algorithm, I see no need for strong 
>>> authentication
>>> of external contributors.
>>>
>>> For the algorithms on the standards track, the IESG is defined as owner
>>> (may authenticate itself to IANA with a signature).
>>>
>>> | What if a change is required but the original owner is no longer
>>> | available?
>>> | E.g., the owner might be specified by email address. Later the mail
>>> | server for that email address may no longer exist.
>>>
>>> In this case the part "[...] where the owner of the registration [...]
>>> has moved out of contact [...]" can be applied.
>>
>> I won't belabor this point because it will clearly be an infrequent event.
>>
>>      Thanks,
>>      Paul
>>
>> _______________________________________________
>> Gen-art mailing list
>> [e-mail]
>> https://www.ietf.org/mailman/listinfo/gen-art
>>
> 
> 
> _______________________________________________
> Gen-art mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/gen-art
>