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

When we do a file import and give a bad password we get wrong errors back

RESOLVED FIXED in 3.9

Status

NSS
Libraries
P2
normal
RESOLVED FIXED
15 years ago
14 years ago

People

(Reporter: Sailesh Davuluri, Assigned: Robert Relyea)

Tracking

unspecified
x86
Windows 2000

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

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

Comment 1

15 years ago
I think the error numbers were -8177 for a .p12 file and -8190 for a .pfx file.

Updated

15 years ago
Blocks: 122869

Comment 2

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

Updated

15 years ago
Target Milestone: 3.6 → 3.7

Comment 3

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

Comment 5

14 years ago
Let's take care of this in NSS 3.9.
Target Milestone: --- → 3.9
(Assignee)

Comment 6

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

Comment 7

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

Updated

14 years ago
Attachment #133233 - Flags: superreview?(jpierre)
Attachment #133233 - Flags: review?(wchang0222)

Comment 8

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

Comment 9

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

Updated

14 years ago
Attachment #133453 - Flags: superreview?(wchang0222)
Attachment #133453 - Flags: review?(jpierre)
(Assignee)

Updated

14 years ago
Attachment #133233 - Flags: review?(wchang0222)

Comment 10

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

Comment 11

14 years ago
Created attachment 133458 [details] [diff] [review]
Incorporate Julian's comments for completeness.
(Assignee)

Comment 12

14 years ago
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: 14 years ago
Resolution: --- → FIXED

Comment 13

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

Comment 14

14 years ago
Bob, please reclose this bug after you have made
the changes I suggested.  Thanks!
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 15

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

Comment 16

14 years ago
marking fixed
Status: REOPENED → RESOLVED
Last Resolved: 14 years ago14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.