405155 - add support for TLS-SRP, rfc5054

Status: NEW

(NSS :: Libraries, enhancement, P3)

Description

[Steffen Schulz]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b1) Gecko/2007110903 Firefox/3.0b1
Build Identifier: 

Attached patches enhance NSS with SRP authentication in TLS.

A very basic softtoken has been added, so that PK11_PubDerive() and PK11_GenerateKeyPair() can be used. I've tried to break the algorithm in to pieces that match the usual procedures, i.e. create pubkey/prvkey get the peers pubkey and derive a common secret. I've considered that real tokens, storing long-term secrets v at the server or P and the client, could be in use may or may not actually know the secret. So the interfaces are such that the application is always asked for the parameters, which may then be overridden by real tokens in the GenerateKeyPair-steps. In case some parameters are not provided, no error is reported until entering the softoken code.

Contrary to suggestions on nss-dev, IMO the key derivation must not be broken down further, using other token/softtoken, as this would allow attacks on (hypothetical) real SRP tokens. The GenerateKeyPair() for the client happens to be a simple ephermal DH key pair with a different secret key bitlength. For consistency, I nevertheless created a function in freebl/srp.c.

srp.h contains the group parameters from the rfc in base64, as well as a few other constants. Its exported and used in the applications as well. A new app cmd/srputil is introduced to manage the verifier file (think: pw-hashes at the server). cmd/tstclient and cmd/SSLSample/server have been modified to provide login information(username/pass resp. verifier-data) to SSL. There is a non-interactive SSL_SetUserLogin function and an interactive callback to set the password after the server key exchange is received. This way, the UI could ask the users password only after certificate was validated and displayed.

It's unfortunate that the server app needs to know the srp group parameters to supply them along with the verifier to the server SSL code. This should maybe be changed to optionally supply only a groupid that SSL looks up in srp.h itself.

The patch works with bypass and PK11-paths, all group sizes specified in the rfc, optional rsa or dsa server certificates. It works against the old patched openssl library and, by changing the tls ids, with gnutls 2.1.6. And of course nss itself.

I tell all this of course so that (hopefully small) issues I'm not aware of can be brought forward without reading all the code. Oh, maybe someone also knows why dsa certifcates are indexed with kt_null.

How do you use it?

create a verifier file: srputil -a -u user1 -p mypass -s vfile
start a server: server -p 4433 -s vfile -d certs -n rsa_server
start a client: tstclnt -o -3 -2 -c :c01b -h 127.0.0.1 -p 4433 -d certs -l user1 -k mypass

Both programs accept -B for bypass-mode.

Reproducible: Always

Comment 1

[Steffen Schulz]

Attachment: V1 patches ssl, pk11wrap, freebl, softoken

Comment 2

[Steffen Schulz]

Attachment: patches SSLSample, tstclnt, adds srputil

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

I confirm that this is a trunk enhancement request. :)

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 289963 [details] [diff] [review]
V1 patches ssl, pk11wrap, freebl, softoken

Steffen,

Welcome to bugzilla.  :)
You've asked yourself for review of this "monster" patch. :)
I'll ask myself to review it instead.

A few preliminary patch review questions and comments:

Q1) In pkcs11t.h, You've defined a bunch of new symbols in the PKCS#11 
symbol name space and various PKCS#11 standard-controlled number spaces, 
rather than in the "vendor defined" number spaces, such as:
+#define CKK_SRP             0x00000026
+/* SRP attributes */
+#define CKA_SECRET             0x00000190
+#define CKA_USER               0x00000191
+#define CKA_SALT               0x00000194
+#define CKM_SRP_SERVER_KEY_PAIR_GEN    0x0000002A
+#define CKM_SRP_CLIENT_KEY_PAIR_GEN    0x0000002B
+#define CKM_SRP_DERIVE                 0x0000002C
+#define CKR_SRP_UNSUPPORTED_GROUP             0x00000301
+#define CKR_SRP_ILLEGAL_PARAMETER             0x00000302
 
+typedef struct CK_SRP_PARAMS {
 ...
+} CK_SRP_PARAMS;

Have those symbols and values been recognized as standard extensions by 
the PKCS#11 working group?  Are the 3 new mechanisms now standardized 
mechanisms? Or do we need to switch to numbers in the vendor-defined spaces?

