303159 - smime: NSSCMSRecipientKeyIdentifierTemplate template does not match the struct NSSCMSRecipientKeyIdentifierStr

Status: NEW

(NSS :: Libraries, defect, P2)

Description

[Alexei Volkov]

nss/lib/smime/cmsasn1.c:328 :

const SEC_ASN1Template NSSCMSRecipientKeyIdentifierTemplate[] = {
    { SEC_ASN1_SEQUENCE,
          0, NULL, sizeof(NSSCMSRecipientKeyIdentifier) },
    { SEC_ASN1_OCTET_STRING,
          offsetof(NSSCMSRecipientKeyIdentifier,subjectKeyIdentifier) },
    { SEC_ASN1_OPTIONAL | SEC_ASN1_OCTET_STRING,
          offsetof(NSSCMSRecipientKeyIdentifier,date) },
    { SEC_ASN1_OPTIONAL | SEC_ASN1_OCTET_STRING,
          offsetof(NSSCMSRecipientKeyIdentifier,other) },
    { 0 }
};

nss/lib/smime/cmst.h:353 :

struct NSSCMSRecipientKeyIdentifierStr {
    SECItem *                           subjectKeyIdentifier;
    SECItem *                           date;                   /* optional */
    SECItem *                           other;                  /* optional */
};

typedef struct NSSCMSRecipientKeyIdentifierStr NSSCMSRecipientKeyIdentifier;

Decoder corrupts memory while doing the decoding.

I think here we should have pointers to octet strings template instead
of SEC_ASN1_OCTET_STRING.

--------------------------------------------------------------------------
Comments from Nelson Bolyard:

Based on my understanding of our ASN.1 encoders/decoders, it appears to me
that the template and the structure are not in agreement.  I believe that
this template corresponds to the following structure:

struct NSSCMSRecipientKeyIdentifierStr {
    SECItem     subjectKeyIdentifier;
    SECItem     date;                   /* optional */
    SECItem     other;                  /* optional */
};

We can either fix this bug by changing the structure to match the template
or the template to match the structure.   Either way, fixing it will
introduce a binary incompatibility.

I would personally prefer to fix the struct to match the template, since
the extra level of pointer indirection is utterly superfluous.  But that
may not be politically expedient.  Changing the structure will cause any
code that uses the structure to stop compiling, which will alarm people.
(Bureaucrats will not be at all alarmed that the code is so wrong, but
they will explode at the mere suggestion of changing a public structure.)

I think we should try to determine if any code anywhere uses that struct.
If not, then we could change the struct.  If code in mozilla or in Sun
products uses the struct, then we probably need to change the template to
match the struct.

------------------------------------------------------------------------
Comments from Julien:

I agree the template doesn't match the structure . This looks like a
bug. I don't understand how this bug wouldn't have been tripped by any
QA or application, though . It seems that it would probably cause memory
to be overwritten, unless perhaps the two optional fields are commonly
missing , and the application never uses the required field .

Comment 1

[Alexei Volkov]

The structure is used by smime interface function
NSS_CMSRecipientInfo_GetCertAndKey through a chain of calls.

Although the is not used for encoding/decoding directly in NSS
code(see comments to 304361), NSS users can potentially do so.

Changing template can create problems that can be discovered
as runtime only. 

By changing the structure, we will end up with code incompatibility
and break somebodies build.

Need to understand what are the products that using NSS smime, what is
a impact to each of them and identify the preferable way to fix the problem.

Also SEC_ASN1_CONTEXT_SPECIFIC | 0 and SEC_ASN1_CONTEXT_SPECIFIC | 1 tags
should be added to optional templates to avoid ambiguity.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Adding Bob to ASN.1 encoder/decoder related bugs

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Alexei, have you developed a patch for this bug?  Please attach it so it
can be reviewed.  This needs to be fixed in 3.11

Comment 4

[Alexei Volkov]

Targeting to 3.12. Better to introduce binary incompatibility in new release, not in 3.11.x.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Unsetting target milestone in unresolved bugs whose targets have passed.

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, and this bug has priority 'P2'.
:beurdouche, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 7

[Marco Castelluccio [:marco]]

We have modified the bot to only consider P1 as high priority, so I'm cancelling the needinfo here.