337733 - implement an rc4 xpcom interface

Status: RESOLVED FIXED

(Core :: Security: PSM, enhancement)

Description

[Tony Chang (Google)]

It would be nice to have an xpcom interface to the rc4 implementation in freebl.

Specifically, safe browsing would like to use it to encrypt urls on the client side.  We're currently using a javascript implementation (in /toolkit/components/url-classifier/content/js/arc4.js), but it's pretty slow.

Comment 1

[Wan-Teh Chang]

We need to keep track of all crypto code in Mozilla for
export or import filing.  Someone should notify the
appropriate person at Mozilla Corp. or Mozilla
Foundation about the RC4 implementation in
/toolkit/components/url-classifier/content/js/arc4.js.

RC4 is not a "FIPS Approved" algorithm, so the use of
/toolkit/components/url-classifier/content/js/arc4.js
or the new RC4 XPCOM interface requested by this bug
needs to be disabled when Mozilla is in "FIPS mode".
Darin, you went through this exercise when you added
code that calls Microsoft CryptoAPI to do MD4 for
NTLM. 

Comment 2

[Bob Lord]

Rather than keep track of crypto outside of NSS, I would rather keep all the crypto inside of NSS.  

Are there needs which would make this a challenge?

Comment 3

[Darin Fisher]

The plan is to introduce a new XPCOM API (a'la nsICryptoHash) for RC4.  It'll be implemented by PSM, probably in security/manager/ssl/src/, and it will delegate to the appropriate NSS API.  If there are any problems with that solution, then please let us know.

Comment 4

[Robert Relyea]

I have no problem exporting more crypto functionality from PSM, however
some one from NSS should review the API. Unlike the hash API, RC4 involves keys, which means we need a source of that key material. Having that application present raw keys won't always work in all environments (that is things will break if you turn FIPS on). Going around PSM won't help because doing so invalidates FIPS.

For RC4 to do any good, presumably the RC4 key came from somewhere secure (RSA operation, DH operation, PBE, etc). That means we should export that functionality as well.

If we export RC4, we should also export AES, DES, and DES3 as well (possible RC5). The API should be similiar which just initialization paramters varying.

bob

Comment 5

[Wan-Teh Chang]

US Federal government employees may need to run
the Mozilla clients in "FIPS mode" at work.  When
in FIPS mode, the Mozilla clients can only use crypto
algorithms approved by the US National Institute
of Standards and Technology (NIST).  RC4 is not a
FIPS Approved algorithm.  Therefore, we will have
to make the RC4 XPCOM interface fail in FIPS mode.

I recommend you consider switching to AES to allow
your code to work in FIPS mode.  Did you choose RC4
because it's simple enough to be implemented in
JavaScript?

Comment 6

[Robert Relyea]

OK, I should have read the original need.

1. So the keys are generated and stored locally. Sounds like SDR. Why don't we export that api to javascript.

SDR take plain text and encrypts it with a secret key stored (encrypted) in the key database. Mozilla uses this to store your passwords for websites.

2. From a cryptographer's point of view, RC-4 is a bad choice for this kind of operation. RC-4 is a stream cipher, which means 2 things which make it unsuitable for this kind of application:

1) Attackers can change underlying content without knowing the key.
2) If you encrypt more than one data stream with the same key, you expose both streams to immediate attack.

If we're talking about a component that needs to go into mozilla itself, you probably should at least have a couple of NSS engineers review the protocol (Ideally a full cryptographer should also review the protocol as well).

AES is a block cipher and does not suffer from either problems 1 or 2.

bob

Comment 7

[Tony Chang (Google)]

Attachment: v1: add nsIStreamCypher with rc4 implementation

Two comments/questions:
1) I'm not sure if this is the right way to link against freebl.
2) new/delete is a bit messy.  I could make a wrapper method for RC4_Encrypt that takes nsACStrings as parameters, but it would involve more string copying.

Comment 8

[Wan-Teh Chang]

Comment on attachment 226256 [details] [diff] [review]
v1: add nsIStreamCypher with rc4 implementation

"blapi.h" is a private NSS header file, as suggested
by the need to add -I$(DIST)/private/nss.  You need
to use the functions declared in a header in
-I$(DIST)/public/nss *and* exported in
mozilla/security/nss/lib/nss/nss.def. The patch for
this bug also needs to be reviewed by Bob Relyea
<[e-mail]>.

Comment 9

[Tony Chang (Google)]

Attachment: v2: add nsIStreamCypher with rc4 implementation

Switch to using pk11 interface.

I'll send to [e-mail] for sr after darin reviews.

Comment 10

[Darin Fisher]

