When I import a .p12 file and give a bad file password, I get an error SEC_ERROR_BAD_PASSWORD from the call to SEC_PKCS12DecoderVerify(). But when I do the same for a .pfx file I get a different error code.
I think the error numbers were -8177 for a .p12 file and -8190 for a .pfx file.
Bob, please take a look at this file. Note the behavior depends on whether the file's extension is .p12 or .pfx.
Priority: -- → P2
Target Milestone: --- → 3.6
Moved to target milestone 3.8 because the original NSS 3.7 release has been renamed 3.8.
Target Milestone: 3.7 → 3.8
Remove target milestone of 3.8, since these bugs didn't get into that release.
Target Milestone: 3.8 → ---
Let's take care of this in NSS 3.9.
Target Milestone: --- → 3.9
This problem may require a rewrite of the pk12 subsystem. I took an initial look at this before, and it's non-trivial. We can do it in NSS 3.9, I just want to set the proper expectation about what the cost is. bob
Created attachment 133233 [details] [diff] [review] When we fail to decode based on a bad password, don't continue. So once we've tried failed to decode a ANS.1 stream, don't continue collecting more data. On microsoft.pfx files, we would wind up decoding to the end of the encrypted stream, then fail in the padding in PKCS #7. This code bypasses this problem by making sure we don't continue to try to decode data once we've hit a bad password failure. A smaller fix would be to check to see if the SEC_Error has already been set to BAD_PASSWORD in SEC_PKCS7Decode() when we detect a bad padding, and not resetting the PORT_Error(). This fix would not handle any cases where there are only places where we may be failing the decode and resetting the error code.
Comment on attachment 133233 [details] [diff] [review] When we fail to decode based on a bad password, don't continue. This looks like an unrelated modutil patch.
Attachment #133233 - Flags: superreview?(jpierre) → superreview-
Created attachment 133453 [details] [diff] [review] Attach the real patch, not a stall old patch This should be the right one.
Attachment #133233 - Attachment is obsolete: true
Comment on attachment 133453 [details] [diff] [review] Attach the real patch, not a stall old patch Looks good, except you should check for NULL context pointers in your abort functions.
Attachment #133453 - Flags: review?(jpierre) → review+
Checking in nss/nss.def; /cvsroot/mozilla/security/nss/lib/nss/nss.def,v <-- nss.def new revision: 1.115; previous revision: 1.114 done Checking in pkcs12/p12d.c; /cvsroot/mozilla/security/nss/lib/pkcs12/p12d.c,v <-- p12d.c new revision: 1.20; previous revision: 1.19 done Checking in pkcs7/p7decode.c; /cvsroot/mozilla/security/nss/lib/pkcs7/p7decode.c,v <-- p7decode.c new revision: 1.12; previous revision: 1.11 done Checking in pkcs7/p7encode.c; /cvsroot/mozilla/security/nss/lib/pkcs7/p7encode.c,v <-- p7encode.c new revision: 1.7; previous revision: 1.6 done Checking in pkcs7/secpkcs7.h; /cvsroot/mozilla/security/nss/lib/pkcs7/secpkcs7.h,v <-- secpkcs7.h new revision: 1.4; previous revision: 1.3 done Checking in smime/smime.def; /cvsroot/mozilla/security/nss/lib/smime/smime.def,v <-- smime.def new revision: 1.29; previous revision: 1.28 done Checking in util/secasn1.h; /cvsroot/mozilla/security/nss/lib/util/secasn1.h,v <-- secasn1.h new revision: 1.10; previous revision: 1.9 done Checking in util/secasn1d.c; /cvsroot/mozilla/security/nss/lib/util/secasn1d.c,v <-- secasn1d.c new revision: 1.28; previous revision: 1.27 done Checking in util/secasn1e.c; /cvsroot/mozilla/security/nss/lib/util/secasn1e.c,v <-- secasn1e.c new revision: 1.12; previous revision: 1.11 done
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
Comment on attachment 133453 [details] [diff] [review] Attach the real patch, not a stall old patch r=wtc. This patch is basically good. It has some minor problems that I'd like you to fix. I left my comments on a copy of this patch (rather the next patch) in your cube.
Attachment #133453 - Flags: superreview?(wchang0222) → superreview+
Bob, please reclose this bug after you have made the changes I suggested. Thanks!
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
wtc changes have been checked in. Julien, the check for ->cx == NULL isn't necessary since the underlying call was doing the check. The whole thing is mute since wtc's changes was to remove the null checks since they are really programming errors and should crash.
Status: REOPENED → RESOLVED
Last Resolved: 15 years ago → 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.