If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

CMS does not allow content types other than S/MIME

RESOLVED FIXED in 3.12.10

Status

NSS
Libraries
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Robert Relyea, Assigned: Robert Relyea)

Tracking

3.12.6
3.12.10

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 6 obsolete attachments)

(Assignee)

Description

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

Comment 1

7 years ago
Created attachment 462592 [details] [diff] [review]
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.
(Assignee)

Updated

7 years ago
Assignee: nobody → rrelyea
(Assignee)

Comment 2

7 years ago
Created attachment 480979 [details] [diff] [review]
V2: This patch adds the ability to handle new content types.

Remove stuff in cmd that doesn't apply. Include cmsudf.c (new file).
Attachment #462592 - Attachment is obsolete: true
(Assignee)

Comment 3

7 years ago
Created attachment 502962 [details] [diff] [review]
Bob's patch updated with Nalin's changes.
Attachment #480979 - Attachment is obsolete: true
(Assignee)

Comment 4

7 years ago
Created attachment 506912 [details] [diff] [review]
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.
(Assignee)

Comment 5

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

r+ rrelyea

other reviewers welcome.
Attachment #506912 - Flags: review+
(Assignee)

Comment 6

7 years ago
Created attachment 506925 [details] [diff] [review]
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
Attachment #502962 - Attachment is obsolete: true
Attachment #506925 - Flags: review?(emaldona)
Attachment #506925 - Flags: feedback?(wtc)

Comment 7

7 years ago
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?
Attachment #506925 - Flags: review?(emaldona) → review-
(Assignee)

Comment 8

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

yes it should. good catch.
(Assignee)

Comment 9

7 years ago
Created attachment 507160 [details] [diff] [review]
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.
Attachment #506925 - Attachment is obsolete: true
Attachment #507160 - Flags: review?(emaldona)
Attachment #507160 - Flags: feedback?(wtc)
Attachment #506925 - Flags: feedback?(wtc)

Comment 10

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

7 years ago
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.
Attachment #507160 - Flags: review?(emaldona) → review-
(Assignee)

Comment 12

7 years ago
> >+ * 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;).
(Assignee)

Comment 13

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

Include elio's comments.
Attachment #507160 - Attachment is obsolete: true
Attachment #507923 - Flags: review?(emaldona)
Attachment #507160 - Flags: feedback?(wtc)

Updated

7 years ago
Attachment #507923 - Flags: review?(emaldona) → review+
(Assignee)

Comment 14

7 years ago
Created attachment 508022 [details] [diff] [review]
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
Attachment #508022 - Flags: review?(rrelyea)
(Assignee)

Comment 15

7 years ago
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.
Attachment #508022 - Flags: review?(rrelyea) → review-
(Assignee)

Comment 16

7 years ago
Created attachment 508033 [details] [diff] [review]
Nalin's changes on top of my patch + fixes to my review comments.
Attachment #508022 - Attachment is obsolete: true
Attachment #508033 - Flags: superreview?(emaldona)
Attachment #508033 - Flags: review?(rrelyea)
(Assignee)

Comment 17

7 years ago
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.
Attachment #508033 - Flags: review?(rrelyea) → review+
(Assignee)

Updated

7 years ago
Blocks: 491918
(Assignee)

Comment 18

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

Updated

7 years ago
Attachment #508033 - Flags: superreview?(emaldona) → superreview+
(Assignee)

Comment 19

7 years ago
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
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Comment 20

7 years ago
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
Target Milestone: --- → 3.12.10
(Assignee)

Comment 21

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

bob
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

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

7 years ago
There is a patch submitted with Bug 632439 that renames the field to privateInfo.
You need to log in before you can comment on or make changes to this bug.