Comment on attachment 226269 [details] [diff] [review]
v2: add nsIStreamCypher with rc4 implementation

>Index: security/manager/ssl/public/nsIStreamCipher.idl

>+    /**
>+     * Discard aLen bytes of the keystream.
>+     * These days 1536 is considered a decent amount to drop to get
>+     * the key state warmed-up enough for secure usage.
>+     */
>+    void discard(in long aLen);
>+};

I'm not sure I understand that comment.  Is the second sentence taken
from some other API documentation?


>Index: security/manager/ssl/src/nsStreamCipher.cpp

>+  if (nsIStreamCipher::RC4 == aAlgorithm) {

I'm a big fan of returning early if the rest of the function
body is underneath this block.  That way you can reduce
overall indentation by one level :)


>+    unsigned char* key = (unsigned char*)(PromiseFlatCString(aKey).get());

|key| now points to memory that has possibly been freed.  PromiseFlatCString
returns a reference to an object, whose lifetime determines the lifetime of
the character array returned by .get().  To do this properly, you need to
do the following:

      const nsCString& flatKey = PromiseFlatCString(aKey);
      unsigned char* key = (unsigned char*) flatKey.get();


>+  return NS_ERROR_INVALID_ARG;

You might want to use the macro, NS_ENSURE_ARG at the top of this method.


>+NS_IMETHODIMP nsStreamCipher::UpdateFromString(const nsACString& aInput)
...
>+  unsigned char* input = (unsigned char*)(PromiseFlatCString(aInput).get());

same memory management bug here.


r=darin w/ these issues resolved

Comment 11

[Robert Relyea]

> >+    unsigned char* key = (unsigned char*)(PromiseFlatCString(aKey).get());

> |key| now points to memory that has possibly been freed.  PromiseFlatCString
> returns a reference to an object, whose lifetime determines the lifetime of
> the character array returned by .get().  To do this properly, you need to
> do the following:
>
>      const nsCString& flatKey = PromiseFlatCString(aKey);
>      unsigned char* key = (unsigned char*) flatKey.get();

Whenever I see 'unsigned char *key' or 'char *key' in a crypto related application, I need to ask myself, "Why is the application mucking with key data". Doing so will break FIPS (as well as not being good crypto hygene).

We should not be exporting an API that passes raw key data over javascript. Tony can you give me a pointer to the code that is planning on calling this interface? The source of your key will determine what additional APIs we need to export.

The API should take an opaque Key object, then we can add API's to safely aquire Key objects.

Just reviewing the API, the only other issue I would bring up is that either the init function should have a IV parameter for other algorithms, or a companion init function should have such a parameter (which is up to the accepted usage in XPCOM for such things). Other than that it seems to be generic enough to handle other symetric algorithms (AES, DES, etc).

bob

Comment 12

[Robert Relyea]

>+    /**
>+     * Discard aLen bytes of the keystream.
>+     * These days 1536 is considered a decent amount to drop to get
>+     * the key state warmed-up enough for secure usage.
>+     */
>+    void discard(in long aLen);
>+};

Is this an attempt to get around the RC-4's stream cipher nature, or is there a real attack against the first 1536 bytes of RC-4?

That number sounds suspiciously like the size of some common data encrypted with RC-4. The correct way of dealing with that is to use salted keys (if you insist on using RC-4).

I never did get an answer to why RC-4. Are you really sending streamed data (like SSL?). If not RC-4 has several very undesirable characteristics. A block cipher like triple-DES or AES (or even RC-2) would be perferable.

If this is part of mozilla, you should stay with FIPS-104-2 ciphers like DES3 and AES.

bob

Comment 13

[Tony Chang (Google)]

(In reply to comment #10)
> >+    /**
> >+     * Discard aLen bytes of the keystream.
> >+     * These days 1536 is considered a decent amount to drop to get
> >+     * the key state warmed-up enough for secure usage.
> >+     */
> >+    void discard(in long aLen);
> >+};
> 
> I'm not sure I understand that comment.  Is the second sentence taken
> from some other API documentation?

I copied it from the arc4.js implementation.
http://lxr.mozilla.org/seamonkey/source/toolkit/components/url-classifier/content/js/arc4.js#81

