Re: [smime] draft-housley-ct-keypackage-receipt-n-error-00

["Jim Schaad" <[e-mail]>]

> -----Original Message-----
> From: Russ Housley [mailto:[e-mail]]
> Sent: Thursday, May 02, 2013 7:08 AM
> To: Jim Schaad
> Cc: 'IETF SMIME'
> Subject: Re: [smime] draft-housley-ct-keypackage-receipt-n-error-00
> 
> Jim:
> 
> Thank you for the review.
> 
> > 1.  What is SIR - not defined
> 
> Source Intermediary Recipient (SIR) entity name

Good

> 
> > 2.  Is it your expectation that name types which do not have ASN.1
> > values are going to be created?  If not then why is there an OCTET
> > STRING wrapper for nameValue.  This choice should be justified in the
> document.
> 
> The nameValue is an OCTET STRING, which allows the canonical form of any
> name to be carried.  Two names of the same type are considered equal if
the
> octet strings are the same length and contain the same string of octets.

Good

> 
> > 3.  Should you define a relationship for relating nameType and
> > nameValue information?  Automated packages would find it useful, it
> > also makes the fact that you are use Name rather than possibly
> > GeneralName explicit in the module.
> 
> I am not totally sure what you are suggesting.  Let me know if I got it
right.
> 
>   SIR-ENTITY-NAME ::= CLASS {
>       &SIRNameType  OBJECT IDENTIFIER UNIQUE,
>       &SIRNameValue
>   } WITH SYNTAX {
>       SYNTAX &SIRNameValue IDENTIFIED BY &SIRNameType
>   }
> 
>   SIRNames{SIR-ENTITY-NAME:SIRNameSet} ::=
>       SEQUENCE SIZE (1..MAX) OF SIRName{{SIRNameSet}}
> 
>   SIRName{SIR-ENTITY-NAME:SIRNameSet} ::= SEQUENCE {
>       sirNameType      SIR-ENTITY-NAME.&SIRNameType({SIRNameSet}),
>       sirNameValue   OCTET STRING (CONTAINING
>
SIR-ENTITY-NAME.&SIRNameValue({SIRNameSet}{@sirNameType}))

Yes that looks correct.  You could use a fixed name set if you wanted to
rather than having it be a parameter.  This would depend on how you are
planning to use it.

> 
> > 4.  You should probably give a reference to where signed,
> > authenticated and content attributes are found.  I am familiar with
> > the first two since I do a lot of CMS work, however the last one
> > really needs to be tied to an RFC and a specific content type.
> 
> This attribute can appear as a signed, authenticated, and content
attribute.
> Signed attributes are carried in the CMS Signed-data content type
described
> in Section 5 of [RFC5652].  Authenticated attributes are carried in the
CMS
> Authenticated-data content type described in Section 9 of [RFC5652] or in
the
> CMS Authenticated-enveloped-data content type described in Section 2 of
> [RFC5083].  Content attributes are carried in the Content-with-attributes
> content type described in Section 3 of [RFC4073].
> 

Good

> > 5.  Can the key package identifier and receipt request attribute have
> > multiple values or is a single value attribute?
> 
> Even though the ATTRIBUTE syntax is defined as a SET OF AttributeValue, a
> key-package-identifier-and-receipt-request attribute MUST have a single
> attribute value; zero or multiple instances of AttributeValue are not
> permitted.
> 

Good

> > 6.  We can rehash this discussion.  First I don't see any reason for
> > incrementing the version number unless you are going to re-assign the
> > same OID for the new structure as you did for the old one.  If a new
> > OID is used there is more than enough information to distinguish
> > between the two different structures.  Second, I am not a fan of
> > assigning version numbers to these structures because they do not help
any
> encoding/decoding systems.
> > The structure will be encoded or decoded based on the ASN and not on
> > the version number.
> 
> I'd rather not rehash this discussion,  Instead, I'd like to follow the
convention
> used in CMS.

I don't need to rehash the discussion either.  I will just always raise it.

> 
> > 7.  What is the purpose of the (1..MAX) on the definition of
KeyPkgVersion.
> > Are you just trying to say that it cannot be a negative value?  It
> > would be more helpful if you used a smaller version such as 2^32-1 so
> > that a compiler would know that it fit into an int32 value.  I would
> > also note that this is a change from the old definition of the field
> > in RFC 6031
> 
> Yes, the reader is warned that this definition is different.  This is also
a reason
> not to import the definition:
> 
> -- Revised definition of KeyPkgVersion from [RFC6031] KeyPkgVersion ::=
> INTEGER  { v1(1), v2(2) } (1 .. MAX)
> 
> I think a maximum of 65535 is very safe.

I agree that is probably more than sufficient.


> 
> > 8.  Is there a requirement that systems should accept
> > KeyPkgIdentifier.attribute values that they do not understand as it
> > can be reflected in the receipt without having to decode it?
> 
> As with all CMS processing, unrecognized attributes are ignored.  I'm not
sure
> this needs to be repeated further.  It comes up here:
> 
>        * badUnsignedAttrs is used to indicate that the unsignedAttrs
>          within SignerInfo contains one or more attributes.  Since
>          unrecognized attributes are ignored, this error code is used
>          when the object identifier for the attribute is recognized, but
>          the value is malformed or internally inconsistent.


I don't think that this is an acceptable solution ore response at this
point.

If I send you 

Key package id and receipt request ::= {
   pkgID = { random OID you never heard of, binary value }
  receiptReq = {
   encryptReceipt FALSE,
   receiptsFrom - absent
   receiptsTo = {Me}
}}

You have three options:

1 - say that the signed attribute is bad because you do not understand a
piece if it and neither process nor receipt the package
2 - say that you don't care that the signed attribute is bad and process it
and return a receipt because you do not need to understand the key package
identifier
3 - say that you ignore things you do not understand and process the package
but do not return a receipt.

> 
> > 9.  Why is there a requirement that the content types have to be
> > encoded using the DER rules?  This is not a general requirement
> > imposed by CMS and therefore needs some justification text.
> 
> I am not sure that justification needs to be added to the document, but
I'll
> share what I was thinking.  This content could be processed inside a
security
> boundary.  Using DER allows more straightforward processing, including the
> use of templates.

That's fine - I thought it might be something like that.  

> 
> > 10.  I find the field names errorOf and errorBy to be obtuse - but
> > that is not a strong reason to change them if they have a meaning that
> > is not documented.
> 
> The field names make sense to me.  Do you find the explanation wanting?

The explanations are just fine.   I merely find the names odd.  I would have
expected something more along the lines of

errorOnPackage - name of package
errorReportedBy -

That is longer more descriptive names.  But as I said - I don't really care
that much.

> 
> > 11. does badContentInfo apply to embedded content types (i.e.
> > eContent) as well as the ContentInfo structure?
> 
> I was thinking that a failure at an embedded layer would result in a
> badEncapContent error code.

Ok - that is what is reflected.

> 
> > 12.  Why should there be a problem with having more than one entry in
> > the digestAlgorithms field?  I can potentially understand complaining
> > if there is one that is not understood but not if there is more than
one.
> 
> I'm trying to keep it simple by requiring a single algorithm used by a
single
> signer.

Ok - I can see this as being reasonable.

> 
> > 13.  I must have missed the rule that says that there is a problem if
> > you have a signed attribute that is unknown and not ignored.  Is that
> > part of the badSignedAttrs field or is this only an issue with the ASN.1
> encoding?
> 
> AsI said above, I am following the normal CMS processing, unrecognized
> attributes are ignored.


Ok - may not be correct behavior in all cases, but that can be application
specific and that is the rule you are using in this application.

> 
> > 14.  notAuthroized - this description seems off for this content type.
> > Is the issue that the TA is not authorized or the TA does not root the
> > authorization.  It would not be the signer itself in this case one
presumes.
> 
> TAMP (RFC 5934) can be used to associate a list of authorized content
types
> with a TA.

It also allows for a list of authorized contents to be associated with a
signer certificate as well I thought.  If so then this description would
seem to not allow for that case.  That is the current signing certificate
can be more restrictive than the TA is.


> 
> > 15.  put in a reference for the content-decryption-key-identifier
attribute.
> 
> Good idea:
> 
>        * noDecryptKey indicates that the receiver does not have the key
>          named in the content-decryption-key-identifier attribute (see
>          [RFC6032]).
> 

Good

> > 16.  Is there a reason for the badKeyTransportRecipientInfo item being
> > absent?
> 
> Added ...
> 
>        * badKeyTransRecipientInfo indicates that the
>          KeyTransRecipientInfo syntax is invalid or the version is
>          unknown or unsupported.

Good

> 
> > 17.  Do you want to distinguish between a decryptFailre and a failure
> > processing a key management item?  This is the basis of some online
> > attacks to get a key when dealing with RSA v1.5 and RSA OEAP.  This
> > merits a security consideration notice all by itself.
> 
> Please see my response to comment 29 below.
> 

Yes that deals with the issue.

> > 18.  Are there any security attacks that occur by differentiating
> > between decryptFailure and invalidMAC for AuthEnvelopedData?
> 
> I do not think so.  AES-CCM and AES-GCM do not return any plaintext if
there
> is a integrity failure.

I was thinking in terms of something similar to the attacks that exist for
the RSA v1.5/OAEP differences.  There may be something that may leak
information to an attacker that could be useful.  However I think that the
comment on 29 probably addresses this issue as well.

> 
> > 19.  For mismatchedDigestAlg - are you comparing with the signature
> > algorithm or with the content digest algorithm?
> 
> I think this one is clear.  It indicates that the digest algorithm in
> digestAlgorithms field within SignedData does not match the digest
algorithm
> used by the content signer.

I might prefer s/used by the content signer/used in the signature algorithm/
However this was a double check so the current text is probably sufficient.

> 
> > 20.  You need some text to distinguish missingCertificate and
> > noTrustAnchor if keep the "using a trust anchor" text.
> 
> TAMP (RFC 5934) tells how a TA can sign the content directly.  That
applies
> here too.

Ok - I can see this

> 
> > 21.  tooManySigners - where is this restriction imposed?
> 
> I think this makes sense in most key distribution scenarios.  How about
this
> text?
> 
>        * tooManySigners indicates that a SignedData content contained
>          more than one SignerInfo for a content type that requires only
>          one signer.
> 

Good

> > 22.  can the missingSignedAttributes be used if there are attributes
> > that are required to be present but are absent - even if there are
> > some attibutes present?
> 
> Yes.

Ok

> 
> > 23.  I don't understand the reason for the missingContentHints.  This
> > attribute would be an encrypted structure not a signed structure.  Why
> > would a content hint make any difference in terms of the processing?
> 
> This is related to CMS Content Constraints (CCC) defined in RFC 6010.
> 

Yes - I had forgotten that type.

> > 24.  Are there any security attacks that are uncovered by the use of
> > the badMessageDigest error code rather than just saying that the
> > signature failed to validate?
> 
> Please see my response to comment 29 below.

Again - that covers the issue

> 
> > 25.  badAttributes should be modified to say that this is an error
> > only if the attribute is defined to say that it is not legal for that
attribute.
> > For some attributes having multiple attributes or multiple values is
legal.
> 
> How about this?
> 
>        * badAttributes indicates that an attribute collection contained
>          either multiple instances of the same attribute type that
>          allows only one instance or contained an attribute instance
>          with multiple values in an attribute that allows only one
>          value.
> 

Yes that works


> > 26.  The description of unsupportedAsymmetricKeyPackage does not make
> > sense
> > - There is a difference between not prepared and unsupported
> 
> How about this?
> 
>        * unsupportedSymmetricKeyPackage indicates that the
>          implementation does not support symmetric key packages
>          [RFC6031].
> 
>        * unsupportedAsymmetricKeyPackage indicates that the
>          implementation does not support asymmetric key packages
>          [RFC5958].
> 

Yes that deals with the issue

> > 27.  Should I be able to return more than one errorCode if I find more
> > than one error during processing?
> 
> No.  I'd like to keep it simple.

That is fine with me, I was just making sure that you had considered the
issue.

> 
> > 28.  Given the number of times that AuthenticatedData was mentioned in
> > the text, is there a reason it is omitted from section 6?
> 
> Added ...
> 
>      o AuthenticatedData can be used to integrity protect the content
>        type with message authentication algorithms that support
>        authenticated encryption, where key management information is
>        handled in a manner similar to EnvelopedData.
> 

Looks good

> > 29.  Security consideration on the cost benefits of using a generic vs
> > a specific error code.  Some specific codes might leak security
information.
> 
> Does the following text capture your point?
> 
>    In some situations, returning very detailed error information can
>    provide an attacker with insight into the security processing.  Where
>    this is a concern, the implementation should return the most generic
>    error code that is appropriate.  However, detailed error codes are
>    very helpful during development, debugging, and interoperability
>    testing.  For this reason, implementations may want to have a way to
>    configure the use of a generic error code or a detailed one.

Yes that addresses the issues that I was looking at

> 
> > 30.  Do you really want to make the KeyPkgVersion revised or is it a
> > totally independent thing?  As currently setup it will be a different
> > thing since it is in a different module.  If you want it to be a
> > change on what is in the original module then you need to re-publish
that
> module as well.
> 
> I do not see this as a problem.  The v2 value is not defined in RFC 6031,
> otherwise an IMPORT might be appropriate.
> 

Ok - no big deal.  

> Again, thanks very much for the review.

Welcome - jim

> 
> Russ