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

On Tue, Sep 19, 2017, at 10:42 AM, Alexey Melnikov wrote:
> Hi 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,
> 
> I asked the editor to submit a new version addressing all other
> concerns. I wanted to see how the rest of the draft looks.
>  
> > 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.
> 
> I was looking at your comment and it was very hard to figure out how big
> of a deal it was. You are right that ABNF by itself will cause the
> ordering issue that you reported. However, section 3.6 of RFC 5322 says
> (ignoring for the moment trace header fields, that have some ordering):
> 
>    It is important to note that the header fields are not guaranteed to
>    be in a particular order.  They may appear in any order, and they
>    have been known to be reordered occasionally when transported over
>    the Internet.  However, for the purposes of this specification,
>    header fields SHOULD NOT be reordered when a message is transported
>    or transformed.  More importantly, the trace header fields and resent
>    header fields MUST NOT be reordered, and SHOULD be kept in blocks
>    prepended to the message.  See sections 3.6.6 and 3.6.7 for more
>    information.
> 
> So I think this already clear that header fields other than trace or
> resent can appear in any order. This is effectively provides an extra
> level of flexibility not covered by the ABNF from RFC 5322. As far as I
> remember, none of the new header fields defined for Netnews articles are
> trace or resent header fields.
> 
> > 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.
> 
> I think I agree with you that RFC 5322 could have introduced a few extra
> ABNF productions to make extensibility easier.
> 
> However in short term, I think we should go with the easiest fix. Pete
> suggested:
> 
> Section 3 of RFC 5536:
> 
> Original Text:
> 
>     fields          =/ *( approved /
>                           archive /
>                           control /
>                           distribution /
>                           expires /
>                           followup-to /
>                           injection-date /
>                           injection-info /
>                           lines /
>                           newsgroups /
>                           organization /
>                           path /
>                           summary /
>                           supersedes /
>                           user-agent /
>                           xref )
> 
> Corrected Text:
> 
>     optional-field  =/ *( approved /

Just to clarify: optional-field is defined in RFC 5322 and is
specifically designed for extensibility.

We can also drop "*" in front of "(approved", as this field is already
allowed multiple times in an RFC 5322 message.

>                           archive /
>                           control /
>                           distribution /
>                           expires /
>                           followup-to /
>                           injection-date /
>                           injection-info /
>                           lines /
>                           newsgroups /
>                           organization /
>                           path /
>                           summary /
>                           supersedes /
>                           user-agent /
>                           xref )
> 
> The name optional-field is a bit unfortunate, but it was always intended
> for extensibility. So I think doing this change and adding a note saying
> that some of Netnews header fields are mandatory (despite
> "optional-field" production name) would be the best way forward.
> 
> > 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.
> 
> Thank you for the erratum. Unless there is a better proposal, I will
> edit resolution based on Pete's feedback and I will approve it.
> 
> > 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.
> 
> As per my comments above, I think we should acknowledge that there is a
> subtle issue with ABNF in RFC 5536. I think the best way forward is to
> approve some fix to your erratum and let
> draft-baeuerle-netnews-cancel-lock be reviewed by IESG. At this moment
> it is not clear to me that work on RFC 5536 will happen any time soon,
> so I don't think we should hold this draft hostage.
> 
> Best Regards,
> Alexey
> 
> > 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
> > > Gen-art@ietf.org
> > > https://www.ietf.org/mailman/listinfo/gen-art
> > > 
> > 
> > 