2) Your patch collides in a big way with the patch for bug 403563. 
Assuming both patches get approved, whichever of those two gets approval 
second will have to produce an updated patch to deal with the changes 
from the other checkin.

3) The big strings in srp.h must move to a .c file somewhere, I think.  
What do they contain?  Do those values come from an RFC or other document?
If so, that should be cited.  It would be good to have another 
representation  available in a comment, like decimal or hex.

4) If this review should determine that other changes must be made, there
are a lot of other minor things that should be done different in any
subsequent version of this patch.  I'll let you know what those are if/when
the time comes.

Comment 5

[Steffen Schulz]

Q1) I'm not aware of any efforts for standardizing an interface for srp-tokens. So yes, if the symbols feel better over there they should be moved. 

I introduced CKR_* because these exakt two errors must be propageted back into SSL, but maybe they can exchanged for one of the existing error symbols.

There are two CKM_SRP_KEY_PAIR_GEN mechanisms as this seemed to be the easiest way to let the token know if we are server or client. CKM_SRP_DERIVE on the other hand includes a boolean in the key template.


Q2) It looks as these conflicts are mostly because of refractured/moved code. Needed to be done anyway. I tried not to make unnecassary changes, although I accidently renamed two structs in the patch('TotallyUnusedClientHello')


Q3) All groups are from RFC5054 which took them from other RFCs. 5054 explicitly allows any group to be used, but then they it must be checked if the prime is safe and g is a primitive generate, so whitelists are used instead.

There were multiple srp.h files at first, now one file is exported.
- The client code MUST check group parameters against a whitelist. This is done in srp.c. Hex or base64 could be used to strcmp() the values. The alternative is to check if the prime is a sophie germain prime and g a primitive root.
- The verifier file contains {username, group-id, salt, verifier}. So srputil and the server application need to have some mapping group-id<->group to provide the group to SSL.
- Some anonymous group-id is not enough for srputil, it needs to compute the v=g^P%N. But the server could optionally only specify a valid group-id to SSL.
- SRP_BUFLEN is only used in srputil and server, but depends on the maximum length of base64-strings.
- The openssl patch defines a callback so that the app can allow for additional group parameters. Probably not relevant, as many group sizes are covered and primes don't worn out. :)

Q4) Sure.

Comment 6

[Ralf Hauser]

see also bug 268835

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 289963 [details] [diff] [review]
V1 patches ssl, pk11wrap, freebl, softoken

OK, based on the answers in Steffen's comment 5, I conclude that

a) all the CK[AMR]_* symbols defined in this patch must be allocated
from "vendor defined" space, not from standards-defined space.  

They should also all have names that identify them as related to SRP
and perhaps as being part of the NSS vendor-defined symbols,
e.g. CKA_NSS_SRP_SECRET instead of CKA_SECRET

b) The large constants that this patch defines as big strings in srp.h 
must move to a .c file somewhere, and maybe should change form into a
form that doesn't require conversion.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 289964 [details] [diff] [review]
patches SSLSample, tstclnt, adds srputil

asking myself to review this patch, also.

Comment 9

[Steffen Schulz]

Attachment: V2 patches ssl, pk11wrap, freebl, softoken

Okay...I didn't want to upload a new version during your review, but if there is nothing else to change for now, here it is.

a) The new symbols CK[KAMR]_* are now counting up from 0x8000000.
b) srp.h is removed, constants are defined in in .h files of the respective modules. srp.c now compares SECItems against stored hex-strings. It still needs conversion+strcmp, but I don't know how to fix this.

srputil needs these parameters encoded as SECItem and as base64, so storing them as base64 should be okay here? I'd also like to change srputil to write the group values into a second file, so that the server app doesn't need to have them hard-coded.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 297121 [details] [diff] [review]
V2 patches ssl, pk11wrap, freebl, softoken

Preliminary review comments.  Full review not done yet. 
Don't need another patch immediately.  More review comments
will follow, eventually. :)

1. srp.h is still in nss/lib/freebl/manifest.mn

2. The patch to nss/lib/softoken/pkcs11t.h still defines CKK_SRP
rather than CKK_NSS_SRP.

3. The definitions of all the CKx_NSS_* symbols belong in 
nss/lib/softoken/pkcs11n.h rather than in nss/lib/softoken/pkcs11t.h
and the values associated with them must not conflict with values 
already assigned to other NSS vendor-defined symbols.

