Closed Bug 400742 Opened 13 years ago Closed 5 years ago

Add interfaces to derive AES keys from passphrases, as well as to encrypt/decrypt

Categories

(Core :: Security: PSM, defect)

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: hello, Assigned: hello)

References

Details

Attachments

(1 file, 12 obsolete files)

20.83 KB, patch
nelson
: review-
Details | Diff | Splinter Review
 
We (Labs) are in need of some interfaces to encrypt/decrypt from (chrome) scripts.  Searching with Dan Veditz we got to nsIStreamCipher and nsIKeyObject[Factory], but unfortunately the interface there is a little cumbersome, and doesn't allow for us to decrypt at all.

Ideally, what I'd like to see is:

key = deriveKeyFromPassphrase("secure passphrase");
ciphertxt = encrypt(key, "cleartext");
cleartxt = decrypt(key, ciphertxt);
Summary: Add → Add interfaces to derive AES keys from passphrases, as well as to encrypt/decrypt
Component: Security → Security: PSM
Product: Firefox → Core
Attached patch patch v0 (obsolete) — Splinter Review
The patch doesn't work yet.  Questions:

I don't see any AES algorithms for PBE in the list below, should there be some / am I looking in the wrong place?

http://mxr.mozilla.org/mozilla/source/security/nss/lib/util/secoidt.h#69

I've tried several other algorithms from the list above.  If I choose an AES one (e.g., SEC_OID_AES_128_CBC), then PK11_PBEKeyGen returns NULL.  If I choose one of the PBE algorithms, then I'll get a key, but PK11_CreateContextBySymKey will fail.  Attaching a debugger reveals that it's failing because the mechanism doesn't support the action (CKA_ENCRYPT). Help?
Assignee: nobody → thunder
Status: NEW → ASSIGNED
Looks like a good start. We should also include a parameter to pass the algorithm in (probably by oid). I'll look up how to do AES PBE just now...
Are the oids set in stone?  If it's at all possible they might change, I don't want to expose them via xpcom.  In that case, I'd want to expose an opaque uint with an internal switch statement on the c++ side (where I can get to the .h file directly) to set the correct value.

If they are guaranteed to not change, then I'd be OK with a getter/setter that takes a uint, which happens to be an oid.  c++ callers would be able to include the nss .h file and use the enum, and for javascript callers I'd define some constants in the idl for the common algorithms.  Or they can hardcode the algorithm numbers in their sources.

There would also need to be some guarantees that no negative values would ever be added to that enum, but I assume that won't be a problem.
Thanks for looking at my patch, btw.  Any ideas for how to solve the problems I mentioned earlier?  I'm a little stuck :-/
OIDs are standards, If you create new ones, then you need to do them from a space you own. NSS can accept new OIDs programmatically.

I've looked into the issue and it looks the there is a fundamental problem with NSS's PBE code. It currently only supports PKCS12 PBE at the top level. Most new stuff (including AES) uses PKCS 5 v2 PBE's. I have some work to do to get this working. The good news is there is a related bug that also needs this code as well. (openssl is now exporting pkcs12 with AES).

bob
Attached patch patch v0.1 (obsolete) — Splinter Review
I think I misunderstood earlier, I didn't know that OIDs were a standard outside of NSS, so I thought you were talking about the SECOidTag enum.

Did you mean that consumers of this interface should be able to pass in an integer corresponding to the values in SECOidTag, or some other value corresponding to the OID standard? (e.g., "2.16.840.1.101.3.4.1" for aes, according to my searches).

I would like to limit the interface to only an integer, whether an opaque one, or directly using SECOidTag values.

Attached is a new patch which implements a getter/setter to set the algorithm using SECOidTag values.

Unrelated to the above; I tried SEC_OID_PKCS12_PBE_WITH_SHA1_AND_128_BIT_RC4 which looks from your comments like it should work, but it's not able to generate a key.  Is there something wrong with the way I'm using NSS?  Or should I try a different algorithm? (which one?)

Thanks for your help.
Attachment #285961 - Attachment is obsolete: true
Attached patch patch v0.2 (obsolete) — Splinter Review
Trying different things, I got to this new patch.  I've gotten NSS to give me some data back, finally, but unfortunately it doesn't seem correct.  The return value of PK11_CipherOp is != SECSuccess, and the input/output lengths don't match.

It seems that I must use ECB, when I try CBC I get a null context back.  Is it because I don't have an IV?  I don't know what that is, so I can't give it one.
Attachment #286095 - Attachment is obsolete: true
Yes, an IV is critical to CBC ciphers.

IV means 'initialization vector', and using it helps thwart certain kinds of attacks (basically you want to use CBC with IV for any encryption of plan text, ECB is really only useful for encrypting high entropy data like keys).

In protocols, IVs can be made public without compromising the data, though for convinience you can generate an IV from your PBE.


bob
Depends on: 401928
Sorry about going dark, I had some other patches to get out.

SEC_OID_ are integer mappings to real oid values. We keep a table of oid values within NSS and map them to integers for more convenient referencing. There are interfaces for adding new oids to NSS on the fly.

Use of oids allow you applications to handle decryption in a cipher agnostic way. If cipher is encapsulated in something like CMS (like S/MIME), then the cipher types are encoded as oids in the actual encrypted data. The application simply passes the DER encode algorithm ID to NSS, and it can handle new ciphers without change as NSS gets new ciphers.