(In reply to comment #11)
> We should not be exporting an API that passes raw key data over javascript.
> Tony can you give me a pointer to the code that is planning on calling this
> interface? The source of your key will determine what additional APIs we need
> to export.

Keys are downloaded via https using a simple GET.  The code for downloading a key is:
http://lxr.mozilla.org/seamonkey/source/toolkit/components/url-classifier/content/url-crypto-key-manager.js

The description of the server response is here:
http://wiki.mozilla.org/Phishing_Protection:_Server_Spec#GetKey_Request

> The API should take an opaque Key object, then we can add API's to safely
> aquire Key objects.
> 
> Just reviewing the API, the only other issue I would bring up is that either
> the init function should have a IV parameter for other algorithms, or a
> companion init function should have such a parameter (which is up to the
> accepted usage in XPCOM for such things). Other than that it seems to be
> generic enough to handle other symetric algorithms (AES, DES, etc).

Ok, I'll add a second init method for that.

Comment 14

[Tony Chang (Google)]

(In reply to comment #12)
> I never did get an answer to why RC-4. Are you really sending streamed data
> (like SSL?). If not RC-4 has several very undesirable characteristics. A block
> cipher like triple-DES or AES (or even RC-2) would be perferable.

Some of the data we use for phishing is licensed from third parties and opaque to us (rc4 encrypted).

Comment 15

[Wan-Teh Chang]

Bob, there was a cryptanalytic attack on RC4
in early 2000's and the defense is to discard
the first N bytes of the key stream.  (I don't
remember what N is.)

Comment 16

[Tony Chang (Google)]

Attachment: v3: add nsIStreamCipher with rc4 implementation

Includes fixes from Darin's review and adds an initWithIV method to the interface.

RE: key data
One possibility would be to change the key from a nsACString to a byte array + int length so we don't have to modify the key.  The downside to this is that it's very slow converting a string in javascript to a byte array (see
http://lxr.mozilla.org/seamonkey/source/toolkit/components/url-classifier/content/moz/base64.js#195
).

Comment 17

[Robert Relyea]

> RE: key data
> One possibility would be to change the key from a nsACString to a byte array +
> int length so we don't have to modify the key.  The downside to this is that
> it's very slow converting a string in javascript to a byte array (see

That has the same problem. If the application is supplying the bits in the key, then the api is wrong. keys should be opaque data types that you get from some other function. In order for the API to be complete you need to define an XPCom Key object that you get from somewhere else.

Can you give me a pointer to the actual application that is going to use this?

bob

Comment 18

[Tony Chang (Google)]

(In reply to comment #17)
> 
> Can you give me a pointer to the actual application that is going to use this?

http://wiki.mozilla.org/Phishing_Protection:_Design_Documentation#Remote_Lookups

As mentioned earlier, rc4 is also used to decrypt urls from a third party.

Comment 19

[Christian :Biesinger (don't email me, ping me on IRC)]

+    char *asciiData = BTOA_DataToAscii((unsigned char*)(mValue.get()),
+                                       mValue.Length());
+    _retval.Assign(asciiData);

don't you need to free this string?

this patch seems to have various changes that are not related to this bug

Comment 20

[Christian :Biesinger (don't email me, ping me on IRC)]

> don't you need to free this string?

oops, you do. not sure why I missed that...

Comment 21

[Tony Chang (Google)]

Attachment: v4: add nsIStreamCipher with rc4 implementation

drop extra files

Bob: I'd be happy to implement a opaque key object, but I'm not sure what exactly this entails.  Can you point me to an example of this?

Comment 22

[Robert Relyea]

So, I'm not quite getting what you are trying to avoid here. Is Ks a single key that you generate and share among lots of servers? (this is the only case that looks like a win to me).

The protocol is:
1. Make an SSL connection. Fetch Kc and Ks(Kc) 
                               [I presume the server can forget Kc]
2. Client encrypts the URL with MD5(Kc|nonce) using RC4 and sends the encrypted URL, the nonce and Ks(Kc).
3. Server still has Ks and can decrypt MD5(Kc|nonce).

This is to avoid the SSL connection on each request.

My concerns here are:
 1) That future SSL connections would use the already negotiated master secret, and thus be nothing more than a Hash-derive of the master secret and an RC4 encryption (pretty much identical to what you are doing now).* The only other thing SSL does is the MAC, which brings us to the second concern..
 2) That the connection is not authenticated. An attacker could substitute any of the encryted RC4 data without knowing the value of Kc or nonce since RC4 is a stream cipher. To be safe, it should be MAC'ed. In some sense this is a bit moot becasue the whole protocol can be hi-jacked by replacing Ks(Kc) with your own Ks(Kc). In short this protocol is not safe from MITM attacks between the browser and Google (either to allow all URL's or to never allow URL's). This may not be a problem, but it should be spelled out so other's don't make assumptions that the protocol is safe in this respect.
 3) The protocol has RC-4 hard wired. This means the browser must turn this feature off to go into FIPS mode. Using AES and SHA-1 (or allowing their use) would go most of the way to improving the situation. The sending of the key as plain text through an SSL pipe is still a problem for FIPS.

* I'm assuming the issue here is distribution of the masterkey's among a farm of servers, and maintaining a separate key for each client. This may indeed seem to be a good enough reason to go with this funky protocol over straight SSL, depending on the load. Still a number of other vendors seem to handle large numbers of simultaneous SSL users without issue. Moores law makes it important to continue to evaluate the assumption the 'SSL is too slow'. Google is probably the only company that I would buy that the argument may still hold.

Comment 23

[Robert Relyea]

As far as the API.

1) I would like to see the API also support AES out of the box. (should be fairly simple to add to your current patch).
2) I would like to see an key factory. I would prefer the 'create a key from raw key data' be a hidden part of the API if possible. If not, it should clearly be marked as 'don't use'. Use of this API will break in FIPS-140-2 mode.
   The factory should eventually have API's to look up fixed keys, unwrap keys from other keys (including private keys), and derive keys from other keys (including derive from concatenation and hashing).*

Keys should be xpcom objects with few public interfaces (maybe 'GetKeyObj' which returns a void * opaque object, a GetType(), etc. GetData() should not have an implemenation). Under the covers it should point to a SymKey (or maybe a SymKey, PublicKey, or PrivateKey depending on the key type).


*I'll leave it to the xpcom experts as to whether or not there should be a factory, or if these are different constructors on a 'Key' object.

Comment 24

[Robert Relyea]

Tony, 

nsIPK11Token.idl in mozilla/security/manager/ssl/public is an example of a fairly opaque object. If your key object wraps a PK11SymKey (or maybe a union of PK11SymKey, SECKEYPrivateKey, and SECKEYPublicKey) it should be about right. Export anything that you can fetch from a PK11Symkey except the Data should be fine.

bob

Comment 25

[Robert Relyea]

Tony, did I give you enough information for to build a key objects, or are there things that are still unclear.

bob

Comment 26

[Tony Chang (Google)]

Attachment: v5: add nsIKeyObject and nsIKeyObjectFactory

Still no AES (working on it), but thought I'd go ahead and post what I have so far.

Comment 27

[Mike Beltzner [:beltzner, not reading bugmail]]

--> target = mozilla1.8.1beta2 to appear on drivers' queries ...

Comment 28

[Robert Relyea]

Comment on attachment 231156 [details] [diff] [review]
v5: add nsIKeyObject and nsIKeyObjectFactory

Much better!

The only changes:
Required:   initKey should take a PK11SymKey, SECKEYPublicKey, or a SECKEYPrivateKey. For this implementation a PK11SymKey should be sufficient. 

You then move the PK11_ImportSymKey into the factory.

Strongly Suggested:

The implementation for InitWithIV is identical to Init except the IV value is collected into a SECItem and passed to PK11_ParamFromIV instead of NULL. (you can implement both functions easily by providing a private function which takes a SECItem and does all the work, Init can pass NULL, and InitWithIV would build the SECItem and pass it.

Nit's (Not required to be fixed, but would be nice):

There are several asserts that claim a problem in 'RC4', though the problem could be in some other cipher.

--- Very close.

bob

Comment 29

[Tony Chang (Google)]

Attachment: v6: init takes a key and add InitWithIV

> Required:   initKey should take a PK11SymKey, SECKEYPublicKey, or a
> SECKEYPrivateKey. For this implementation a PK11SymKey should be sufficient.
> You then move the PK11_ImportSymKey into the factory.

I changed initKey to take an algorithm and a key.  This also means that the key object is now responsible for cleaning freeing the memory used by the key.

> The implementation for InitWithIV is identical to Init except the IV value is
> collected into a SECItem and passed to PK11_ParamFromIV instead of NULL. (you
> can implement both functions easily by providing a private function which takes
> a SECItem and does all the work, Init can pass NULL, and InitWithIV would build
> the SECItem and pass it.

Done.

> There are several asserts that claim a problem in 'RC4', though the problem
> could be in some other cipher.

Oops.  Fixed.

Comment 30

[Robert Relyea]

Comment on attachment 232620 [details] [diff] [review]
v6: init takes a key and add InitWithIV

Thanks for the changes tony.
this one looks good.

bob

Comment 31

[Tony Chang (Google)]

on trunk

Comment 32

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 232620 [details] [diff] [review]
v6: init takes a key and add InitWithIV

a=beltzner on behalf of drivers, let's get this on the branch sooner than later to spot any regressions

Comment 33

[Tony Chang (Google)]

Attachment: bonecho patch (small Makefile.in differences)

Comment 34

[Tony Chang (Google)]

on branch