584224 - CMS does not allow content types other than S/MIME

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Robert Relyea]

Other protocols, like kerberos, use the CMS spec for wrapping their data. Some of the define content types which aren't used in S/MIME. CMS needs to support those new content types.

Comment 1

[Robert Relyea]

Attachment: This patch adds the ability to handle new content types.

This patch consists of the following:
   1) A new content type called 'Generic' which can represent any application defined content type.
   2) A set of functions that act as accessors for Generic content.
   3) Two crypto context values have been moved to a opaque private member.
   4) new functions to register new content types.

Comment 2

[Robert Relyea]

Attachment: V2: This patch adds the ability to handle new content types.

Remove stuff in cmd that doesn't apply. Include cmsudf.c (new file).

Comment 3

[Robert Relyea]

Attachment: Bob's patch updated with Nalin's changes.

Comment 4

[Robert Relyea]

Attachment: Patch from Nalin

This patch updates the getkeytype function in cryptohi/seckey.c to accept algids as well as just algorithms in returning the key type.

Comment 5

[Robert Relyea]

Comment on attachment 506912 [details] [diff] [review]
Patch from Nalin

r+ rrelyea

other reviewers welcome.

Comment 6

[Robert Relyea]

Attachment: Bob's patch updated with Nalin's changes. -formated propperly

This patch is fairly large. If you feel like reviewing it wtc, then I'd be happy to get your review. Otherwise just comment on a couple on thing:

Unfortunately we did not keep the SMIME datastructures private when we created them. I've taken 2 fields that are extremely unlikely to be referenced by an application (crypto and hash contexts), and used those fields to hang a private data structure off of. The contexts have been moved to this datastructure. I just want a santity check that this seems reasonable.

bob

Comment 7

[Elio Maldonado]

Comment on attachment 506925 [details] [diff] [review]
Bob's patch updated with Nalin's changes. -formated propperly