About the PK11_CipherOp failure, It's likely you need to make sure that the input and output lengths are integral block lengths (if the cipher is a block, unpadded cipher). You can get around this by using a _PAD type cipher, but then the underlying code will always keep the last block back until you call finish, and the cipher text lenght will never equal the plain text length.

I'll have to check on the RC4 problem, but it's likely we don't support RC4 PBE's. It's not a good idea to use stream ciphers (like RC4) in this context. Stream ciphers have the need for extra care to be secure, and usually aren't suitable for contexts with PBEs.


Thanks Robert!

I'm still thinking about your oid comments, but setting that aside for a moment - I have a question about the PK11_CipherOp failure.  I'd like to try out the _PAD cipher, and the one available is for a CBC cipher.  So I'm wondering about your earlier comment, "...you can generate an IV from your PBE".  Should I use this function?

PK11_GetPBEIV(SECAlgorithmID *algid, SECItem *pwitem);

And if so, do I use this function to generate the pwitem?

SECItem * 
PK11_CreatePBEParams(SECItem *salt, SECItem *pwd, unsigned int iterations);

I'm also curious about the padding--what extra data will I get back after an encrypt/decrypt roundtrip when using a _PAD cipher?  extra whitespace?

Thanks again, and I'll give this a shot tomorrow!
Hi dan.

yes PK11_GetPBEIV is the correct way to get the IV. pwitem is the same parameter you pass to PK11_CreatePBEParams.

