Re: [Last-Call] Last Call: <draft-ietf-cose-countersign-06.txt> (CBOR Object Signing and Encryption (COSE): Countersignatures) to Internet Standard

[Benjamin Kaduk <[e-mail]>]

Hi Russ,

Inline.

On Wed, Aug 17, 2022 at 10:54:04AM -0400, Russ Housley wrote:
> Ben:
> 
> Thanks very much for the very careful review.
> 
> You caught one very significant mistake.   Thanks very much.
> 
> > On Wed, Jul 20, 2022 at 09:18:16AM -0700, The IESG wrote:
> >> 
> >> The IESG has received a request from the CBOR Object Signing and Encryption
> >> WG (cose) to consider the following document: - 'CBOR Object Signing and
> >> Encryption (COSE): Countersignatures'
> >>  <draft-ietf-cose-countersign-06.txt> as Internet Standard
> >> 
> >> The IESG plans to make a decision in the next few weeks, and solicits final
> >> comments on this action. Please send substantive comments to the
> >> [e-mail] mailing lists by 2022-08-10. Exceptionally, comments may
> >> be sent to [e-mail] instead. In either case, please retain the beginning
> >> of the Subject line to allow automated sorting.
> >> 
> >> The file can be obtained via
> >> https://datatracker.ietf.org/doc/draft-ietf-cose-countersign/
> > 
> > This draft fills a claer need, providing "actual countersignature"
> > semantics for COSE.  However, it's also intended to play a second role,
> > namely to deprecate the original "countersignature" functionality from RFC
> > 8152 (which did not always provide "actual countersignature" semantics).
> > 
> > I think we probably need to expound further on what exactly is done to
> > achieve this deprecation: the current version uses the stem "deprecate"
> > only twice, once in text that is essentially duplicated from
> > draft-ietf-cose-rfc8152bis-struct, and the final time in the IANA
> > considerations, where IANA is to add "(Deprecated by [[This Document]]" to
> > the existing registrations.  (Note: the mismatched parentheses are in the
> > original.)  There is no discussion of what the deprecation means in
> > practice for implementations, which is a rather serious omission.
> 
> First, the missing ")" was corrected in the -07 version.
> 
> Second, the codepoints for "counter signature" and "CounterSignature0" are not removed from the IANA registry so that old counter signatures can be handled.

Right, and that seems correct to me.

> I suggest that we add the following:
> 
>    With the publication of this document, implementers are encouraged to
>    migrate used of the previous countersignature algorithm to the one
>    specified in this document.  In particular, uses of
>    "CounterSignature" will migrate to "CounterSignatureV2", and uses of
>    "CounterSignature0" will migrate to "CounterSignature0V2".  Of
>    course, the validation of "CounterSignature" and "CounterSignature0"
>    might be supported to handle previously generated counter signatures.