In manifest.mn a source file is added, "+	cmsudf.c"
but he cmsudf.c file is missing from the patch.
How come in smime.def has
+;+NSS_3.12.7 {   # NSS 3.12.7 release
for the new calls? Shouldn't it be NSS 3.12.10?

Comment 8

[Robert Relyea]

+ for the new calls? Shouldn't it be NSS 3.12.10?

yes it should. good catch.

Comment 9

[Robert Relyea]

Attachment: V2: Bob's patch updated with Nalin's changes. -formated propperly, include udf and fix smime.def value

Patch it Elio's first round of comments....

wtc, same request as last time.

interdiff should work on this patch.

Comment 10

[Elio Maldonado]

Comment on attachment 507160 [details] [diff] [review]
V2: Bob's patch updated with Nalin's changes. -formated propperly, include udf and fix smime.def value

Partial review, what have so far:

lib/cryptohi/seckey.c - Okay
lib/smime/

cms.h
>+ * turn off streaming for this content type.
>+ */
>+extern SECStatus
>+NSS_CMSContentInfo_SetDontStream(NSSCMSContentInfo *cinfo, PRBool dontStream);
>+
You should expand the comment to spare the developer too much code inspection.
Remind the caller to check PORT_GetError() on failure. From inspection I see that
it failure is due to a memory alocation failure in which case the PR errode
gets set to SEC_ERROR_NO_MEMORY.

cmsasn1.c, cmscinfo.c, cmsencdata.c, and cmslocal.h:  I'll do next

cmsudf.c:
>+static PLHashNumber
>+nss_cmstype_hash_tag(const void *key)
>+{
>+   return (PLHashNumber) key;
>+}
cast from pointer to integer of different size
On plhash.h I see typedef PRUint32 PLHashNumber;

>+static PRIntn
>+nss_cmstype_compare_values(const void *v1, const void *v2)
>+{
>+   PLHashNumber value1 = (PLHashNumber) v1; 
cast from pointer to integer of different size" warnings, as above

>+   PLHashNumber value2 = (PLHashNumber) v2; 
cast from pointer to integer of different size" warnings, as above
>+
>+   return (value1 == value2);
>+}


>+const SEC_ASN1Template *
>+NSS_CMSType_GetTemplate(SECOidTag type)
>+{
>+    const nsscmstypeInfo *typeInfo = nss_cmstype_lookup(type);
>+
>+    if (typeInfo && typeInfo->template) {
>+	return typeInfo->template;
>+    }
>+
>+}
control reaches end of non-void function
should it return NULL if the condition isn't met?


>+
>+void
>+NSS_CMSGenericWrapperData_Destroy(SECOidTag type, NSSCMSGenericWrapperData *gd)
>+{
>+    const nsscmstypeInfo *typeInfo = nss_cmstype_lookup(type);
>+
>+    if (typeInfo && typeInfo->destroy) {
>+	(*typeInfo->destroy)(gd);
>+    }
>+    
>+}
another control reaches end of non-void function
>+
>+
>+SECStatus

>+
>+SECStatus
>+NSS_CMSGenericWrapperData_Decode_AfterEnd(SECOidTag type, 
>+				   NSSCMSGenericWrapperData *gd)
>+{
>+    const nsscmstypeInfo *typeInfo;
>+
>+    /* short cut common case */
>+    if (type == SEC_OID_PKCS7_DATA) {
>+	return SECSuccess;
>+    }
>+
>+    typeInfo = nss_cmstype_lookup(type);
>+    if (typeInfo) {
>+	if  (typeInfo->decode_end) {
>+	    return (*typeInfo->decode_end)(gd);
>+	}
>+	/* decoder ops optional for data tags */
>+	if (typeInfo->isData) {
>+	    return SECSuccess;
>+	}
>+    }
>+    /* expected a function, but none existed */
>+}
control reaches end of non-void function
Maybe you want to return SECFailure as you did in similar functions

cmsutil.c - coming next
manifest.mn -- okay
smime.def -- okay

I'll do the remaining ones next.

Comment 11

[Elio Maldonado]

Comment on attachment 507160 [details] [diff] [review]
V2: Bob's patch updated with Nalin's changes. -formated propperly, include udf and fix smime.def value

The remaining ones looked okay. I'm giving an r- for the reasons mentioned earlier.

Comment 12

[Robert Relyea]

> >+ * turn off streaming for this content type.
> >+ */
> >+extern SECStatus
> >+NSS_CMSContentInfo_SetDontStream(NSSCMSContentInfo *cinfo, PRBool dontStream);
> >+
> You should expand the comment to spare the developer too much code inspection.
> Remind the caller to check PORT_GetError() on failure. From inspection I see
that
> it failure is due to a memory alocation failure in which case the PR errode
gets set to SEC_ERROR_NO_MEMORY.

OK I'll comment on failure cases for this function.

> cast from pointer to integer of different size
> On plhash.h I see typedef PRUint32 PLHashNumber;

Yes, PLHashNumber is the hash table index, which is 32 bits (and can get even smaller internally).

The requirement for PLHashNumber is that colisions are rare, not that there are never a collision (more than one entry can be found in a single bucket.

>>+static PRIntn
>>+nss_cmstype_compare_values(const void *v1, const void *v2)
>>+{
>>+   PLHashNumber value1 = (PLHashNumber) v1; 
>cast from pointer to integer of different size" warnings, as above

This function is misnamed I'll correct the name. It's supposed to compare the keys for v1 and v2, so the trucation is intentional. (It needs to be the same truncation as nss_cmstype_hash_tag

>>+const SEC_ASN1Template *
>>+NSS_CMSType_GetTemplate(SECOidTag type)
>control reaches end of non-void function
>should it return NULL if the condition isn't met?

Good catch

>>+SECStatus
>>+NSS_CMSGenericWrapperData_Decode_AfterEnd(SECOidTag type, 
>>+				   NSSCMSGenericWrapperData *gd)

Good catch

>>+void
>>+NSS_CMSGenericWrapperData_Destroy(SECOidTag type, NSSCMSGenericWrapperData *gd)
>another control reaches end of non-void function

This one is void;).

Comment 13

[Robert Relyea]

Attachment: V3: Bob's patch updated with Nalin's changes. -formated propperly, include udf and fix smime.def value

Include elio's comments.

Comment 14

[Robert Relyea]

Attachment: Nalin's changes on top of my patch

Pure Nalin patch on top of the patch I created.

* expose an NSSCMSGenericWrapperDataTemplate that includes the ContentInfo
* properly return plain data from NSS_CMSContentInfo_GetContent
* fix what appear to be some copy/paste errors
* get pointers to pointers to functions out of the interface, to match the rest
  of the CMS API
* correctly add new types to the type hash
* don't return CMS contents from NULL CMS messages

Comment 15

[Robert Relyea]

Comment on attachment 508022 [details] [diff] [review]
Nalin's changes on top of my patch

r-

Most of the patch looks good. The only issue is the exported data Template. It's exported in a non-portable way.

Comment 16

[Robert Relyea]

Attachment: Nalin's changes on top of my patch + fixes to my review comments.

Comment 17

[Robert Relyea]

Comment on attachment 508033 [details] [diff] [review]
Nalin's changes on top of my patch + fixes to my review comments.

r+ pending elio's review of my changes to this patch.

Comment 18

[Robert Relyea]

Trunk:

bobs-laptop(417) cvs commit cryptohi/seckey.c
Checking in cryptohi/seckey.c;
/cvsroot/mozilla/security/nss/lib/cryptohi/seckey.c,v  <--  seckey.c
new revision: 1.59; previous revision: 1.58
done
bobs-laptop(418) cvs commit
Checking in smime/cms.h;
/cvsroot/mozilla/security/nss/lib/smime/cms.h,v  <--  cms.h
new revision: 1.24; previous revision: 1.23
done
Checking in smime/cmsasn1.c;
/cvsroot/mozilla/security/nss/lib/smime/cmsasn1.c,v  <--  cmsasn1.c
new revision: 1.8; previous revision: 1.7
done
Checking in smime/cmscinfo.c;
/cvsroot/mozilla/security/nss/lib/smime/cmscinfo.c,v  <--  cmscinfo.c
new revision: 1.8; previous revision: 1.7
done
Checking in smime/cmsdecode.c;
/cvsroot/mozilla/security/nss/lib/smime/cmsdecode.c,v  <--  cmsdecode.c
new revision: 1.11; previous revision: 1.10
done
Checking in smime/cmsdigdata.c;
/cvsroot/mozilla/security/nss/lib/smime/cmsdigdata.c,v  <--  cmsdigdata.c
new revision: 1.6; previous revision: 1.5
done
Checking in smime/cmsencdata.c;
/cvsroot/mozilla/security/nss/lib/smime/cmsencdata.c,v  <--  cmsencdata.c
new revision: 1.12; previous revision: 1.11
done
Checking in smime/cmsencode.c;
/cvsroot/mozilla/security/nss/lib/smime/cmsencode.c,v  <--  cmsencode.c
new revision: 1.8; previous revision: 1.7
done
Checking in smime/cmsenvdata.c;
/cvsroot/mozilla/security/nss/lib/smime/cmsenvdata.c,v  <--  cmsenvdata.c
new revision: 1.12; previous revision: 1.11
done
Checking in smime/cmslocal.h;
/cvsroot/mozilla/security/nss/lib/smime/cmslocal.h,v  <--  cmslocal.h
new revision: 1.6; previous revision: 1.5
done
Checking in smime/cmsmessage.c;
/cvsroot/mozilla/security/nss/lib/smime/cmsmessage.c,v  <--  cmsmessage.c
new revision: 1.7; previous revision: 1.6
done
Checking in smime/cmssigdata.c;
/cvsroot/mozilla/security/nss/lib/smime/cmssigdata.c,v  <--  cmssigdata.c
new revision: 1.30; previous revision: 1.29
done
Checking in smime/cmst.h;
/cvsroot/mozilla/security/nss/lib/smime/cmst.h,v  <--  cmst.h
new revision: 1.11; previous revision: 1.10
done
RCS file: /cvsroot/mozilla/security/nss/lib/smime/cmsudf.c,v
done
Checking in smime/cmsudf.c;
/cvsroot/mozilla/security/nss/lib/smime/cmsudf.c,v  <--  cmsudf.c
initial revision: 1.1
done
Checking in smime/cmsutil.c;
/cvsroot/mozilla/security/nss/lib/smime/cmsutil.c,v  <--  cmsutil.c
new revision: 1.16; previous revision: 1.15
done
Checking in smime/manifest.mn;
/cvsroot/mozilla/security/nss/lib/smime/manifest.mn,v  <--  manifest.mn
new revision: 1.11; previous revision: 1.10
done
Checking in smime/smime.def;
/cvsroot/mozilla/security/nss/lib/smime/smime.def,v  <--  smime.def
new revision: 1.36; previous revision: 1.35
done

Comment 19

[Robert Relyea]

Trunk:

Checking in smime/cms.h;
/cvsroot/mozilla/security/nss/lib/smime/cms.h,v  <--  cms.h
new revision: 1.25; previous revision: 1.24
done
Checking in smime/cmsasn1.c;
/cvsroot/mozilla/security/nss/lib/smime/cmsasn1.c,v  <--  cmsasn1.c
new revision: 1.9; previous revision: 1.8
done
Checking in smime/cmscinfo.c;
/cvsroot/mozilla/security/nss/lib/smime/cmscinfo.c,v  <--  cmscinfo.c
new revision: 1.9; previous revision: 1.8
done
Checking in smime/cmsencode.c;
/cvsroot/mozilla/security/nss/lib/smime/cmsencode.c,v  <--  cmsencode.c
new revision: 1.9; previous revision: 1.8
done
Checking in smime/cmst.h;
/cvsroot/mozilla/security/nss/lib/smime/cmst.h,v  <--  cmst.h
new revision: 1.12; previous revision: 1.11
done
Checking in smime/cmsudf.c;
/cvsroot/mozilla/security/nss/lib/smime/cmsudf.c,v  <--  cmsudf.c
new revision: 1.2; previous revision: 1.1
done
Checking in smime/smime.def;
/cvsroot/mozilla/security/nss/lib/smime/smime.def,v  <--  smime.def
new revision: 1.37; previous revision: 1.36
done


branch:

hecking in lib/smime/cms.h;
/cvsroot/mozilla/security/nss/lib/smime/cms.h,v  <--  cms.h
new revision: 1.23.2.2; previous revision: 1.23.2.1
done
Checking in lib/smime/cmsasn1.c;
/cvsroot/mozilla/security/nss/lib/smime/cmsasn1.c,v  <--  cmsasn1.c
new revision: 1.7.2.2; previous revision: 1.7.2.1
done
Checking in lib/smime/cmscinfo.c;
/cvsroot/mozilla/security/nss/lib/smime/cmscinfo.c,v  <--  cmscinfo.c
new revision: 1.7.192.2; previous revision: 1.7.192.1
done
Checking in lib/smime/cmsencode.c;
/cvsroot/mozilla/security/nss/lib/smime/cmsencode.c,v  <--  cmsencode.c
new revision: 1.6.66.3; previous revision: 1.6.66.2
done
Checking in lib/smime/cmst.h;
/cvsroot/mozilla/security/nss/lib/smime/cmst.h,v  <--  cmst.h
new revision: 1.10.142.2; previous revision: 1.10.142.1
done
Checking in lib/smime/cmsudf.c;
/cvsroot/mozilla/security/nss/lib/smime/cmsudf.c,v  <--  cmsudf.c
new revision: 1.1.2.3; previous revision: 1.1.2.2
done
Checking in lib/smime/smime.def;
/cvsroot/mozilla/security/nss/lib/smime/smime.def,v  <--  smime.def
new revision: 1.35.32.2; previous revision: 1.35.32.1
done

Comment 20

[Wan-Teh Chang]

The patches in this bug seem to have caused cmsutil to crash on several NSS tinderboxes.
Here is an example:

cmsutil -D -i alice.env -d ../bobdir -p nss -o alice.data1
./all.sh: line 289: 13869 Segmentation fault      (core dumped) ${PROFTOOL} ${BINDIR}/cmsutil -D -i alice.env -d ${P_R_BOBDIR} -p nss -o alice.data1
smime.sh: #882: Decode Enveloped Data Alice . - Core file is detected - FAILED

Comment 21

[Robert Relyea]

More forensics have determined that it was the patch in bug 592489 which is causing the problems.

bob

Comment 22

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 507923 [details] [diff] [review]
V3: Bob's patch updated with Nalin's changes. -formated propperly, include udf and fix smime.def value

>Index: smime/cmst.h

>@@ -164,8 +174,8 @@ struct NSSCMSContentInfoStr {
> 							 * (only used by creation code) */
>     SECOidTag			contentEncAlgTag;	/* oid tag of encryption algorithm
> 							 * (only used by creation code) */
>-    NSSCMSCipherContext		*ciphcx;		/* context for en/decryption going on */
>-    NSSCMSDigestContext		*digcx;			/* context for digesting going on */
>+    NSSCMSContentInfoPrivate	*private;		/* place for NSS private info */

private is reserved in C++, so any C++ code that includes this header, such as Firefox, would fail to compile.  See https://bugzilla.redhat.com/676036

Comment 23

[Wan-Teh Chang]

Ah, yes.  In NSPR we had to name such a field 'secret' to avoid this problem.
So we'll need to rename the 'private' field of struct NSSCMSContentInfoStr.

Comment 24

[Elio Maldonado]

There is a patch submitted with Bug 632439 that renames the field to privateInfo.