[dtn-interest] [Fwd: Fwd: Nits on draft-irtf-dtnrg-bundle-security]

[Elwyn Davies <[e-mail]>]

Comments received during IESG review

--- Begin Message ---

Aaron and Elwyn,

I should have cced you on this.  That'll teach me to not read the 
history before I send these out.

spt

-------- Original Message --------
Subject: Nits on draft-irtf-dtnrg-bundle-security
Date: Sat, 29 Jan 2011 17:23:28 -0500
From: Sean Turner <[e-mail]>
To: [e-mail]
CC: [e-mail], The IESG <[e-mail]>

Hi,

Here's some comments/questions I have on this document (note this isn't
my RFC 5742 review):

- No 'Intended status' indicated for this document; I'm assuming
Experimental as per the datatracker.  I think you should probably add
the header line to make sure though.

- There are two unused references: RFC 3370 and RFC 4106.

I think 3370 could we worked in somewhere when describing the CMS bits.

For the 4106 I think the checker is barfing on the following from
Section 4.3 and 4.4:

[Note: parts of the following description are borrowed from RFC 4106].

Maybe replace it with:

Note: parts of the following description is borrowed from [RFC4106].

- Obsolete reference to [RFC3280].  Should be to [RFC5280].

- Couldn't find the [DTNRB] reference.  Did the document die a graceful
death?

- Expand DTN, EID, SDNV, BA, and PI on first use

- Authentication is mentioned for the first time in Sec 2 when
discussing the PIB.  Wouldn't it be better to mention it earlier?

- Section 2.1: When describing the two EIDs and cipher suite flags as
"(optional)" they should be "(OPTIONAL)" if conveying 2119 requirements.
  Same for the Figure 3 description (i.e., the paragraph before the figure).

- Section 2.3: r/the key information item Section 2.6 is optional, and
if not provided then the key should be/the key information item Section
2.6 is OPTIONAL, and if not provided then the key SHOULD be

- Sections 2.4 & 2.5: r/and that encryption-only ciphersuites NOT be
used/and it is NOT RECOMMENDED that encryption-only ciphersuites be used
Using NOT so far after the RECOMMENDED isn't "NOT RECOMMENDED".  Also if
you do this need to add "NOT RECOMMENDED" to the first para in section 1
like the errata 499:
http://www.rfc-editor.org/errata_search.php?rfc=2119

- Section 2.4: Some "SHOULDs":

    It is RECOMMENDED to apply a Payload
    Confidentiality ciphersuite to non-payload blocks only if these
    SHOULD be super-encrypted with the payload.  If super-encryption of
    ^^^^^^
    the block is not desired then protection of the block SHOULD be done
                                                          ^^^^^^
    using the Extension Security Block mechanism rather than PCB.

- Section 2.4: "SHOULD NOT" instead of "should not":

    These multiple PCB instances, are
    not "related" and SHOULD NOT contain correlators.
                      ^^^^^^^^^^

- Section 2.8: In the paragraph about authenticated and encrypted
bundles, I think the last sentence should be "RECOMMENDED".  It's
interesting to note that you'd prefer encrypt then sign as opposed to
sign and then encrypt.  Looking forward to the security consideration
about why.

- Section 3.2: r/MUST not/MUST NOT ?

- Section 3.2: r/No other blocks are
    required to follow the payload block and it is RECOMMENDED that they
    NOT do so./No other blocks are
    required to follow the payload block and it is NOT RECOMMENDED
    that they do so.

- Section 3.4.2: r/This algorithm is intended to protect parts of the
bundle which should not be changed in-transit./This algorithm is
intended to protect parts of the bundle which SHOULD NOT be changed
in-transit.

- Sections 3.4.1 & 3.4.2: Need a reference for URI.

- Sections 4.1-4.4: What's the OIDs for these "eContents"?  I assume
id-data, but you should probably be explicit about it.