BTW there will be some new API's for PKCS5v2. Pretty much anything that doesn't take an algorithm id will need to change. (PKCS v2 no longer lumps the PBE mechanism with the crypto mechanism with a single oid tag.

In the end we should wind up with something streamlined.

RE: padding...

There is not change in the data size after the encrypt/decrypt round trip, but the intermediate ciphertext will be larger than the plantext, and the order will be slightly different. Also when using the PAD mechanism there is data returned in the Final step.

In general the _PAD functions holds back up to one full block of data. The output from _PAD is always multiples of blocks. When you call final it returns the final block, which included coding to tell how many blocks is bad.

On decrypt it always keeps a complete block back in it's buffer. When you call final it examines the final block for the padding code and returns the non-pad blocks.

The upshot is you can't expect inputsize to equal outputsize on the CipherOp calls and you need to include an output buffer in the final call.

bob
Attached patch patch v0.3 (obsolete) — Splinter Review
New version generates an IV from the password.  It fails to get a context, though, I'm not sure why.  Any ideas?  I've tried removing the PK11_ParamFromIV call and passing the return value of PK11_GetPBEIV as the 4th param to PK11_CreateContextBySymKey, but that didn't seem to make any difference.  Is there a way (other than attaching gdb) to find out why it's failing?
Attachment #286225 - Attachment is obsolete: true
You are basically stuck that the point that NSS does not support PKCS5 v2.

In your patch, you are doing a PK11_KeyGen against regular AES, which does not generate a PBE key, but a new random AES key. This will work for testing, but you won't be able to decrypt the output.

You have a similar problem with PK11_GetPBEIV. There isn't a PBE mechanism that supports AES yet, so PK11_GetPBEIV will fail. With no IV, you can't get a context.

If you use a non-AES PBE (say DES3 PKCS 12), you should be able to get this code working (though you really want to use PK11_GenPBEKey() to get the key).

To get AES to work you need to wait for 401928.

bob




What are you getting for IV.

 The algorithm ID passed to PK11_GetPBEIV should be a PBE algorithm, but you currently aren't
Bob, thanks!  I seem to have gotten things closer to working with DES3.  One question about padding, though.  I assume I have to call this:

SECStatus PK11_DigestFinal(PK11Context *context, unsigned char *data, 
				unsigned int *outLen, unsigned int length);

to get the last chunk of data.  How do I know how big of a buffer to give it, though?  I guess it can't be bigger than 128 bits (for a 128 bit cipher)?

Also, it seems to work either way, but do I need to do:

SECItem *param = PK11_ParamFromIV(mCipherMechanism, iv);

to pass the iv in the 4th parameter to PK11_CreateContextBySymKey?  Or can I just give it the return value of PK11_GetPBEIV directly?
Attached patch 3DES hard-coded testing variant (obsolete) — Splinter Review
Attachment #287075 - Attachment is obsolete: true
Attachment #298510 - Flags: review?(rrelyea)
Attached patch patch v0.5 (obsolete) — Splinter Review
Generates a random salt; allows for the mechanisms to be customized.

Still doesn't read/generate modules, but that can be left to the consumers of this interface to do.

Added unit tests.
Attachment #298510 - Attachment is obsolete: true
Attachment #298510 - Flags: review?(rrelyea)
Hi Dan,

The PKCS 5v2 support landed in NSS 3.12 Beta 1.

The primary change you will need to make is to 'move up a level'. PKCS #5 v2 has a different low level PKCS #11 Mechanism parameter format, so you will want to extract that from the higher level SECAlgorithmID structure.

To Encrypt you want:
    algorithmID = PK11_CreatePBEAlgorithmID()  or PK11_CreatePBEV2AlgorithmID()
       both will create V1 and V2 algorithms ID's, The second one gives you finer control over which is created. (The PBE algorithm and the cipher algorithm is split). In PK11_CreatePBEAlgorithmID you either pass a combined algorithm (e.g. SEC_OID_PKCS12_V2_PBE_WITH_SHA1_AND3KEY_TRIPLE_DES_CBC) or a cipher algorithm (e.g. SEC_OID_DES_EDE3_CBC or SEC_OID_AES_192_CBC). In the latter case PK11_CreatePBEAlgorithmID() will pick the appropriate PBE algorithm (for DES it would be PKCS 5 v1 or PKCS12 v2 depending on key size, for AES it would be PKCS 5 v2). The salt is passed in the PK11_Create call as a parameter. if NULL is passed a randome salt will be generated.

     PK11SymKey *key = PK11_PBEKeyGen(slot, algorithmID, pwitem, PR_FALSE, pwarg).
     SECItem *mechanismParam;
     CK_MECHANISM_TYPE cryptoMechType = PK11(algorithmID, &mechanismParam, pwitem).
     CK_MECHANISM_TYPE cryptoPadMechType = PK11_GetPadMechnaism(cryptoMechType);

     PK11Context *context = PK11_CreateContextBySymKey(cryptoPadMechType,
              CKA_ENCRYPT, key, mechanismParam);
      /* do encrypt */

     /* package algorithmID */
     SEC_ASN1EncodeItem(NULL, &dest, AlgorithmID, SEC_ASN1_GET(SECOID_AlgorithmIDTemplate));
    /* copy dest to your data stream */
     
      /* free key, context, mechanismparam, algorithmID, dest */


Decrypt is...

    /* get dest from your data stream */
    SECStatus rv = SEC_QuickDERDecodeItem(arena, algorithmID, SEC_ASN1_GET(SECOID_AlgorithmIDTemplate), &dest);


     PK11SymKey *key = PK11_PBEKeyGen(slot, algorithmID, pwitem, PR_FALSE, pwarg).
     SECItem *mechanismParam;
     CK_MECHANISM_TYPE cryptoMechType = PK11(algorithmID, &mechanismParam, pwitem).
     CK_MECHANISM_TYPE cryptoPadMechType = PK11_GetPadMechnaism(cryptoMechType);

     PK11Context *context = PK11_CreateContextBySymKey(cryptoPadMechType,
              CKA_DECRYPT, key, mechanismParam);
      /* do decrypt */

     /* free key, algorithmID, mechanismParam */



The Encrypt side takes a single SECOID describing the encryption mechanism and the pbe password. The Decrypt side only needs the data and the pbe password. (NOTE that decrypt will work with multiple algorithms automatically). IV, salt and iteration count is encoded in the 'dest' block as ASN1 data.

Error handling, of course, is still missing from this example.

What is new in the NSS 3.12 beta2 that makes this API work is PK11_GetPBECryptoMechanism() which automatically handles the work of properly extracting (or calculating as appropriate) the IV.
PK11_CreatePBEV2AlgorithmID is also new, but it's only needed if you want to do something like create a PKCS 5 v2 with DES block.


bob


  
    

     
Attachment #299403 - Flags: review?(rrelyea)
Attachment #299403 - Flags: review?(rrelyea)
Thanks Bob, I'll have a new patch up for review as soon as I can.
Hey Bob,

I've been trying to get this working with no luck.  Here's the current problem:

Assertion failure: encode_kind == SEC_ASN1_INLINE && !optional, at secasn1d.c:610

The first few frames of the stack look like this:

(gdb) where
#0  0x9662847a in __kill ()
#1  0x9662846d in kill$UNIX2003 ()
#2  0x9669f782 in raise ()
#3  0x966aed3f in abort ()
#4  0x00061dd9 in PR_Assert (s=0x17aa608 "encode_kind == SEC_ASN1_INLINE && !optional", file=0x17aa4dc "secasn1d.c", ln=610) at /Users/thunder/sources/mozilla-trunk/mozilla/nsprpub/pr/src/io/prlog.c:577
#5  0x017a0921 in sec_asn1d_init_state_based_on_template (state=0x8b2898) at secasn1d.c:610
#6  0x017a2d0e in sec_asn1d_next_in_sequence (state=0x8b2838) at secasn1d.c:2023
#7  0x017a3c96 in SEC_ASN1DecoderUpdate_Util (cx=0x8b2810, buf=0x8b326b "0\n\006\b*?H??\r\002\a?`?H\001e\003\004\001\026???????\004\020od?⍻L?\"?\034? \a?Z", '?' <repeats 153 times>..., len=12) at secasn1d.c:2672
#8  0x017a42aa in SEC_ASN1Decode_Util (poolp=0x541dc0, dest=0xbfffcd50, theTemplate=0x170b300, buf=0x8b3258 "0\035\004\bSALTTODO\002\002\004\027\002\001 0\n\006\b*?H??\r\002\a?`?H\001e\003\004\001\026???????\004\020od?⍻L?\"?\034? \a?Z", '?' <repeats 134 times>..., len=31) at secasn1d.c:2962
#9  0x017a4309 in SEC_ASN1DecodeItem_Util (poolp=0x541dc0, dest=0xbfffcd50, theTemplate=0x170b300, src=0x8b3220) at secasn1d.c:2977
#10 0x0158ff27 in pbe_PK11AlgidToParam (algid=0x8b3214, mech=0x541d30) at pk11pbe.c:799
#11 0x015868de in PK11_ParamFromAlgid (algid=0x8b3214) at pk11mech.c:1229
#12 0x01590b57 in PK11_PBEKeyGen (slot=0x888a00, algid=0x541d60, pwitem=0xbfffcf60, faulty3DES=0, wincx=0xbfffcf54) at pk11pbe.c:1368
#13 0x013dd680 in nsPBECipher::Encrypt (this=0x50ded0, aPass=@0x541c20, aIter=1047, aClearText=@0x541c70, aMetadata=@0x541ca0, aCipherText=@0x541cb0) at /Users/thunder/sources/mozilla-trunk/mozilla/security/manager/ssl/src/nsPBECipher.cpp:159

Clearly it's failing to encode some of the metadata during key generation, but I don't know why that would be.  Any ideas?

I'm also (unrelatedly) a bit puzzled about the SEC_ASN1EncodeItem call:
1) Does the return value matter, or is that the same as the out param?  LXR shows me some code checking the return value to be equal to the outparam as a way to check if it succeeded... huh?
2) It returns a SECItem, which I don't know how to serialize.  Do I base64-encode dest.data?  Do I just cast dest.data to a char* and assign it to an nsACString?
Attached patch patch v0.6 (obsolete) — Splinter Review
Non-working patch for reference (see comment #20).

Note: patch moves unit tests to make it possible to run 'make SOLO_FILE="test_pbe.js" check-interactive' (or check-one), which is super useful for testing and running gdb.  I would remove this before submitting the patch for review, though.
QA Contact: firefox → psm
Comment on attachment 301823 [details] [diff] [review]
patch v0.6

>+  try {
>+    var garbage1 = pbe.decrypt("wrong secret", salt.value, 1047, cipherTxt);
>+    do_check_true(garbage2 != "my very secret message!");
>+  } catch (e) {}

I believe you mean garbage1 there.
Whoops, indeed! Thanks for catching that.
Personally I would vote for merging meta data with the encrypted data, but not sure how to do it. Some kind of safe as used for PKCS12?

The change in secasn1d.c is just outline. It seems for me that there is some unfinished job on INLINE | OPTIONAL template items and its decoding. Please check bug 280121, attachment 180145 [details] [diff] [review], and bonsai http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/util/secasn1e.c&rev=1.21 line 262. 

When that assertion is commented out all works perfectly.
Attachment #299403 - Attachment is obsolete: true
Attachment #301823 - Attachment is obsolete: true
Attachment #301984 - Flags: review?(rrelyea)
Comment on attachment 301984 [details] [diff] [review]
patch v1.0, AES, 3DES support, ASN.1 metadata

r- but with fairly simple fixes.

This patch looks REALLY good.

Issues: in InitCipherContext().

It's possible that PK11_GetPBECryptoMechanism could fail, not setting mechParam (or setting it to NULL).

It's ok to let the failure 'roll forward' (mMechanism will be CKM_INVALID_MECHANISM if the call fails), but you will crash in the SECITEM_ZfreeItem() if don't arrange for mechparam to possibly be NULL.

(to induce the failure pass in al algid with a valid oid that doesn't map to a PKCS #11 mechanism (secoid.c table has lots of them... cert extensions would be a good choice).

CipherOperation().

Minor nit, the second PK11_Finalize should be redundant (if things aren't being freed by the first one, let me know.

Finally the commented out Assert. I'm a little afraid of commenting out asserts in the asn1 engine. Those asserts are meant to catch template errors, though you have no templates of your own. I'm wondering what is triggering it. What is the stack traceback?

bob

RE your other comment. pkcs7 (older) and cms (newer) are standard ways of packaging up a algid with the data (this is what pkcs #12 uses). The interface changes to cms/pkcs #7 were made last week to the trunk of NSS...

bob
Attachment #301984 - Flags: review?(rrelyea) → review-
(In reply to comment #25)

> This patch looks REALLY good.

Thanks, good to know we're finally getting close.  And many thanks to Honza for his help with the patch.

> Finally the commented out Assert. I'm a little afraid of commenting out asserts
> in the asn1 engine. Those asserts are meant to catch template errors, though
> you have no templates of your own. I'm wondering what is triggering it. What is
> the stack traceback?

See comment #20 for a traceback (if you need to see frames above nsPBECipher::Encrypt, let me know).
This is patch with two small fixes you supposed in comment 25.

I reverted that assertion removal to let you see/test it if you have a time. This assert fails on new PBEv2 template here:

http://lxr.mozilla.org/seamonkey/source/security/nss/lib/pk11wrap/pk11pbe.c#107

on the prfAlgId item. It was checked in recently as part of the new PBE v5 support.

PKC7 encryption and decryption: I spent two days to make this work and I completely failed. I was trying to use CMS API from smime and then (completely different) pkcs7 API. I wasn't able to make this work (even encrypt) because there is no sufficient documentation nor good example. PKC12 is too complicated to take as an example or trace (I did try that to help me understand).

If we want to have this in the PBE cipher I really need some advice here. Thanks.
Attachment #301984 - Attachment is obsolete: true
Attachment #302597 - Flags: review?(rrelyea)
Thanks Honza.

I thought there may be a prayer getting pkcs #7 working with the current interface (since pkcs #12 uses it), but it may be missing some stuff that pkcs #12 gets through private interfaces...:(.

CMS looks like there's a long way to go to get PBE working through that interface.

I did identify one area, where we don't have the ability to supply a pkcs #5v2 cipher through the existing CMS interface, but I also think that there is a missing key gen operation in CMS as well.

bob
Attached patch PKCS7 draft (obsolete) — Splinter Review
Ok, I got that to work - result of the PBE cipher is regular PKCS7 encrypted data thus, we don't need meta data anymore. 

But it seems that we are at the beginning again. There is no support for AES but just DES. It fails to parse alg id somewhere in the deep (I will refer tomorrow). And there is also no chance to setup the iteration count because it is hard coded to 1.

The patch is just *DRAFT*, so please be tolerant :)
Comment on attachment 302597 [details] [diff] [review]
patch v1.1, AES, 3DES support, ASN.1 metadata

1. We'll look into the Assert, I think the NSS team may have a test case for it.

2. please initialize mechParam to NULL for paranoia sake.

>  SECItem *mechParam;
>	
>  mMechanism = PK11_GetPBECryptoMechanism(aAlgid, &mechParam, &pwitem);
>
>  CK_MECHANISM_TYPE padMech = PK11_GetPadMechanism(mMechanism);

Thanks,

bob
Attachment #302597 - Flags: review?(rrelyea) → review+
Comment on attachment 302845 [details] [diff] [review]
PKCS7 draft

I haven't reviewed this thoroughly yet, but a scan seems like this is the right track.

Thanks for taking the effort to figure out the PKCS #7. I think there will be a huge benefit in staying with a standard format like this.

I think you'll need a new PKCS#7)_Create call to get pkcs 5 v2 pbe's working. I'll try to get a patch for that attached this week.

bob
Attached patch PKCS7 final, v2.0, 3DES support (obsolete) — Splinter Review
So this supports only 3DES. Other ciphers fails to create the context during encryption.

It would also be perfect to enable customization of iteration count that is currently hard-coded to 1. Anyway, thanks for pulling the patch for that!
Attachment #302845 - Attachment is obsolete: true
Attachment #303380 - Flags: review?(rrelyea)
Comment on attachment 303380 [details] [diff] [review]
PKCS7 final, v2.0, 3DES support

r- but the issues should be quick to fix, I'll take a look at any new patch much more quickly than I got to this one.

This patch is basically sound, the only issue is one of some memory leaks... all of them in error conditions...

In nsPBECipher::Encrypt()
SEC_PKCS7EncoderFinish should be call even if the update does not succeed (it's needed to free up the EncoderContect created by SEC_PKCS7EncoderStart().

In Decrypt, same issue for SEC_PKCS7DecoderFinish()

Everything else looks correct (ContentInfo and keys are being properly destroyed).

bob
Attachment #303380 - Flags: review?(rrelyea) → review-
Attached patch PKCS7 final, v2.1, 3DES support (obsolete) — Splinter Review
Attachment #302597 - Attachment is obsolete: true
Attachment #303380 - Attachment is obsolete: true
Attachment #309377 - Flags: review?(rrelyea)
Comment on attachment 309377 [details] [diff] [review]
PKCS7 final, v2.1, 3DES support

r+ relyea..
Attachment #309377 - Flags: review?(rrelyea) → review+
Thanks for all comments and reviews, Robert! 

Dan, we probably need to land the patch in TRUNK ASAP. Do I have to rise blocking1.9 flag?
I think we're past the point where we can get blocking status for a new feature.  We should try to get this into the next release.

For Weave, we'll also need more interfaces exposed to switch to NSS, since we're already using RSA keys and AES (via openssl).

Some nits on patch v2.1:

1) The *Callback functions should be above the // nsIPBECipher comment, since they're not part of the interface.

2)
+      NS_ASSERTION(ecx, "Could not create PKCS#7 encoder context");

I think that should be an NS_ENSURE_TRUE or similar, not an assertion.  You can use NS_WARNING if you want to get a string displayed as an error.

3) Encrypt takes an aIter argument which is not used anymore.

4) This patch still moves the tests around (see comment #21).  I'm ok with it if the module owner doesn't object, though.

5)
+  dump("salt: " + sat + "\n");
+  dump("ciphertxt: " + cipherTxt + "\n");

Should be removed from the test.

6)
+  // The following may cause decryption to fail, but that's ok