4. I'm not sure wherer the declaration of 
typedef struct CK_SRP_PARAMS { ... } CK_SRP_PARAMS 
belongs, but I think it belongs in pkcs11n.h, too.
I'll ask Bob Relyea.

5. Please add a comment to this bug, explaining what the srputil
program does, and how to use it.  I think eventually we'll want 
you to add code to the ssl.sh test script for it.

6. I see (in the other patch) that you've added support for SRP to
the programs in the SSLSample directory, and to tstclnt, but not
to "selfserv".  Have you actually tested with those programs?

We don't presently use SSLSample, and are contemplating removing it.  
We use tstclnt, strsclnt and selfserv for all our SSL testing.  
So I think we will want you to add support to selfserv and strsclnt.  
But Don't do that just yet.  I want to study the server side code 
more first.

Comment 11

[Steffen Schulz]

5) srputil is a tool to manage the user database at the server. For each user, the server must know username, srp-group, salt and verifier, where the verifier is much like a hash of the user password. It creates the database file if nonexistant. The file format is pretty obvious and documented in the code.

Main use cases:
create an entry wit 1024 bit srp group:
  srputil -a -g 1024 -u testuser -p testpass -s vfile
reset password for user:
  srputil -r -u testuser -p newpass -s vfile
delete user from file:
  srputil -d -u testuser -s vfile

6)
I've not tested with selfserv or strsclnt. Example cmd lines for tstclnt and SSLSample are in the first comment. But note that the missing srp.h breaks the cmd-patch.
The v2 srputil has to keep its own set of good srp groups and will also create a file 'vfile.conf' which contains the groups, so that the v2 server app can look for the group identifers from vfile in vfile.conf.

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 289964 [details] [diff] [review]
patches SSLSample, tstclnt, adds srputil

I am canceling the review requests for the patches attached
to this bug.  If & when the day comes that people are doing 
the work to add the necessary UI support in Firefox and/or 
Thunderbird, then new review requests for these patches can be 
made.

Comment 13

[Mike Shaver (:shaver emeritus)]

UI support for these things will likely first manifest as extensions -- is this NSS support something that can be added by such an extension as well?  The UI will need a lot of experimentation to get right, and I'd really love it if that experimentation wasn't blocked on people needing to build a custom NSS.  (And I really don't want to encourage people to build a custom NSS at all, to be honest; rather leave that to the experts. :) )

Comment 14

[Steffen Schulz]

The main problem with the interface is the layer between ssl and chrome. SRP-enabled sites need to be detected, so the user is not asked for a SRP username at every SSL site. Then it would be nice if the UI would know priliminary results from the ssl exchange before the handshake is finished, so that the user sees the certificate common name before typing his password. A few error conditions must also be handled differently, e.g. 'bad record mac'.

This layer needs to be implemented before people can play with the chrome. I started, but i've been too busy with my masters thesis. (frankly, its also been a pain so far and i don't know anything about chrome)

