Subject: Re: [Gen-art] Gen-ART Last Call review of draft-baeuerle-netnews-cancel-lock-05

From: Michael Bäuerle <[email-masked]>
Date: Fri, 7 Jul 2017 17:47:50 +0200

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.

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

> (3) Minor:
> [...]

Issue 2 should be discussed first.

The hash function used for HMAC in Section 4 is unrelated to <scheme>
and unrelated to the check algorithm from Section 3.5.

On the other hand, the hash function from <scheme> is not used to
create <c-key-string> elements for the Cancel-Key header field.

-- 
Michael Bäuerle

---

From: Paul Kyzivat <[email-masked]>
Date: Mon, 10 Jul 2017 12:47:00 -0400

Michael,

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

>        the algorithm defined by its <scheme> part and then Base64

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? 
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. This would mean that the cancel-key would be 
checked against locks that used a different hash, but that shouldn't 
hurt anything.

> create <c-key-string> elements for the Cancel-Key header field.

I agree that this comment is moot given the answer to (2).

I didn't see a response from you on (4).

	Thanks,
	Paul

---

From: Julien ÉLIE <[email-masked]>
Date: Mon, 10 Jul 2017 21:31:33 +0200

Hi Paul,

> be some coordination of that fix with this draft.

Is a draft really needed?

In RFC 5536, I for instance read wording about taking into accounts errata:

2.3.  MIME Conformance

    User agents MUST meet the definition of MIME conformance in [RFC2049]
    and MUST also support [RFC2231].  This level of MIME conformance
    provides support for internationalization and multimedia in message
    bodies [RFC2045], [RFC2046], and [RFC2231], and support for
    internationalization of header fields [RFC2047] and [RFC2231].  Note
    that [Errata] currently exist for [RFC2045], [RFC2046], [RFC2047] and
    [RFC2231].

    [Errata]       "RFC Editor Errata",
                   <http://www.rfc-editor.org/errata.php>.

Maybe *this* document could have similar wording, like "it extends 
news-fields defined in Section 3 of [RFC5536] amended with verified 
erratum xxx".

Also note that, when referencing RFCs, the current RFC Editor's practice 
is to link to <http://www.rfc-editor.org/info/rfc5536> (that provides a 
link to the errata page).

Last but not least, for *this* document, if extending the ABNF grammar 
of another RFC gives too much bargain, then why just *not* extend 
anything and just define <cancel-lock> and <cancel-key> as-is?

Definition of Jabber-ID or DKIM-Signature header fields for instance do 
not extend anything from RFC 5322 (mail):
   https://tools.ietf.org/html/rfc7259
   https://tools.ietf.org/html/rfc6376

-- 
Julien ÉLIE

« – Chef ! On vient !
   – On se met en carré ?
   – Non ! En bosquet ! » (Astérix)

---

From: Paul Kyzivat <[email-masked]>
Date: Mon, 10 Jul 2017 16:53:27 -0400

On 7/10/17 3:31 PM, Julien ÉLIE wrote:
>> tentative. *This* document would of course require a similar change. 
>> Or else it could extend that change, via something like
>>
>> 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.
> 

The problem is that if you are going to extend the definition of 
news-fields (news-fields =/ ...), then you need some way to reference 
the base definition that you are extending. If that is only in an 
erratum then I don't know how you can reference it.

Perhaps you can find some way to make the two independent, but nothing 
comes immediately to my mind.

	Thanks,
	Paul

---

From: Michael Bäuerle <[email-masked]>
Date: Tue, 11 Jul 2017 20:39:34 +0200

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

If this is possible, then the current:

   fields =/ *( cancel-lock / cancel-key )

can be changed to:

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

Rewriting RFC 5322 and RFC 5536 should be avoided if possible.

> 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

> 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 

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

-- 
Michael Bäuerle

---

From: Paul Kyzivat <[email-masked]>
Date: Fri, 14 Jul 2017 12:58:29 -0400

Michael,

Please see further comments inline.

(Sorry - the indenting is getting deep, but I think we are pretty much 
done.)

> Is it (in general) possible to reference a modified ABNF definition in

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.

>     though it were news header fields as defined in Section 3 of

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.

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

> has moved out of contact [...]" can be applied.

I won't belabor this point because it will clearly be an infrequent event.

	Thanks,
	Paul

---

From: Michael Bäuerle <[email-masked]>
Date: Mon, 17 Jul 2017 20:00:17 +0200

> invalidated, and if it is wrong then it doesn't matter.

Yes, it is one-time (only matching one target article).
This is the reason why the recommended algorithm in Section 4 uses the
unique Message-ID as base component to calculate K.

Because the Message-ID is public, a second component (local secret)
is used to calculate K. The HMAC algorithm must protect this local
secret, so that it cannot be calculated from the resulting K (that is
intended to be published inside the Cancel-Key header field).

Even with on-path attacks the relevant properties should stay intact:
1) An attacker should not be able to generate valid K values for
   foreign articles himself
2) An attacker should not be able to use hijacked K values to cancel
   different articles (than intended by the legitimate creators of
   these K values)

As a result, only legitimate persons/agents should be able to approve
cancels for existing articles.

Integrity protection for new articles is not a provided property.
Supersedes are not more than cancels combined with new articles.

> each distinct actor (poster, posting agent, moderator or injecting 

The document is labeled to update RFC 5537. The general behaviour of
agents is already documented there:
<https://tools.ietf.org/html/rfc5537#section-3.4> ff.

Specific agent behaviour regarding to this document is, in
general, limited to the principle described in Section 3:
A moderator or injecting agent that uses Cancel-Locks must be able to
authenticate the author and act representitive for the author (if no
Cancel-Lock/-Key header field is present).

Maybe this is too strict and moderators and injecting agents should be
allowed to add the Cancel-Lock header field only for themselves too.

-- 
Michael Bäuerle