smime: NSSCMSRecipientKeyIdentifierTemplate template does not match the struct NSSCMSRecipientKeyIdentifierStr

NEW
Assigned to

Status

P2
normal
13 years ago
8 years ago

People

(Reporter: alvolkov.bgs, Assigned: alvolkov.bgs)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Assignee)

Description

13 years ago
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 .
(Assignee)

Updated

13 years ago
Assignee: wtchang → alexei.volkov.bugs
(Assignee)

Comment 1

13 years ago
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.

Adding Bob to ASN.1 encoder/decoder related bugs
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
Priority: -- → P2
Target Milestone: --- → 3.11
QA Contact: jason.m.reid → libraries
Target Milestone: 3.11 → 3.11.1
(Assignee)

Comment 4

12 years ago
Targeting to 3.12. Better to introduce binary incompatibility in new release, not in 3.11.x.
Target Milestone: 3.11.1 → 3.12
Unsetting target milestone in unresolved bugs whose targets have passed.
Target Milestone: 3.12 → ---
You need to log in before you can comment on or make changes to this bug.