I'm not 100% sure that is true.  Are we guaranteeing an exception when decryption fails?  The tests should then be modified to check for that.
(In reply to comment #37)
> Some nits on patch v2.1:
> 
> 1) The *Callback functions should be above the // nsIPBECipher comment, since
> they're not part of the interface.
> 

Good point.

> 2)
> +      NS_ASSERTION(ecx, "Could not create PKCS#7 encoder context");
> 
> I think that should be an NS_ENSURE_TRUE or similar, not an assertion.  You can
> use NS_WARNING if you want to get a string displayed as an error.

I would leave this as is because I need to free cinfo bellow even the context failed to create.

> 
> 3) Encrypt takes an aIter argument which is not used anymore.
> 

IMHO NSS API should change to take this argument. However you are right it should be for now removed from nsIPBECipher API.

> 4) This patch still moves the tests around (see comment #21).  I'm ok with it
> if the module owner doesn't object, though.
> 

Something I wanted to ask closer: personally I would leave the tests as are now, because I know about at least one patch that changes these tests and we may loose changes or make the commit process more difficult when merging with moved files.

> 5)
> +  dump("salt: " + sat + "\n");
> +  dump("ciphertxt: " + cipherTxt + "\n");
> 
> Should be removed from the test.

I will do this on next version

> 
> 6)
> +  // The following may cause decryption to fail, but that's ok
> 
> I'm not 100% sure that is true.  Are we guaranteeing an exception when
> decryption fails?  The tests should then be modified to check for that.
> 

