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 > > > > > > > _______________________________________________ Gen-art mailing list Gen-art@ietf.org https://www.ietf.org/mailman/listinfo/gen-art
