Closed Bug 1399867 Opened 3 years ago Closed 3 years ago

pk12util: add backward compatibility with old NSS produced files

Categories

(NSS :: Tools, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ueno, Assigned: ueno)

Details

Attachments

(4 files, 3 obsolete files)

We recently changed the encoding of passwords used in PKCS #12, when the PBE is PBES2 (PKCS #5).  This matches the guideline:
https://tools.ietf.org/html/draft-mavrogiannopoulos-pkcs5-passwords-01

However, it turned out that some files created with older NSS releases cannot be read with the newer NSS releases (>= 3.30).  OpenSSL also seems to have had a similar issue in the past and added a workaround, which tries old-style password encoding if the new one is not accepted.  The attached patches simulates the behavior.
This adds a new private option __NSS_PKCS12_DECODE_FORCE_UNICODE to enforce the old behavior of passwords encoding, i.e. all passwords are encoded in UTF-16.

The value of the option is read upon SEC_PKCS12DecoderStart and SEC_PKCS12DecoderImportBags.  Bob pointed that it is not thread-safe, but I haven't figured out any other way to tell the decoder to treat passwords in a different encoding, without adding a new API.
Attachment #8908124 - Flags: review?(rrelyea)
This makes pk12util -i accepts the files created with pk12util -e -c <PBES2 mech> from old NSS releases.
Attachment #8908126 - Flags: review?(rrelyea)
This makes pk12util -i accepts the files created with pk12util -e -C <PBES2 mech> from old NSS releases.
Attachment #8908128 - Flags: review?(rrelyea)
Test case for the change, with a PKCS #12 file created with NSS 3.21.
Attachment #8908130 - Flags: review?(rrelyea)
Comment on attachment 8908124 [details] [diff] [review]
0001-pkcs12-Add-a-compat-option-for-password-encoding.patch

Review of attachment 8908124 [details] [diff] [review]:
-----------------------------------------------------------------

Please also get feedback on these patches from mozilla.

These patches have non-treadsafe behavior. I think we may already have that same behavior in pkcs12 already, but I'm not sure. In any case this part of the patch is fine. It's good to give users the option of changing the underlying behavior of the pbe.
Attachment #8908124 - Flags: review?(rrelyea) → review+
OK, I'm incorrect. The majority of this patch is in pk12util, which is a stand alone application, thus the thread safety issue isn't inherent in the code. Applications may have to decide how to deal with it in their code.
Comment on attachment 8908126 [details] [diff] [review]
0002-pk12util-Make-c-to-try-old-password-encoding-if-fail.patch

Review of attachment 8908126 [details] [diff] [review]:
-----------------------------------------------------------------

I have some suggestions. You can answer the suggestions and rerequest a review or you can incorporate the suggestions in a new patch.

bob

::: cmd/pk12util/pk12util.c
@@ +482,2 @@
>          return rv;
>      }

This code assumes that __NSS_PKCS12_DECODE_FORCE_UNICODE is set to false on entry. That's sort of fine except it could break if someone sets it by policy to true in the future. We probably should check it's value here and do something reasonable.