The general sense of this looks good to me.
For transparency, RFC-to-be 9052 appears to have all needed approvals
(including Paul's as AD), and says this (in the context of "crit"):

  Additionally, the header parameter "counter signature" (label 7) defined
  by [RFC8152] must be understood by new implementations, to remain
  compatible with senders that adhere to that document and assume all
  implementations will understand it.

with lowercase "must".  So I think the text in this document would better
align with 9052 if we s/might be supported/must be supported/.

> 
> > In particular, the current state of affairs gives the COSE header parameter
> > with label 7 ("counter signature") privileged treatment in that senders are
> > free to assume that recipients can understand and process it, even if it is
> > not listed in the "crit" header's value.  While a reasonable reader might
> > assume that being marked as "deprecated" directs a sender to not omit the
> > parameter (though more concrete guidance on that point seems apropos to
> > me), I do not think that there is a clear implication about whether an
> > impelementation must continue to implement support for processing this
> > header parameter.  In order to ensure a smooth deprecation process, I think
> > this document needs to make a specific statement about whether the
> > requirement on implementations to understand this parameter remains.
> > (I am currently grappling with how to describe this situation for RFC-to-be
> > 9052, that does not discuss "counter signature" but does retain RFC 8152's
> > guidance that labels 0-7 SHOULD be omitted from the "crit" value.  I
> > believe that the requirement on implementations to understand and be able
> > to process "counter signature" must remain for the purposes of RFC 9052.)
> 
> We could also warn that over time implementations are expected to stop recognizing "CounterSignature" and "CounterSignature0".  That said, i would probably be good to have RFC-to-be 9052 and this document handle this the same way.

I support warning that over time we expect implementations to phase out
support for the legacy structures.  I mostly expect that we would have a
new document in a year or two that formally Updates 9052 to ease this
requirement (but I also wouldn't mind if we decided to put that change into
the current -countersign document).  I'm not sure if that aligns with your
indicated preference to handle things the same way with 9052 and
-countersign.

> 
> > Since I'm posting to last-call, I did make an attempt to read the rest of
> > the document, and have some additional remarks.
> > 
> > Prior to sending to the RFC Editor, please do a pass to compare the specific
> > wordings used for definitions and other shared text between this document and
> > RFC 9052, to avoid needless divergence in phrasing.  A few specific areas
> > stuck out to me as meriting this comparison (but this should not be assumed
> > to be an exhaustive list): the introduction in general, but most notably
> > around "to be a schema-free decoder"; the security considerations, and the
> > document terminology section.
> 
> Will do.
> 
> The Security Consideration in this document primarily points to  RFC-to-be 9052, and then includes a few things explicitly related to counter signatures.  Please tell me where you see misalignment.

It looks like I was looking at -countersign-05 when I wrote this; in the
-05, the first ~80% of the security considerations looked like a copy/paste
job from 9052, and I was pointing out that the original source had been
edited since the copy was made.  Looking at the -06 and -07, that concern
seems to no longer be an issue.

> 
> > Section 1
> > 
> >   countersignature, were those that the working group desired, the
> >   decision was made to remove all of the countersignature text from
> >   [I-D.ietf-cose-rfc8152bis-struct] and create a new document to both
> >   deprecate the old countersignature algorithm and to define a new one
> >   with the desired security properties.
> > 
> > We should probably specifically indicate that the "new document" is this
> > one, in some fashion.
> 
> This was reworded a bit, and I think the last sentence addresses your point:
> 
>    During the process of advancing COSE to an Internet Standard, it was
>    noticed that while the description of the security properties of
>    countersignatures was accurate, the associate specification of their
>    implementation in Section 4.5 of [RFC8152] was incorrect for the
>    COSE_Sign1 structure.  To remedy this situation, the working group
>    decided to remove all of the countersignature text from
>    [I-D.ietf-cose-rfc8152bis-struct], which obsoletes [RFC8152].  This
>    document defines a new countersignature with the desired security
>    properties.

Yes, that looks good.

> 
> >   The new algorithm is designed to produce the same countersignature
> >   value in those cases where the cryptographic computed value was
> >   already included.  This means that for those structures the only
> >   thing that would need to be done is to change the value of the header
> >   parameter.
> > 
> > It's not entirely clear to me on first read that this is a feature.
> > Creating ambiguity about how a given ciphertext was created or should be
> > processed has historically given rise to vulnerabilities.  That said, since
> > in this particular case the actual cryptographic content is the same and
> > this is the scenario where the construction being deprecated did provide
> > the intended "actual countersignature" semantics, maybe there is not really
> > any grounds for "confusion" here.
> 
> These are Jim's words, and I would not like to change them unless there is a real need.

I cannot supply a "real need" at this time, and thus defer to your
preference.

> 
> > Section 3
> > 
> >   timestamp, a time point at which the signature exists.  When done on
> >   a COSE_Sign, this is the same as applying a second signature to the
> >   payload and adding a parallel signature as a new COSE_Signature is
> >   the preferred method.  
> > 
> > I don't think I understand what this is trying to say, as what it seems to say
> > to me doesn't make sense.  In particular, this seems to say that the act of
> > applying a countersignature to a COSE_Sign structure is functionally
> > equivalent to just appending to the "signatures" array of the COSE_Sign
> > object, and indeed that appending to "signatures" is preferred.  But that
> > seems to provide semantics that are not the (desired) semantics of a true
> > countersignature, which would cover the existing (primary) signature in
> > addition to the content!
> 
> Wow.  This error has been here a very long time.  Thanks for noticing!
> 
>    ...  That is, the countersignature makes a
>    statement about the existence of a signature and, when used as a
>    timestamp, a point in time at which the signature exists.  When a
>    timestamp is not used, a countersignature on a COSE_Sign is the same
>    as applying a second signature to the payload; therefore, adding a
>    parallel signature as a new COSE_Signature is the preferred method.

I had to read this several times to convince myself that it makes sense, so
let me write out some description of how I resolved it, to try to
cross-check my reasoning.
Just as a baseline/refresher (since I've confused myself about it in the
past), the primary COSE structures for signatures are COSE_Sign and
COSE_Sign1; the COSE_Signature structure is a substructure that holds "the
signature and information about the signature".  When we construct a
countersignature on a COSE_Sign, the procedures have us pull into the
to-be-signed bytes both the payload and "all bstr fields after the second"
(among other things), but since COSE_Sign has only two bstr fields
(protected_headers and payload), that means the "other_fields" element is
omitted.  In particular, the "signatures" member of COSE_Sign is an array
and not a bstr, so the other existing signatures in the COSE_Sign are not
included in other_fields and thus not covered by the countersignature --
we'd need to apply the countersignature on the COSE_Signature object itself
in order to do that.  Having made that insight, the focus in the new text
about timestamp/no-timestamp does make more sense, and in the no-timestampp
case the countersignature's to-be-signed bytes basically match those of a
primary signature ("new COSE_Signature [in the COSE_Sign]"), and the
recommendation here does make more sense.

That said, this seems to be the only place in the document where we mention
"timestamp", so I wonder if we want to put some additional discussion
somewhere about the mechanics of using a countersignature to add a
timestamp (e.g., in external_aad?).



> 
> > NITS
> > 
> > Section 1.2
> > 
> >   CBOR grammar in the document is presented using CBOR Data Definition
> >   Language (CDDL) [RFC8610].
> > 
> > singular/plural mismatch
> > 
> >   The collected CDDL can be extracted from the XML version of this
> >   document via the following XPath expression below.  (Depending on the
> >   XPath evaluator one is using, it may be necessary to deal with &gt;
> >   as an entity.)
> > 
> >   //sourcecode[@type='CDDL']/text()
> > 
> > Carsten pointed out for 9052 that the type needs to be lowercase in order to
> > work.
> 
> Fixed.
> 
> 
> > Section 2
> > 
> >         Generic_Headers /= (
> >         ? TBD10 => COSE_Countersignature / [+COSE_Countersignature]
> >         ; V2 Countersignature
> >         ? TBD11 => COSE_Countersignature0  ; V2 Countersignature0
> >         )
> > 
> > Since only one of the comments fits to the right, I'd suggest making both be
> > on their own line, and appearing before the content they describe.
> 
> How about this?
> 
>          Generic_Headers /= (
>          ; V2 Countersignature
>          ? TBD10 => COSE_Countersignature / [+COSE_Countersignature]
>          ; V2 Countersignature0
>          ? TBD11 => COSE_Countersignature0
>          )

Just what I was imagining.  Thanks!

-Ben