1399867 - pk12util: add backward compatibility with old NSS produced files

Status: RESOLVED FIXED

(NSS :: Tools, enhancement)

Description

[Daiki Ueno [:ueno]]

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.

Comment 1

[Daiki Ueno [:ueno]]

Attachment: 0001-pkcs12-Add-a-compat-option-for-password-encoding.patch

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.

Comment 2

[Daiki Ueno [:ueno]]

Attachment: 0002-pk12util-Make-c-to-try-old-password-encoding-if-fail.patch

This makes pk12util -i accepts the files created with pk12util -e -c <PBES2 mech> from old NSS releases.

Comment 3

[Daiki Ueno [:ueno]]

Attachment: 0003-pk12util-Make-C-to-try-old-password-encoding-if-fail.patch

This makes pk12util -i accepts the files created with pk12util -e -C <PBES2 mech> from old NSS releases.

Comment 4

[Daiki Ueno [:ueno]]

Attachment: 0004-tests-Add-test-for-reading-PKCS-12-files-created-wit.patch

Test case for the change, with a PKCS #12 file created with NSS 3.21.

Comment 5

[Robert Relyea]

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.

Comment 6

[Robert Relyea]

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 7

[Robert Relyea]

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.

Comment 8

[Robert Relyea]

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

Comment 9

[Robert Relyea]

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.

Comment 10

[Daiki Ueno [:ueno]]

Attachment: 0002-pk12util-Make-c-try-different-password-encoding-if-f.patch

Read __NSS_PKCS12_DECODE_FORCE_UNICODE at startup, and revert to the default value if we set the option internally.

Comment 11

[Daiki Ueno [:ueno]]

Attachment: 0003-pk12util-Make-C-try-different-password-encoding-if-f.patch

Same as the -c patch; revert to the default value PR_FALSE.

Comment 12

[Daiki Ueno [:ueno]]

(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?

Comment 13

[Robert Relyea]

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!

Comment 14

[Robert Relyea]

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

Comment 15

[Kai Engert (:KaiE:)]

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

Comment 16

[Daiki Ueno [:ueno]]

Attachment: 0002-pk12util-Make-c-try-different-password-encoding-if-f.patch

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.

Comment 17

[Kai Engert (:KaiE:)]

Comment on attachment 8910178 [details] [diff] [review]
0002-pk12util-Make-c-try-different-password-encoding-if-f.patch

r=kaie for the delta

Comment 18

[Kai Engert (:KaiE:)]

https://hg.mozilla.org/projects/nss/rev/222ed4d2deb6
https://hg.mozilla.org/projects/nss/rev/835f791a0918
https://hg.mozilla.org/projects/nss/rev/3c7359ad3ce6
https://hg.mozilla.org/projects/nss/rev/16eb0e48e438

Comment 19

[Martin Thomson [:mt:]]

OBE (sorry, I was at the beach for the last week).