BTW test must be changed - API has changed (I was testing using my own and forget to do that). When you enter wrong password decryption will fail because we are using padding schema of the mechanism (p7 does it). This adds some magic padding to the plain text and is being look up when decrypting. When it is not found (because of bad password=>key) then decryption cannot cut the padding and fails. We may return empty string but it is IMO not correct. Failure in such a case is better.
(In reply to comment #38)

> > 2)
> > +      NS_ASSERTION(ecx, "Could not create PKCS#7 encoder context");
> > 
> > I think that should be an NS_ENSURE_TRUE or similar, not an assertion.  You can
> > use NS_WARNING if you want to get a string displayed as an error.
> 
> I would leave this as is because I need to free cinfo bellow even the context
> failed to create.

That's even more of a reason to not use an assertion.  Simply do NS_WARNING and continue execution.

> > 4) This patch still moves the tests around (see comment #21).  I'm ok with it
> > if the module owner doesn't object, though.
> > 
> 
> Something I wanted to ask closer: personally I would leave the tests as are
> now, because I know about at least one patch that changes these tests and we
> may loose changes or make the commit process more difficult when merging with
> moved files.

I think the tests are in the wrong place right now, and being able to run 'make
SOLO_FILE="test_pbe.js" check-interactive' is a pretty big win.  However, it's beyond the scope of this bug, so I'd leave it to someone else to decide.

Having a patch that touches those files doesn't matter, they can very easily be changed to apply to the moved files.

> > 6)
> > +  // The following may cause decryption to fail, but that's ok
> > 
> > I'm not 100% sure that is true.  Are we guaranteeing an exception when
> > decryption fails?  The tests should then be modified to check for that.
> > 
> 
> BTW test must be changed - API has changed (I was testing using my own and
> forget to do that). When you enter wrong password decryption will fail because
> we are using padding schema of the mechanism (p7 does it). This adds some magic
> padding to the plain text and is being look up when decrypting. When it is not
> found (because of bad password=>key) then decryption cannot cut the padding and
> fails. We may return empty string but it is IMO not correct. Failure in such a
> case is better.

