226271 - implement RFC 3546 (TLS v1.0 extensions)

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P1)

Description

[Mrten]

User-Agent:       Mozilla/5.0 (Windows; U; WinNT4.0; en-US; rv:1.6a) Gecko/20031025
Build Identifier: 

RFC 3546 (from June 2003) describes extensions to TLS v1.0 that are very usable,
of which the most important seems to be:

-  Allow TLS clients to provide to the TLS server the name of the
   server they are contacting.  This functionality is desirable to
   facilitate secure connections to servers that host multiple
   'virtual' servers at a single underlying network address.

it is now difficult/impossible to host multiple hosts on the same IP if you want
to use SSL. this extension makes that possible.

i'm not sure whether someone has already implemented this server-side to test it on.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Stephane Saux]

To NSS.

Comment 2

[Julien Pierre]

Bug 116168 was filed as an RFE for the "server name indication" extension . I
marked it as depending on this bug, rather than making this a dupe

This bug should be used for adding support to all the extensions in RFC3546, not
just the server name indication.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

I think it is possible that we will implement some, but not all, of the 
extensions defined in that RFC, Just as we do not implement all of the 
defined cipher suites.  As always, immediate utility in the mozilla
client products, and in the various NSS-based servers is likely to drive
this work.  

I will use this bug to drive the addition of a general TLS extension 
framework, to which code for specific extensions can be added.  
I expect the framework will define several new callbacks for libSSL.  
I expect to begin that work soon.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Add SSL error codes and alerts for TLS extensions

This is preparatory work for the TLS ECC hello extensions.

Comment 5

[Julien Pierre]

Comment on attachment 217085 [details] [diff] [review]
Add SSL error codes and alerts for TLS extensions

The name of this error has a typo :

+ER3(SSL_ERROR_CERTIFICATE_UNAVAILALBLE_ALERT , (SSL_ERROR_BASE + 105),

Also in :
+    certificate_unavailalble        = 111,

+SSL_ERROR_CERTIFICATE_UNAVAILALBLE_ALERT	= (SSL_ERROR_BASE + 105),

This is not a comment on this patch, but why do we have two copies of SSLerrs.h ? Can't SSLsample just use the file from cmd/lib ?

Comment 6

[Julien Pierre]

Comment on attachment 217085 [details] [diff] [review]
Add SSL error codes and alerts for TLS extensions

Also, RFC3546 uses "certificate_unobtainable" as the name for alert 111 rather than "certificate_unavailable" . We should probably use the same name as the RFC.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v2 (checked in)

Well, at least I was consistent with my typos!  :)
Thanks for catching those.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

This second patch also changed some undesirable tabs to spaces.
In answer to your questoin about why two copies of sslerrs.h,
the answer is: ask the person who wrote sslsample.  
Fixing sslsample is about P5 right now.  

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 217090 [details] [diff] [review]
patch v2 (checked in)

request second review for 3.11 branch.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 217090 [details] [diff] [review]
patch v2 (checked in)

cmd/SSLsample/SSLerrs.h; new revision: 1.3; previous revision: 1.2
cmd/lib/SSLerrs.h;       new revision: 1.4; previous revision: 1.3
lib/ssl/ssl3prot.h;      new revision: 1.12; previous revision: 1.11
lib/ssl/sslerr.h;        new revision: 1.5; previous revision: 1.4

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Generic extension implementation, v1 (checked in)

This patch adds:
- handling of new alerts

- ssl3_CallHelloExtensionSenders() which goes through table of registered 
  functions to format extensions, and calls them to append their extensions, 
  or to merely report the length they *would* append if asked to do so.  
  This function gets called twice, once to get the lengths, and again to 
  append. This function is used to send both client and server hello 
  extensions.  It is called for client hellos ONLY if TLS is enabled.
  Presently, the table of senders of client hello extensions is static.  
  Dynamic registration is not done.  But dynamic registration *is* done 
  for the server side.

- ssl3_HandleClientHelloExtensions(), goes through the received client hello 
  extensions, distributing them to functions registered for each type, if any.  
  Presently the table of registerd client hello extension handlers is static.  
  There is not yet a table of server hello extension handlers.  Server hello 
  extensions are ignored.

