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

NSS_CMSSignedData_GetDigestValue and NSS_CMSSignedData_GetDigestByAlgTag are duplicates

RESOLVED FIXED in 3.9

Status

NSS
Libraries
RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: Wan-Teh Chang, Assigned: Wan-Teh Chang)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

1.85 KB, patch
Robert Relyea
: review+
Nelson Bolyard (seldom reads bugmail)
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

14 years ago
The functions NSS_CMSSignedData_GetDigestValue and
NSS_CMSSignedData_GetDigestByAlgTag are duplicates.
Neither is exported from the NSS S/MIME library, so
we can simply delete one.

I think it is better to delete the function
NSS_CMSSignedData_GetDigestValue because it is not
being used, but use its code because it has better
error checking (it tests for a null sigd->digestAlgorithms
and a -1 return value from NSS_CMSAlgArray_GetIndexByAlgTag).
(Assignee)

Comment 1

14 years ago
Created attachment 136376 [details] [diff] [review]
Proposed patch

Upon closer inspection, I think it is better to make
NSS_CMSSignedData_GetDigestValue the surviving function
because it has a corresponding NSS_CMSSignedData_SetDigestValue,
which is exported.

This patch deletes NSS_CMSSignedData_GetDigestByAlgTag
and replaces it by NSS_CMSSignedData_GetDigestValue.
Note that NSS_CMSSignedData_GetDigestValue already handles
errors correctly.  Its only problem is that it doesn't set
error code, which I noted with a comment.
(Assignee)

Updated

14 years ago
Attachment #136376 - Flags: superreview?(MisterSSL)
Attachment #136376 - Flags: review?(rrelyea0264)

Comment 2

14 years ago
Comment on attachment 136376 [details] [diff] [review]
Proposed patch

The fix isn't quite complete. The caller of NSS_CMSSignedDAta_GetDigestValue is
not expecting a failure.

Either NSS_CMSSignedData_GetDigestValue
 or its caller should set
 SEC_ERROR_DIGEST_NOT_FOUND

Other than these two issues, the patch is fine.
Attachment #136376 - Flags: review?(rrelyea0264) → review-
(Assignee)

Updated

14 years ago
Attachment #136376 - Flags: superreview?(MisterSSL)
(Assignee)

Comment 3

14 years ago
Created attachment 136385 [details] [diff] [review]
Proposed patch v1.1

This is the same patch, but the code it patches has
just changed.

It shows that the caller of NSS_CMSSignedData_GetDigestValue
is expecting a failure and sets the error code
SEC_ERROR_PKCS7_BAD_SIGNATURE.	The error code is not what
Bob suggested (SEC_ERROR_DIGEST_NOT_FOUND), but that's the
subject of Bugscape bug 54057 comments 3 and 4.
Attachment #136376 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #136385 - Flags: superreview?(MisterSSL)
Attachment #136385 - Flags: review?(rrelyea0264)
Attachment #136385 - Flags: superreview?(MisterSSL) → superreview+
(Assignee)

Comment 4

14 years ago
Fix checked in on the NSS trunk (NSS 3.9).

Checking in cms.h;
/cvsroot/mozilla/security/nss/lib/smime/cms.h,v  <--  cms.h
new revision: 1.18; previous revision: 1.17
done
Checking in cmssigdata.c;
/cvsroot/mozilla/security/nss/lib/smime/cmssigdata.c,v  <--  cmssigdata.c
new revision: 1.20; previous revision: 1.19
done
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.9

Comment 5

14 years ago
Comment on attachment 136385 [details] [diff] [review]
Proposed patch v1.1

r=relyea
Attachment #136385 - Flags: review?(rrelyea0264) → review+
You need to log in before you can comment on or make changes to this bug.