- Section 4.1: r/Security parameters are optional with this
scheme/Security parameters are OPTIONAL with this scheme

- Section 4.1: A couple of comments/questions on the following:

    Implementations MUST support use of "AuthenticatedData" type as
    defined in [RFC5652] section 9.1, with RecipientInfo type
    KeyTransRecipientInfo containing the issuer and serial number of a
    suitable certificate.  They MAY support additional RecipientInfo
    types.  They MAY additionally use the "SignedData" type described in
    [RFC5652] Section 5.1.  In either case, the "eContent" field in
    EncapsulatedContentInfo contains the encrypted HMAC key.

1. Normally, we'd use the SKI instead of the issuer/serial # combo.  Any
reason you chose differently?  Most certificates have SKIs (now) and
it's probably smaller than the issuer/serial # combo.

2. How is SignedData used here?  It says it MAY be used - is that in
addition to?  There's no RecipientInfo with SignedData so I am a little
confused.

- Section 4.1 and 4.2: Should probably say something about whether you'd
recommend no unsigned/unauthenticated and signed/authenticated
attributes.  I'm assuming you're not recommending any ;)

- Sections 4.2, 4.3, & 4.4: Normally, we'd use the SKI instead of the
issuer/serial # combo.  Any reason you chose differently?  Most
certificates have SKIs and it's probably smaller than the issuer/serial
# combo.

- Section 4.2: Had the following:

   RSA is used with SHA256 as specified for the id-sha256 PKCSv2.1
   signature scheme in [RFC4055].

Is this RSA with SHA-256 PKCS#1 Version 1.5 or RSA with SHA-256 using
RSASSA-PSS?  If it's the later is the id-sha256 the hashAlgorithm in the
RSASSA-PSS-params or something else.  Also wouldn't you have to say more
like what's the hashAlgorithm, maskGenAlgorithm, whether
RSASSA-PSS-params is included in certificates?

Also r/RSA with SHA256/RSA with SHA-256

- Sections 4.1-4.4: The eContent values for the CMS content types threw
me for a loop.  You're not using them to protect the BAB, PIB, etc. it's
just the encrypted HMAC key, signed checksum, and encrypted BEK.  It
reads like there are two sets of operation for each of the four Block
Types.  You do one for the BAB, PIB, etc. values and then another for
the CMS structures?  Am I reading that right?  The parts about the
"eContent" e.g.,:

The "eContent" field in EncapsulatedContentInfo contains the signed
checksum (SignatureValue) of the data.

That tells me the CMS processing generates a message digest over the PIB
SignatureValue and then signs the output of the message digest and put
it in the SignerInfo.signature value field.

Couldn't you just omit all the eContents?  They're optional in all cases
and can be gotten from elsewhere in the CMS blobs.

- Section 7: You should point to [RFC5652]/[RFC5280] for it's security
considerations when using CMS/certificates.  This save you recreating
text about keeping keys secret, etc.  Need some additional motherhood
and apple pie statements about randomly generating keys and point to
[RFC4106].  And a pointer to [RFC5084] for concerns about AES-GCM.

- Section 7: I didn't see a security consideration that addresses why
you'd prefer to encrypt then authenticate.  The text earlier implied and
attack of some kind.  RFC 5751 also had something to say about this:

    There are security ramifications to choosing whether to sign first or
    encrypt first.  A recipient of a message that is encrypted and then
    signed can validate that the encrypted block was unaltered, but
    cannot determine any relationship between the signer and the
    unencrypted contents of the message.  A recipient of a message that
    is signed then encrypted can assume that the signed message itself
    has not been altered, but that a careful attacker could have changed
    the unauthenticated portions of the encrypted message.

- IANA Considerations: When defining an IANA controlled registry the
document needs to indicate how the registry is updated (e.g., RFC
required, expert review, etc.).  See Section 4 of RFC 5226.

Cheers,

spt

--- End Message ---