Yeah, I wasn't making a case against hard failures.  I think that's fine.  But I think we should be clear about what is expected and fix the tests to reflect that.  That is, if we expect a failure, the test should make sure it fails on bad input.
Sorry for this delay, there were a lot of priority work on FF3. This is patch targeting your comments. There is one little problem: the test failed this way because of assertion I was not aware before of - http://lxr.mozilla.org/mozilla/source/security/nss/lib/pkcs7/p7local.c#588. Not sure this assertion is correct because decryption and search for the padding pattern may fail when there is incorrect password/key used. I propose to remove it, therefor.
Attachment #309377 - Attachment is obsolete: true
Attachment #317027 - Flags: review?(thunder)
I don't know all the background of what's motivating this request, but based 
on a superficial read, it appears that this RFE wants to invent a new 
implementation that will accomplish roughly the same thing as the SDR 
("Secret Decoder Ring" :) interface, already implemented and scriptable.

So I want to ask: Can your needs be met through SDR, without all this 
additional development?
Honza, the link you gave in comment 40 does not point to an assertion.
Did you not really mean assertion? or were you referring to another line?

The test at the line you cited is correct, and should not be changed.

BTW, the code in lib/pkcs7 is very old and, IMO, obsolete.  It implements 
a very old version of PKCS7.  NSS contains a newer implementation that 
conforms to the CMS v3.0 RFC in lib/smime (which is misnamed and should be
lib/cms).  IMO. all new code should be useing lib/SMIME.  We want to get 
rid of lib/pkcs7 as soon as all code is converted to use lib/smime.
Nelson, I'm not involved in the development, nor have I requested this feature, nor am I advocating for it, but I think I understand one possible intention.

The SDR requires storage.

A passphrase does not require storage.

Thus, if everything gets derived from a passphrase, and if you never use SDR but always phassphrase-derived, it will work on computers that don't have your personal SDR storage.