- ssl3_SendServerNameIndicationExtension() formats and appends an SNI 
  extension only if ECC cipher suites are enabled.  Uses the "URL" name that
  the app registers with libSSL, provided that it is not a dotted decimal 
  value.  

- ssl3_HandleServerNameIndicationExtension() handles a received SNI extension
  by discarding it.  (Room for enhancement here. :)

- ssl3_RegisterServerHelloExtensionSender() allows dynamic registration of 
  sender functions for server hello extensions.  Used to send replies only 
  to the extensions received from the client.

- ssl3_AppendHandshakeNumber() and ssl3_ConsumeHandshakeNumber() are no 
  longer static to ssl3con.c, so that the extension senders and handlers 
  in ssl3ecc.c can use them.

I will ask Vipul and Bob to review, but others are welcome.

Comment 12

[Vipul Gupta]

Comment on attachment 217133 [details] [diff] [review]
Generic extension implementation, v1 (checked in)

Due to time constraints, I haven't been able to spend a lot of time on the review. Having said that, here are a few questions/comments:
1. In this pach, the extension handler doesn't really do much with the extension (it ignores it). Typically, the extension appended to the ServerHello will   depend on the extensions included with the incoming Client Hello. Am I correct in assuming that any state that needs to be saved between the time the incoming extension is processed and the response extension is generated will be stored in the sslSocket structure?

2. What happens if sslv2 is enabled? Will a ClientHello in v2 format still carry the TLS extensions (SSLv2 does not have a notion of hello extensions).

3. Typo in one of the comments inside ssl3_HandleClientHelloExtensions: "find extension_type *in* (instead of *is*)"

vipul

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 217133 [details] [diff] [review]
Generic extension implementation, v1 (checked in)

Checked in, with Vipul's suggested changes, on trunk only.
Checking in ssl3con.c;  new revision: 1.85; previous revision: 1.84
Checking in ssl3ecc.c;  new revision: 1.6; previous revision: 1.5
Checking in sslimpl.h;  new revision: 1.48; previous revision: 1.47

Comment 14

[Robert Relyea]

Comment on attachment 217090 [details] [diff] [review]
patch v2 (checked in)

r+=relyea

Comment 15

[Robert Relyea]

Comment on attachment 217133 [details] [diff] [review]
Generic extension implementation, v1 (checked in)

r=relyea

Minor comments, do not need to be fixed before checkin..

>	PRUint32 maxBytes = 65535; /* 2^16 - 1 */
>	PRInt32  extLen;
>
>	extLen = ssl3_CallHelloExtensionSenders(ss, PR_FALSE, maxBytes, NULL);
>	if (extLen < 0) {
>	    return SECFailure;
>	}
>
>	maxBytes        -= extLen;
>

The subtraction above is unnecessary as maxBytes goes out of scope in 3 more lines without being used.

---
At some point it would be nice to allow the register function to be accessible to the application. At that point we would have to make the static array dynamic. That is not the goal of this code, so the comment does not indicate a need to change anything.

bob

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 217090 [details] [diff] [review]
patch v2 (checked in)

Checked in on branch
cmd/SSLsample/SSLerrs.h; new revision: 1.2.28.1; previous revision: 1.2
cmd/lib/SSLerrs.h;       new revision: 1.3.28.1; previous revision: 1.3
lib/ssl/ssl3prot.h;      new revision: 1.10.2.2; previous revision: 1.10.2.1
lib/ssl/sslerr.h;        new revision: 1.4.28.1; previous revision: 1.4

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 217133 [details] [diff] [review]
Generic extension implementation, v1 (checked in)

Checked in on 3.11 branch.
Checking in ssl3con.c; new revision: 1.76.2.9; previous revision: 1.76.2.8
Checking in ssl3ecc.c; new revision: 1.3.2.3; previous revision: 1.3.2.2
Checking in sslimpl.h; new revision: 1.42.2.5; previous revision: 1.42.2.4