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

[Alexey Melnikov <[e-mail]>]

Paul,

On Mon, Sep 18, 2017, at 10:14 PM, 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?

It is on my to do list *before* the telechat. I contacted Pete Resnick
and he suggested a slightly better fix to your erratum. This is just
taking time on my part.

Best Regards,
Alexey

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