Subject: [COSE] WGLC comments from Mike Jones on draft-ietf-cose-msg-12

From: Mike Jones <[email-masked]>
Date: Mon, 13 Jun 2016 06:43:17 +0000

I'll say up front that this spec is much improved since the last time I did a full read through.  Good progress.

Substantive comments meriting working group visibility are in this note.  Corrections to grammatical, punctuation, and consistency nits are contained in the accompanying GitHub pull request, https://github.com/cose-wg/cose-spec/pull/161, which (lightly) touches 160 lines.

====

Document Title:
Change "CBOR Encoded Message Syntax" to "CBOR Object Signing and Encryption (COSE)" so that the title contains and explains the intended acronym for the work.

1.4.  CBOR Related Terminology
Please change "Applications can either fail processing or process messages with incorrect labels, however they MUST NOT create messages with incorrect labels" to "Applications MUST fail processing for messages with incorrect labels and they MUST NOT create messages with incorrect labels".  The current wording is a major security hole in a security specification.  Please close the hole!

Also, please consider moving this important semantic requirement to a section other than the terminology section, in which it is likely to be overlooked.

2.  Basic COSE Structure
Readers could be misled by the very general wording of this section into thinking that COSE_Key structures are COSE Messages, as defined in this section.  Saying explicitly that a COSE_Key is not a COSE Message would help eliminate this potential source of confusion.

One of the places that I was confused in this way was the text "The following CDDL fragment identifies all of the top level messages defined in this document".  And yet even though COSE_Key is a top-level data structure, it wasn't in the list.

3.  Header Parameters

In "Applications SHOULD perform the same checks that the labels appearing in the protected and unprotected headers are unique as well", change the "SHOULD" to "MUST".  This is the same security hole as in 1.4, which needs to be closed.  Then delete the then-superfluous sentence: "If the message is not rejected as malformed, attributes MUST be obtained from the protected bucket before they are obtained from the unprotected bucket."

The following sentence seems garbled or as if it's missing word(s) near "generic": "It uses forward references to a group definition of headers for generic and algorithms."  Please revise.

In the following text, I can't tell what "; Algorithm_Headers," is intended to mean.  Is it a comment on either the previous or subsequent line?  If so, maybe move it onto the line.

header_map = {
    Generic_Headers,
    ; Algorithm_Headers,
    * label => values
}

4.4.  Signing and Verification Process
In the following sentence, I can't work out what "the appropriate authorization is done" refers to: "In addition to performing the signature verification, one must also perform the appropriate checks to ensure that the key is correctly paired with the signing identity and that the appropriate authorization is done."  Please clarify.

5.3.  Encryption Algorithm for AEAD algorithms
This instruction is ambiguous because it doesn't say how the key is represented: "For recipients of the message, recursively perform the encryption algorithm for that recipient, using the encryption key as the plain text".  I suspect "encryption key" should be changed to "encryption key represented as a COSE_Key structure".  This same language also occurs in 5.4.

7.1.  COSE Key Common Parameters
Change either the order of the parameters in Table 3 or the order of their descriptions afterwards so that they are presented in the same order in both places.

11.2.  Context Information Structure
In the AlgorithmID description, it should probably say that the AlgorithmID is represented in the same manner as the 'alg' header parameter.

12.2.  Key Wrapping
Similarly to the ambiguity in 5.3 and 5.4, the text "The plain text to be encrypted is the key from next layer down" doesn't say how the key is represented.  In this case, I think it's a raw symmetric key that's encrypted, right?  Please say what it is.

A.2.  Counter Signature Without Headers
Should the "counter signature w/o headers | 9 | bstr" value in Table 27 be added to the appropriate registry?  If the answer is "no", please change "9" to "TBD".

                                                       Thanks,
                                                       -- Mike

---

From: "Jim Schaad" <[email-masked]>
Date: Mon, 13 Jun 2016 17:03:30 -0700

See   https://github.com/cose-wg/cose-spec/pull/162

From: COSE [mailto:[email-masked]] On Behalf Of Mike Jones
Sent: Sunday, June 12, 2016 11:43 PM
To: [email-masked]
Subject: [COSE] WGLC comments from Mike Jones on draft-ietf-cose-msg-12

I'll say up front that this spec is much improved since the last time I did
a full read through.  Good progress.

Substantive comments meriting working group visibility are in this note.
Corrections to grammatical, punctuation, and consistency nits are contained
in the accompanying GitHub pull request,
https://github.com/cose-wg/cose-spec/pull/161, which (lightly) touches 160
lines.

====

Document Title:

Change "CBOR Encoded Message Syntax" to "CBOR Object Signing and Encryption
(COSE)" so that the title contains and explains the intended acronym for the
work.

[JLS] See previous mail

Please change "Applications can either fail processing or process messages
with incorrect labels, however they MUST NOT create messages with incorrect
labels" to "Applications MUST fail processing for messages with incorrect
labels and they MUST NOT create messages with incorrect labels".  The
current wording is a major security hole in a security specification.
Please close the hole!

[JLS] What is the security hole that you see here.  I don't see what it
would be myself.

Also, please consider moving this important semantic requirement to a
section other than the terminology section, in which it is likely to be
overlooked.

2.  Basic COSE Structure

Readers could be misled by the very general wording of this section into
thinking that COSE_Key structures are COSE Messages, as defined in this
section.  Saying explicitly that a COSE_Key is not a COSE Message would help
eliminate this potential source of confusion.

One of the places that I was confused in this way was the text "The
following CDDL fragment identifies all of the top level messages defined in
this document".  And yet even though COSE_Key is a top-level data structure,
it wasn't in the list.

[JLS] Deleted keys from the table.

3.  Header Parameters

In "Applications SHOULD perform the same checks that the labels appearing in
the protected and unprotected headers are unique as well", change the
"SHOULD" to "MUST".  This is the same security hole as in 1.4, which needs
to be closed.  Then delete the then-superfluous sentence: "If the message is
not rejected as malformed, attributes MUST be obtained from the protected
bucket before they are obtained from the unprotected bucket."

[JLS] This is not the same security hole as in section 1.4.  They are
talking about totally different subjects.  There is no true security hole in
this text given that a correct method of dealing with the problem is given
should duplicates not be detected.  I do not think that these need to be
changed.

The following sentence seems garbled or as if it's missing word(s) near
"generic": "It uses forward references to a group definition of headers for
generic and algorithms."  Please revise.

In the following text, I can't tell what "; Algorithm_Headers," is intended
to mean.  Is it a comment on either the previous or subsequent line?  If so,
maybe move it onto the line.

header_map = {

    Generic_Headers,

    ; Algorithm_Headers,

    * label => values

}

[JLS] Originally I intended to create CDDL for the algorithm specific
headers and I never did.  All things considered I think it can just vanish.

In the following sentence, I can't work out what "the appropriate
authorization is done" refers to: "In addition to performing the signature
verification, one must also perform the appropriate checks to ensure that
the key is correctly paired with the signing identity and that the
appropriate authorization is done."  Please clarify.

[JLS]  Changed

This instruction is ambiguous because it doesn't say how the key is
represented: "For recipients of the message, recursively perform the
encryption algorithm for that recipient, using the encryption key as the
plain text".  I suspect "encryption key" should be changed to "encryption
key represented as a COSE_Key structure".  This same language also occurs in
5.4.

[JLS] I have made a change - I don't know if it will resolve this for you or
not. 

Change either the order of the parameters in Table 3 or the order of their
descriptions afterwards so that they are presented in the same order in both
places.

[JLS] Done

In the AlgorithmID description, it should probably say that the AlgorithmID
is represented in the same manner as the 'alg' header parameter.

[JLS] Done

12.2.  Key Wrapping

Similarly to the ambiguity in 5.3 and 5.4, the text "The plain text to be
encrypted is the key from next layer down" doesn't say how the key is
represented.  In this case, I think it's a raw symmetric key that's
encrypted, right?  Please say what it is.

[JLS] That would be based solely on what the next layer down wanted, I don't
believe it is appropriate to make that statement at this point.

Should the "counter signature w/o headers | 9 | bstr" value in Table 27 be
added to the appropriate registry?  If the answer is "no", please change "9"
to "TBD".

[JLS] Done

                                                       Thanks,

                                                       -- Mike

---

From: Mike Jones <[email-masked]>
Date: Tue, 14 Jun 2016 19:20:15 +0000

Jim - I have updated https://github.com/cose-wg/cose-spec/pull/161 to remove the two proposed changes that you disagreed with.  Unless you weren't done reading the pull request changes, this should now be ready to merge.

                                                       -- Mike

From: Mike Jones
Sent: Sunday, June 12, 2016 11:43 PM
To: [email-masked]
Subject: WGLC comments from Mike Jones on draft-ietf-cose-msg-12

====

Document Title:

2.  Basic COSE Structure

3.  Header Parameters

header_map = {
    Generic_Headers,
    ; Algorithm_Headers,
    * label => values
}

12.2.  Key Wrapping

                                                       Thanks,
                                                       -- Mike