(And I think if we're talking about an "Internet Cafe" scenario, I guess that means you could become a victim of key logging.)
(In reply to comment #42)
> Honza, the link you gave in comment 40 does not point to an assertion.
> Did you not really mean assertion? or were you referring to another line?
> 

The reference was correct at the time I made it. The assertion was removed by patch of bug 436428.

(In reply to comment #43)
> Nelson, I'm not involved in the development, nor have I requested this feature,
> nor am I advocating for it, but I think I understand one possible intention.
> 
> The SDR requires storage.
> 
> A passphrase does not require storage.
> 
> Thus, if everything gets derived from a passphrase, and if you never use SDR
> but always phassphrase-derived, it will work on computers that don't have your
> personal SDR storage.
> 
> (And I think if we're talking about an "Internet Cafe" scenario, I guess that
> means you could become a victim of key logging.)
> 

Quit right. The original intention to have this component is to be able to strongly and in compatible way encrypt any data using a pass phrase and upload the result to a central or any kind of server storage w/o any trace of the encryption credentials and to be able to retrieve it on a different machine only and only with the same pass phrase. And again, leave no traces.
As I understand the SDR API (from searching mxr, as I don't know any authoritative docs), I can create a new random key and encrypt/decrypt data with it via the SDR.  But can't use only a passphrase.  The key may be locally encrypted with a passphrase (the master pw), but that is outside of the API the SDR provides.

I should note that the original intent was to support the Weave project, and since we couldn't get all the interfaces we needed into Fx3, we'll be shipping our own component derived from my and honza's work here.  Our needs have changed somewhat, so 'WeaveCrypto' has more stuff.  We're tracking it in bug 433949.

This bug is still valid imo, as we'll want these interfaces provided by the platform.  Having to write a component to access NSS sets a high bar indeed.
Comment on attachment 317027 [details] [diff] [review]
PKCS7 final, v2.2, 3DES support

Very little of this patch touches NSS, but one of the parts that 
does gets an r- from me.  It is the removal of the following assertion,
which I believe is correct, and serves to detect invalid ASN1 decoder
templates.  

Please explain what templates have a problem, and we can look at 
correcting them.

>--- security/nss/lib/util/secasn1d.c	2008-02-05 16:15:21.914251000 +0100
>+++ security/nss/lib/util/secasn1d.c	2008-02-07 17:43:26.423726800 +0100
>@@ -600,21 +600,21 @@ sec_asn1d_init_state_based_on_template (
> 	    if (encode_kind & SEC_ASN1_INLINE) {
> 		/* check that there are no extraneous bits */
>-		PORT_Assert (encode_kind == SEC_ASN1_INLINE && !optional);
>+		/*PORT_Assert (encode_kind == SEC_ASN1_INLINE && !optional);*/
> 		state->place = afterInline;
> 	    } else {
Attachment #317027 - Flags: review?(thunder) → review-
In reply to comment 44, thanks for the explanation of the requirements.
I agree that the "leave no traces" aspect of this makes SDR unsuitable 
for this purpose.  

Here's some perspective on SDR.  From NSS's perspective, SDR is a method
of providing encryption/decryption of any general data based on a symmetric
key stored locally in the PKCS#11 token by NSS. The symmetric encryption 
key is randomly generated the first time it is used, and then stored.  The passphrase (a.k.a. "Master Password") is used to encrypt the stored key, 
not the data itself.  NSS does not provide or require any storage of the encrypted/decrypted data, but does provide storage of the encryption key.
SDR is suitable for data being encrypted for local storage purposes, and
for purposes where the data will be encrypted and decrypted by the same 
user on the same system (or with the same PKCS#11 token), but is not 
generally suitable for data being transported from one system to another
apart from the PKCS#11 token that stores the symmetric encryption key.
Comment on attachment 309377 [details] [diff] [review]
PKCS7 final, v2.1, 3DES support

i have to give this patch an r- for the same reason as I did so for another patch in this bug.  The assertion in NSS is there to detect invalid templates.  It has done its job well.  I must continue to do that job.  Invalid templates must be fixed.  

Note that my review applies only to the portion of this patch that changes NSS source code.  

We don't know what template caused this.  I recently fixed a bad template in bug 401928.  If that is the same template that was causing troubles, then the problem will be solved by taking a later snapshot of NSS trunk.
Attachment #309377 - Flags: review-
Comment on attachment 317027 [details] [diff] [review]
PKCS7 final, v2.2, 3DES support

My r- for this patch applies only to the modifications to NSS source files.  If this patch was split into separate patches for NSS and non-NSS, my r- would apply only to the NSS part.
guys, what do we need to get this feature going?  

Right now there is a c++ xpcom component based on this code which weave is using.  I would very much like to see this component go away.
For reference, the Weave component dougt refers to is here:

http://hg.mozilla.org/labs/weave/file/tip/src/
The patch should be fine against the current NSS without the fix to ASN.1. The template problem that was causing the asn.1 to fail was in the PBE code as should not be fixed. Try testing without that part of the patch with the latest NSS, but no patches to NSS (particularly the patch to secasn1d.c). 

bob
The Weave component from comment #51 is originally based on this patch, but a bunch has changed.  It doesn't patch NSS anymore (it ends up in an addon and just links with NSS), it adds some features, and it changes the API a bit.  It includes RSA support, and no longer wraps metadata using ASN.1.  It also actually does (and defaults to) AES, instead of 3DES as the latest patch here.

IMO, the way forward here is to grab the Weave component and turn it into a patch for the Mozilla tree.
In comment 52, Bob wrote:
> The template problem that was causing the asn.1 to fail was in the PBE 
> code as should not be fixed. 

I think he meant to write:
... code and should now [already] be fixed.

I fixed a bad ASN.1 template in bug 401928.  Bob believes that was the 
template that was tripping Dan up.  

As an aside, this bug might have progressed a LOT faster if more information
about the asssertion failure had been reported, more than just a patch to 
remove the correct assertion.
(In reply to comment #54)
> I fixed a bad ASN.1 template in bug 401928.  Bob believes that was the 
> template that was tripping Dan up.  

Yup, we could probably use ASN.1 if we wanted to (but I don't).

> As an aside, this bug might have progressed a LOT faster if more information
> about the asssertion failure had been reported, more than just a patch to 
> remove the correct assertion.

Wow, I couldn't disagree more.  Awful API decisions, lack of documentation, old documentation, no one to go to for newbie questions were all factors that *greatly* overshadow any time wasted on the assertion.  I don't even know how I was supposed to know that I needed to report the assertion failure, and I did ask about it in comment #20 on this bug:

> I've been trying to get this working with no luck.  Here's the current problem:
> 
> Assertion failure: encode_kind == SEC_ASN1_INLINE && !optional, at
> secasn1d.c:610

And that's after months of trying to get something (anything) working.

For comparison, I had a basic demo of encryption (including RSA support!) using the openssl command-line in about 20 minutes, and had it integrated into Weave in a few days.
Dan, we run a mozilla developer newsgroup in which we offers lots of help 
to developers who run into troubles using NSS.  We answer LOTs of newbie
questions there.  You didn't ask for any help there (AFAIK).
Remeber that, unlike the weave project, NONE of the NSS developers work 
for MoCo.  If you don't take time to communicate what you're trying to do 
and why with folks outside MoCo's walls, you'll get limited help. 
There's no substitute for directly asking the people who know for help.

Dougt, in reply to comment #50, 
I took a glance at that xpcom code.  I didn't review it, but it doesn't look terrible.  Why do you want to get rid of the XPCom component?  
Is it merely the case that you want PSM to own it rather than weave?
(In reply to comment #56)
> I took a glance at that xpcom code.  I didn't review it, but it doesn't look
> terrible.  Why do you want to get rid of the XPCom component?  
> Is it merely the case that you want PSM to own it rather than weave?

Yes, the idea is not to remove the component altogether, but to take it out of Weave and put it back into PSM.
It's out of scope of this bug, but I would love to have this component somehow exposed to scripts and content.
I'd like to look at the bigger picture here, at the problem that this feature
is intended to solve, to ask questions about the threat model, to determine
if the implementation actually provides the expected security against the 
threats.  This bug may not be the right place for that discussion, and if not
then please suggest the preferred alternative forum.  It must be one with an 
archive (not IRC).

As I understand it, this is another variant of the old "location independence"
idea.  A user goes to a browser, is able to download (from a server, or 
perhaps from a USB stick) personalized info such as prefs, bookmarks, and 
perhaps other stuff, and then when he's done, is able to upload that again, 
then go to another browser somewhere else and do it all again.  The server 
(or USB stick) provides continuity of the roving user's "session".  The idea 
is to move that data "securely" (against some set of threats) without leaving traces behind on any of the browser systems.  

Has anyone written down the threats against which security is desired?  
Is it supposed to be secure against replay attacks?  Or against MITM?  
(Hint: simple PBE schemes are vulnerable to replay.)

If we can write down the complete "protocol" being proposed, and the set of 
threats against which it is hoped to be secure, then we can determine if it
is secure against those threats, and if not, we can determine how to make it 
so.
I think a security review of Weave in general would be very welcome, and I agree this bug is probably not the right place for it.  How about the Weave forum: http://groups.google.com/group/mozilla-labs-weave (available as a mailing list), or one of the other mozilla mail/news groups would be fine too.

We should keep this bug limited to the code in toolkit/psm/whatever that exposes some crypto primitives (e.g., asymmetric, symmetric encryption, deriving keys from passphrases, etc) so that they are available to js addons.

The original request in this bug is for PBE+AES interfaces.  Since then we have also added RSA support, and if it makes sense to keep it all together we can keep that in this bug as well.

IDL for the latest version of the component is here:

http://hg.mozilla.org/labs/weave/file/97ea35c9f9fd/src/IWeaveCrypto.idl

It is basic, but works.  If there are no big issues, we could remove all the "Weave" strings and generate a patch against PSM.
My concern for this bug/RFE is essentially this.  Design of secure protocols 
is difficult.  Many people have wrongly assumed that if they start with good
strong primitives, such as AES or SHA-256, then whatever protocol they devise
will be good and strong.  Sadly, that's not so.  That's why historically the
NSS team has attempted to provide high-level functions (such as SSL or CMS 
(the crypto used in S/MIME), or code signing) that use proven secure protocols, 
and make those readily available in browsers, rather than making the low level
primitives readily available.  

One of the downside risks of making low level primitives available, is that 
if users use them to construct their own crypto protocols that are weak,
and then they discover that their protocol is vulnerable (attackers are 
successful in defeating whatever security those protocols were imagined to 
possess), this ends up reflecting badly on Mozilla and Firefox.  The press
announcements don't say "Some user's home-grown crypto protocol was insecure".
They say "Firefox crypto found to be insecure".  The NSS team has worked 
pretty diligently for over a decade to avoid that scenario.  I'm concerned 
that this proposed set of primitives is a step down that slippery slope.
There's no assumptions on my part that because we're starting with strong primitives we have a strong system.  For all I know there are holes in Weave that we're not aware of (and thus why I would be very happy to have a sec review of it).

And that's what I don't understand about your argument: you haven't actually prevented me from using Mozilla/NSS to create a potentially insecure system.  So your solution is a non-solution as far as I can see.  It's more work, but extensions can still link with NSS and do whatever they wish.
I stumbled across this bug when looking for a way to encrypt data in an add-on of mine and have the encrypted data be readable on another system.  I know of another add-on author who wants the same functionality, though in his case he wants to be able to do a public key encryption without triggering the master password prompt.  We're both looking at the Weave Crypto component and in the other author's case, he's actually including it in his add-on. He included an older version though. What affect that if both Weave and his add-on run simultaneously is unknown.

It seems like the patch for this bug was nearly complete except for a removed assertion and then all progress stopped 6 months ago.  Is this bug still being worked on?
The assertion removal is not necessary any more, that problem has been fixed in bug 401928 (I have to confirm it). I believe I can have a renewed patch w/o NSS modifications for another review round.
At this point, webcrypto is probably the way to go here.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.