implement RFC 3546 (TLS v1.0 extensions)

RESOLVED FIXED in 3.11.1

Status

NSS
Libraries
P1
enhancement
RESOLVED FIXED
14 years ago
10 years ago

People

(Reporter: Mrten, Assigned: Nelson Bolyard (seldom reads bugmail))

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: ECC, URL)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

14 years ago
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

14 years ago
To NSS.
Assignee: ssaux → wchang0222
Component: Client Library → Libraries
Product: PSM → NSS
QA Contact: bmartin → bishakhabanerjee
(Assignee)

Updated

14 years ago
Blocks: 236245

Updated

14 years ago
Blocks: 116168

Comment 2

14 years ago
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.
(Assignee)

Updated

13 years ago
QA Contact: bishakhabanerjee → jason.m.reid

Updated

12 years ago
Depends on: 284450

Updated

12 years ago
Assignee: wtchang → nelson
Priority: -- → P1
Target Milestone: --- → 3.12
(Assignee)

Comment 3

12 years ago
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.
(Assignee)

Updated

12 years ago
Target Milestone: 3.12 → 3.11.1
Version: unspecified → 3.11
(Assignee)

Updated

12 years ago
Whiteboard: ECC
(Assignee)

Updated

12 years ago
QA Contact: jason.m.reid → libraries
(Assignee)

Comment 4

12 years ago
Created attachment 217085 [details] [diff] [review]
Add SSL error codes and alerts for TLS extensions

This is preparatory work for the TLS ECC hello extensions.
Attachment #217085 - Flags: superreview?(rrelyea)
Attachment #217085 - Flags: review?(julien.pierre.bugs)

Comment 5

12 years ago
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 ?
Attachment #217085 - Flags: review?(julien.pierre.bugs) → review-

Comment 6

12 years ago
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.
(Assignee)

Comment 7

12 years ago
Created attachment 217090 [details] [diff] [review]
patch v2 (checked in)

Well, at least I was consistent with my typos!  :)
Thanks for catching those.
Attachment #217085 - Attachment is obsolete: true
Attachment #217090 - Flags: review?(julien.pierre.bugs)
Attachment #217085 - Flags: superreview?(rrelyea)
(Assignee)

Comment 8

12 years ago
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.  

Updated

12 years ago
Attachment #217090 - Flags: review?(julien.pierre.bugs) → review+
(Assignee)

Comment 9

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

request second review for 3.11 branch.
Attachment #217090 - Flags: review?(rrelyea)
(Assignee)

Comment 10

12 years ago
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
Attachment #217090 - Attachment description: patch v2 → patch v2 (checked in)
(Assignee)

Comment 11

12 years ago
Created attachment 217133 [details] [diff] [review]
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.
Attachment #217133 - Flags: superreview?(rrelyea)
Attachment #217133 - Flags: review?(vipul.gupta)

Comment 12

12 years ago
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
Attachment #217133 - Flags: review?(vipul.gupta) → review+
(Assignee)

Updated

12 years ago
Attachment #217090 - Attachment description: patch v2 (checked in) → patch v2 (checked in on trunk only)
(Assignee)

Comment 13

12 years ago
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
Attachment #217133 - Attachment description: Generic extension implementation, v1 → Generic extension implementation, v1 (checked in on trunk)

Comment 14

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

r+=relyea
Attachment #217090 - Flags: review?(rrelyea) → review+

Comment 15

12 years ago
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
Attachment #217133 - Flags: superreview?(rrelyea) → superreview+
(Assignee)

Comment 16

12 years ago
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
Attachment #217090 - Attachment description: patch v2 (checked in on trunk only) → patch v2 (checked in)
(Assignee)

Comment 17

12 years ago
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
Attachment #217133 - Attachment description: Generic extension implementation, v1 (checked in on trunk) → Generic extension implementation, v1 (checked in)
(Assignee)

Updated

12 years ago
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.