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

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Sailesh Davuluri]

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.

Comment 1

[Sailesh Davuluri]

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

Comment 2

[Wan-Teh Chang]

Bob, please take a look at this file.  Note the
behavior depends on whether the file's extension
is .p12 or .pfx.

Comment 3

[Wan-Teh Chang]

Moved to target milestone 3.8 because the original
NSS 3.7 release has been renamed 3.8.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Remove target milestone of 3.8, since these bugs didn't get into that release.

Comment 5

[Wan-Teh Chang]

Let's take care of this in NSS 3.9.

Comment 6

[Robert Relyea]

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

Comment 7

[Robert Relyea]

Attachment: 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 8

[Julien Pierre]

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.

Comment 9

[Robert Relyea]

Attachment: Attach the real patch, not a stall old patch

This should be the right one.

Comment 10

[Julien Pierre]

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.

Comment 11

[Robert Relyea]

Attachment: Incorporate Julian's comments for completeness.

Comment 12

[Robert Relyea]

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

Comment 13

[Wan-Teh Chang]

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.

Comment 14

[Wan-Teh Chang]

Bob, please reclose this bug after you have made
the changes I suggested.  Thanks!

Comment 15

[Robert Relyea]

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.

Comment 16

[Robert Relyea]

marking fixed