I also think that it should be carefully evaluated if SRP should be implemented in SSL, as in my patch, or tunneled through http, as it is done in bug 356855.
One solution may very well have an advantage as to how the protocol fails if attacked and how resistant it can be made against spoofed websites/security indicators/whatever. (I'm not sure what the better approach is, I just implemented the RFC and heard about the other one later on)

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Any discussion about things that must be implemented outside of NSS 
should take place in bug 356855.  Is some sense, comment 12 was just a 
long winded way of saying that this bug is really blocked by bug 356855.

Yes, I believe there is a way to add SRP to NSS as a plugin.  The freebl
and softoken parts of the preceding patch can be plugged into NSS as a 
PKCS#11 module, which is a shared lib conforming to the PKCS#11 API.
There are a number of good reasons to do it that way, too.  

NSS provides a source implementation of a bare bones PKCS#11 module.
It doesn't do any crypto, but it has all the framework into which one would
place any "mechanisms" that one implemented.  Not sure about key storage.

Comment 16

[Quinn Slack]

Attachment: updated to current; plus tests, standard SRP passwd file, selfserv

I have modified this patch to:

* use the same format for the SRP passwd and group param file as libsrp, so that the standard srptool (provided by libsrp or GnuTLS) can be used. This means that srputil (which duplicated srptool's functionality) is no longer necessary to include in this patch. (This means either libsrp or GnuTLS are required to generate SRP passwd files. Is this OK, or does NSS need to be fully self-contained? For the test suite, I have included pre-generated SRP passwd files.)

* implement TLS-SRP in selfserv instead of SSLsample.

* include TLS-SRP tests. (These currently fail because of a memory leak; ignoring that, they pass.)

* apply cleanly against nss-3.12.9.


tstclnt usage:
tstclnt -l jsmith -k abc -h tls-srp.test.trustedhttp.org -d /tmp/certs/ -c :C01D -o -3 -2
# then GET /

selfserv usage:
cd $NSS/mozilla/security/nss/tests
selfserv -n localhost -p 4443 -v -H ssl/tpasswd.conf -K ssl/tpasswd -d /tmp/certs/
tstclnt -p 4443 -h localhost -f -d /tmp/certs -v -l TestUser -k nss -2 -c :C01D


This patch still needs a lot more work before it's ready for serious review, but I wanted to get some feedback on it now. 

Re: demand for TLS-SRP: In the last 3 years, TLS-SRP support has been added to cURL and (shortly) OpenSSL. It's been in GnuTLS for a long time. I have a patch to add TLS-SRP support to Chrome at http://trustedhttp.org/wiki/TLS-SRP_in_Chrome (code review at http://codereview.chromium.org/6804032/) that depends on this NSS patch. John Engler has a Firefox add-on at http://jengler.webs.com/pake.htm, and TLS-SRP would fit in very well with Firefox 4.1's planned Account Manager feature (I have made some progress on a TLS-SRP profile for Acct Mgr). Much of this growing interest in TLS-SRP is because some PAKE patents expired recently; also, CAs are perceived as more vulnerable now than a few years ago, and in certain cases TLS-SRP is a good substitute for or supplement to certificate auth. I hope this addresses the concern over lack of demand for TLS-SRP 3 years ago.

Re: implementing it as a loadable PKCS#11 module. I don't know much about PKCS#11, but it doesn't seem to cleanly apply to TLS-SRP. Any advice here?

Re: eliminating srputil and adding a dependency on either libsrp or GnuTLS srptool to generate SRP passwd files. Is this OK? Or does NSS need to be self-contained and have its own srputil?

Comment 17

[Johnathan Nightingale [:johnath]]

Hey Quinn,

Thanks for jumping in here - it's nice to see the work picking up again. I'll leave the NSS questions to NSS peers, but I appreciate the work you've already put in.

Comment 18

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

It would be better to have the patch split into two parts: one for SRP in FreeBL & Softoken, and one for libssl, because we may want SRP without TLS-SRP, and because there may be different reviewers for those two parts. That is, we should fix bug 356855 without using TLS-SRP.

I do not think it is realistic to deploy TLS-SRP as is, because it sends the username (often the user's email address) in the clear. This is a problem that would have to be solved before it becomes practical to use.

See http://groups.google.com/group/mozilla.dev.tech.crypto/msg/74ed374c68145bdc

Comment 19

[Quinn Slack]

Hey Brian,

Got it. I'll separate the patch. As for bug 356855, I'll work on the NSS part of it, but I'm skeptical that the SRP HTTP auth scheme it proposes is workable. I'll ping the original submitter to see if he's still interested.

> I do not think it is realistic to deploy TLS-SRP as is,
> because it sends the username (often the user's email address)
> in the clear. This is a problem that would have to be solved
> before it becomes practical to use.

Yes, true. I had not thought specifically of the email case. That would make email harvesting on TLS-SRP significantly easier for spammers (both as active and passive network attackers) than on plain HTTP. We're working on a fix that would basically hash the SRP username in the Client Hello. This would prevent attackers from learning new email addresses or usernames, but it would let them test guesses of the form "is this traffic from [e-mail]?". I will post to mozilla.dev.tech.crypto soon about this.

Comment 20

[John Engler]

Sorry for being late to the party. I am not sure what open issues remain for this ticket but I would love to help out however I can.

Comment 21

[Philipp Gühring]

Client certificate Authentication with TLS has the same problem, they also leak the name and email adress in the TLS handshake. (Frustratingly nobody objected the implementation of client certificate authentication before the problem was solved there.) A generic solution for both problems would be great.
One idea I heard for the client certificates was to start with a DHE handshake and then have the client certificate be encrypted already to the DHE key, which would limit the leak to active attackers only. This should not add additional roundtrips like IE/IIS does.