So the easy thing is to read the value of forceUnicode from NSS to start with. That will cause your loop to execute once with forceUnicode set to true (which presumably would be what setting the policy would intend.

@@ +529,5 @@
> +        }
> +    } while (trypw);
> +
> +    /* revert the option setting */
> +    if (forceUnicode) {

If forceUnicode was read as true (as per my suggestion), you don't want to reset it here. So you'll need a flag to detect that case.
Attachment #8908126 - Flags: review?(rrelyea) → review-
Comment on attachment 8908128 [details] [diff] [review]
0003-pk12util-Make-C-to-try-old-password-encoding-if-fail.patch

Review of attachment 8908128 [details] [diff] [review]:
-----------------------------------------------------------------

Same deal as your other patch. I r-'ed

::: cmd/pk12util/pk12util.c
@@ +361,3 @@
>      PRBool trypw;
>      int error;
>  

This as the same issue ad described in your other pkcs11util patch
Attachment #8908128 - Flags: review?(rrelyea) → review-
Comment on attachment 8908130 [details] [diff] [review]
0004-tests-Add-test-for-reading-PKCS-12-files-created-wit.patch

Review of attachment 8908130 [details] [diff] [review]:
-----------------------------------------------------------------

r+ but don't submit it until the other PKCS #12 patches are approved.
Attachment #8908130 - Flags: review?(rrelyea) → review+
Read __NSS_PKCS12_DECODE_FORCE_UNICODE at startup, and revert to the default value if we set the option internally.
Attachment #8908126 - Attachment is obsolete: true
Attachment #8908513 - Flags: review?(rrelyea)
Same as the -c patch; revert to the default value PR_FALSE.
Attachment #8908128 - Attachment is obsolete: true
Attachment #8908514 - Flags: review?(rrelyea)
(In reply to Robert Relyea from comment #7)

> This code assumes that __NSS_PKCS12_DECODE_FORCE_UNICODE is set to false on
> entry. That's sort of fine except it could break if someone sets it by
> policy to true in the future. We probably should check it's value here and
> do something reasonable.
> 
> So the easy thing is to read the value of forceUnicode from NSS to start
> with. That will cause your loop to execute once with forceUnicode set to
> true (which presumably would be what setting the policy would intend.

Thank you for the review, it's a good point.  I have updated the patches.
Would it make sense to have an envvar or a command-line option for this, so user can set the option explicitly?

(In reply to Robert Relyea from comment #5)

> Please also get feedback on these patches from mozilla.

I remember you suggested to ask Martin about it.  Martin, could you take a look at the patches?
Flags: needinfo?(martin.thomson)
Comment on attachment 8908513 [details] [diff] [review]
0002-pk12util-Make-c-try-different-password-encoding-if-f.patch

Review of attachment 8908513 [details] [diff] [review]:
-----------------------------------------------------------------

r+ not the way I was thinking, but it does what I asked. Thanks!
Attachment #8908513 - Flags: review?(rrelyea) → review+
Comment on attachment 8908514 [details] [diff] [review]
0003-pk12util-Make-C-try-different-password-encoding-if-f.patch

Review of attachment 8908514 [details] [diff] [review]:
-----------------------------------------------------------------

r+ This patch depends on the previous patch
Attachment #8908514 - Flags: review?(rrelyea) → review+
I ran a try build with these 4 patches, and apparently there's a memory leak, could you please have a look?

==1603==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 16 byte(s) in 1 object(s) allocated from:
    #0 0x4c44b3 in calloc /scratch/llvm/clang-4/xenial/final/llvm.src/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:74:3
    #1 0x7f0e1f6fe477 in PR_Calloc /home/worker/nspr/Debug/pr/src/malloc/../../../../pr/src/malloc/prmem.c:443:40
    #2 0x7f0e20a16f91 in PORT_ZAlloc_Util /home/worker/nss/out/Debug/../../lib/util/secport.c:116:14
    #3 0x4f13b8 in P12U_UnicodeConversion /home/worker/nss/out/Debug/../../cmd/pk12util/pk12util.c:258:22
    #4 0x4f1ecc in p12U_ReadPKCS12File /home/worker/nss/out/Debug/../../cmd/pk12util/pk12util.c:383:9
    #5 0x4f2e0e in P12U_ImportPKCS12Object /home/worker/nss/out/Debug/../../cmd/pk12util/pk12util.c:511:18
    #6 0x4f6d22 in main /home/worker/nss/out/Debug/../../cmd/pk12util/pk12util.c:1135:9
    #7 0x7f0e1fe1282f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)

https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=b3ade1b79b524bdbb5817a30ce4d583ef36cd9e4&selectedJob=132021758
Assignee: nobody → dueno
Flags: needinfo?(dueno)
Oops, obviously I didn't run nss-try build when submitting this series, sorry about that.  This new patch (replacing 0002) should fix the issue:
https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=1a77d269b1fed76b116bb8116e1f0caf05cc011f

The only change from the previous version is adding:

  SECITEM_ZfreeItem(&uniPwitem, PR_FALSE);

when retrying -c decryption.
Attachment #8908513 - Attachment is obsolete: true
Flags: needinfo?(dueno)
Attachment #8910178 - Flags: review?(kaie)
Comment on attachment 8910178 [details] [diff] [review]
0002-pk12util-Make-c-try-different-password-encoding-if-f.patch

r=kaie for the delta
Attachment #8910178 - Flags: review?(kaie) → review+
OBE (sorry, I was at the beach for the last week).
Flags: needinfo?(martin.thomson)
You need to log in before you can comment on or make changes to this bug.