Closed Bug 403563 (tlsste) Opened 17 years ago Closed 16 years ago

Implement the TLS session ticket extension (STE)

Categories

(NSS :: Libraries, enhancement, P2)

enhancement

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: wtc, Assigned: ngm+mozilla)

References

(Blocks 2 open bugs)

Details

Attachments

(10 files, 13 obsolete files)

7.41 KB, text/plain
Details
128.35 KB, patch
Details | Diff | Splinter Review
125.84 KB, patch
Details | Diff | Splinter Review
12.63 KB, patch
Details | Diff | Splinter Review
98.97 KB, patch
Details | Diff | Splinter Review
138.96 KB, patch
nelson
: review+
Details | Diff | Splinter Review
722 bytes, patch
nelson
: review+
Details | Diff | Splinter Review
137.46 KB, patch
Details | Diff | Splinter Review
34.57 KB, patch
Details | Diff | Splinter Review
11.33 KB, patch
Details | Diff | Splinter Review
Attached patch Proposed patch v1 (obsolete) — Splinter Review
We would like to implement the TLS session ticket extension as specified in
the Internet-Draft draft-salowey-tls-rfc4507bis-01.txt.  This extension allows
session resumption without server-side state.

The attached patch is contributed by Nagendra Modadugu of Google.  It still
needs a little polishing, but I invite you to skim through it and give us
early comments.

The following known issues are outside the scope of this enhancement request.

1. The server-side key management functions haven't been implemented.

2. If the server is running in the bypassPKCS11 mode, the master secret
in the session ticket is unwrapped.  Otherwise, the master secret in the
session ticket is wrapped.  Of course, the session ticket is always
encrypted.

The session ticket extension is disabled by default.  An application needs
to set an SSL socket option to enable the session ticket extension.

The test programs strsclnt, tstclnt, and selfserv have a new -u command-line
option to enable the session ticket extension.  I added new SSL stress tests
in nss/tests/ssl/sslstress.txt to test stateless resumption using session
tickets.
Priority: -- → P2
Priority: P2 → P1
Whiteboard: NSS312B1
Priority: P1 → P2
Whiteboard: NSS312B1
We believe that this feature must not be enabled by default in NSS.
If PSM wants it, PSM would need to enable it.
There probably ought to be a preference or other means by which the 
user can disable it if it causes problems for him.

Is there a separate bug for PSM to implement these things?
Comment on attachment 288414 [details] [diff] [review]
Proposed patch v1

I will take on the review task.
Attachment #288414 - Flags: review?(nelson)
(In reply to comment #1)
> Is there a separate bug for PSM to implement these things?

I haven't seen such a bug yet, if it shall be the responsibility of a client app to enable this new feature, I agree such a bug should be filed.

Questions that should probably answered in the PSM bug:
Should all clients client enable this option?
Should it be used by default for any application protocols (wrapped in TLS or STARTTLS), or should it be limited to https?
I guess it's theoretically possible to see compatibility problems with servers claiming to support STE, but behave incorrect? I guess this would introduce new scenarios where we would have to do an TLS intolerance retry.
Alias: tlsste
Summary: Implement the TLS session ticket extension → Implement the TLS session ticket extension (STE)
Blocks: 404034
Comment on attachment 288414 [details] [diff] [review]
Proposed patch v1

Nelson, could you skim through this patch and give us some
high-level comments first?

For detailed code review, would it help if we split this
patch into the smaller patches?  I should be able to split
it into these four patches, and you can review them in
this order:

1. Refactoring of the TLS extension handling code, moving
it from ssl3ecc.c to the new file ssl3ext.c.

2. Client-side implementation of session tickets.

3. Server-side implementation of session tickets.

4. Code that allows NSS users to enable session tickets,
and changes to the SSL test programs and new SSL test cases.
This is the first part of the complete patch.

ssl3ecc.c has some TLS extension related code at the end.
This sub-patch moves it to the new file ssl3ext.c.

I will provide diffs of the moved code tomorrow to help you
review this sub-patch.
Attachment #290825 - Flags: review?(nelson)
Not a patch.  Intended to help code reviewers review
moved code.
Not a patch.  Intended to help code reviewers review
moved code.
Comment on attachment 288414 [details] [diff] [review]
Proposed patch v1

Today, Wan-Teh asked for a preliminary review of the big patch.
So this is that preliminary review.  While I was doing that review,
I see that Wan-Teh produced a newer patch set, but these comments 
apply to the original patch.  In no particular order...

1. Indentation problems.  Lots of new code seems to be improperly
indented.  Many Newly inserted cases in switch statement have this problem.

2. The types in the file sslt.h are public.  Therefore, to retain binary
compatiblity, it is important not to add new members into the middle of
existing structure types.  The additions to struct SSL3StatisticsStr 
must all come at the end.  It's OK to extend this structure, as long as
the parts previously defined are unchanged.

3. Please explain the use of the renegotiate option to tstclnt, and why
strsclnt doesn't need a similar option.

4. Shouldn't strsclnt's new -r option take an option argument with the 
numeric value, rather than hardcoding it to 5?

5. The changes to ssl3con were to big for a preliminary review, but 
please move the new function ssl3_SendNewSessionTicket, and any static 
functions called exclusively by it, to the new file ssl3ext.c

6. haven't reviewed ssl3ecc.c changes.  Moving the SNI-related functions
to ssl3ext.c is OK in principle.  I haven't reviewed them in detail.
Did the functions change any, other than being moved?

7. have not reviewed ssl3ext.c, nor sslimpl.h, nor sslstress.txt

8. ssl3prot.h comments:

8a) please keep the enumerations in SSL3ShanshakeType in ascending 
numeric order.

8b) I believe that the new struct SessionTicket is a wrapped entity.
That is, it is not sent over the wire as-is, but is encrypted first.
(Right?) Its contents are not actually defined by the TLS session 
ticket RFC, but are implementation dependent.  As such, this file is
the wrong place for it.  This file should contain structs and enums
that are essentially representations of the SSL/TLS message contents
as defined in/by the RFC.  Moving that struct out of this file should
mean that it is not necessary to #include sslt.h in this file.
I consider the need to include sslt.h as a clue that this file 
contains something it shouldn't.

9. sslgather.c  
Instead of using memset, please define a static const copy of this
struct.  No need to initialize it, since the c language guarantees
it will contain zeros.  Then do a structure assignment, rather than
a memset call.  For small structs like this, that's generally cheaper 
than a memset call on most platforms.  

BTW, why this change?  Seems OK, but unnecessary.

10. in sslnonce.c, some of this code is shared by and common to both 
the client and server SID caches.  So I must ask: will the value of 
sid->u.ssl3.session_ticket.ticket.data ever be non-NULL in servers?


11. Other than the above issues, (which need to be fixed :-) the changes 
for the following files seem OK as-is:

selfserv.c
strsclnt.c
tstclnt.c
manifest.mn
ssl.h
sslerr.h
sslsock.c (except for indentation)
sslt.h    (need to move additions to end of struct SSL3StatisticsStr)

I will give this r-, in light of the above issues, and since I already know 
that you'll submit another patch. :-)
Attachment #288414 - Flags: review?(nelson) → review-
Minor correction.  I wrote:
> 4. Shouldn't strsclnt's new -r option take an option argument with the 
> numeric value, rather than hardcoding it to 5?

That should have said tstclnt, not strsclnt.
This diffs file is a better way to review the changes made to
the TLS extension code that was moved from ssl3ecc.c to the
new file ssl3ext.c.  This file was generated by

  diff -pu8 ssl3ecc-part.c ssl3ext.c

where ssl3ecc-part.c is the portions of ssl3ecc.c that were
moved, reordered slightly to make the diffs more readable.
Attachment #290826 - Attachment is obsolete: true
Attachment #290827 - Attachment is obsolete: true
Thanks for the comments, a new patch will be forthcoming.  Answers to questions inline below:

(In reply to comment #8)
> (From update of attachment 288414 [details] [diff] [review])
> Today, Wan-Teh asked for a preliminary review of the big patch.
> So this is that preliminary review.  While I was doing that review,
> I see that Wan-Teh produced a newer patch set, but these comments 
> apply to the original patch.  In no particular order...
> 
> 1. Indentation problems.  Lots of new code seems to be improperly
> indented.  Many Newly inserted cases in switch statement have this problem.
> 
> 2. The types in the file sslt.h are public.  Therefore, to retain binary
> compatiblity, it is important not to add new members into the middle of
> existing structure types.  The additions to struct SSL3StatisticsStr 
> must all come at the end.  It's OK to extend this structure, as long as
> the parts previously defined are unchanged.
> 
> 3. Please explain the use of the renegotiate option to tstclnt, and why
> strsclnt doesn't need a similar option.

The renegotiate option is a way to do session resumes (which tstclnt does not otherwise support), providing an easy way to do one-off tests for the session ticket extension.  strsclnt already does session resumes, so there was no need for additional flags.

> 
> 4. Shouldn't strsclnt's new -r option take an option argument with the 
> numeric value, rather than hardcoding it to 5?
> 

Changed so that strsclnt takes a numeric value as argument.

> 5. The changes to ssl3con were to big for a preliminary review, but 
> please move the new function ssl3_SendNewSessionTicket, and any static 
> functions called exclusively by it, to the new file ssl3ext.c
> 
> 6. haven't reviewed ssl3ecc.c changes.  Moving the SNI-related functions
> to ssl3ext.c is OK in principle.  I haven't reviewed them in detail.
> Did the functions change any, other than being moved?

The extension processing loop was modified to do better error handling (e.g. clients check that a received extension was actually advertised via the ClientHello).

> 
> 7. have not reviewed ssl3ext.c, nor sslimpl.h, nor sslstress.txt
> 
> 8. ssl3prot.h comments:
> 
> 8a) please keep the enumerations in SSL3ShanshakeType in ascending 
> numeric order.
> 
> 8b) I believe that the new struct SessionTicket is a wrapped entity.
> That is, it is not sent over the wire as-is, but is encrypted first.
> (Right?) Its contents are not actually defined by the TLS session 

This is correct -- struct SessionTicket is a wrapped entity.  Moved to sslimpl.h.

> ticket RFC, but are implementation dependent.  As such, this file is
> the wrong place for it.  This file should contain structs and enums
> that are essentially representations of the SSL/TLS message contents
> as defined in/by the RFC.  Moving that struct out of this file should
> mean that it is not necessary to #include sslt.h in this file.
> I consider the need to include sslt.h as a clue that this file 
> contains something it shouldn't.
> 
> 9. sslgather.c  
> Instead of using memset, please define a static const copy of this
> struct.  No need to initialize it, since the c language guarantees
> it will contain zeros.  Then do a structure assignment, rather than
> a memset call.  For small structs like this, that's generally cheaper 
> than a memset call on most platforms.  
> 
> BTW, why this change?  Seems OK, but unnecessary.

I added the memset while debugging; there is no other reason for change (which I have removed).

> 
> 10. in sslnonce.c, some of this code is shared by and common to both 
> the client and server SID caches.  So I must ask: will the value of 
> sid->u.ssl3.session_ticket.ticket.data ever be non-NULL in servers?
> 

sid->u.ssl3.session_ticket.ticket.data is only used by clients.

> 
> 11. Other than the above issues, (which need to be fixed :-) the changes 
> for the following files seem OK as-is:
> 
> selfserv.c
> strsclnt.c
> tstclnt.c
> manifest.mn
> ssl.h
> sslerr.h
> sslsock.c (except for indentation)
> sslt.h    (need to move additions to end of struct SSL3StatisticsStr)
> 
> I will give this r-, in light of the above issues, and since I already know 
> that you'll submit another patch. :-)
> 

The next version of the patch incorporates fixes to address your comments.  Please take another look.
Comment on attachment 290825 [details] [diff] [review]
Sub-patch 1: refactoring TLS extension related code

First, I want to express thanks for the work being done on the patches 
in this bug.  I'm going to suggest a number of changes for issues that are 
mostly cosmetic, affecting the readability of the code.  These comments are 
not in any order.  Forgive my verbosity.

1. in function ssl3_HandleServerHello
> #ifdef DISALLOW_SERVER_HELLO_EXTENSIONS
>     if (length != 0) {	/* malformed */
> 	goto alert_loser;
>     }
>+#else
>+    if (isTLS && length > 0) {
>+	SECItem extensions;
>+	rv = ssl3_ConsumeHandshakeVariable(ss, &extensions, 2, &b, &length);
>+	if (rv != SECSuccess || length != 0)
>+	    goto alert_loser;
>+         rv = ssl3_HandleHelloExtensions(ss, &extensions.data, &extensions.len);
>+         if (rv != SECSuccess) goto alert_loser;
>+    } else if (length > 0) {
>+	goto alert_loser;
>+    }
> #endif

The code inside #ifdef DISALLOW_SERVER_HELLO_EXTENSIONS is dead and may 
be deleted.  The code shown there was once not ifdef'ed and was wrong.
Instead of deleting it, I ifdef'ed it to remind other libSSL developers
what the effect of that code was, and not to make that mistake again.
We have found that there are some old SSL 3.0 implementations that do send 
stuff after the end of the server hello, and we have decided to 
deliberately ignore such stuff in the interest of maximal interoperability 
(being "generous in what you accept").  Let's not reverse that decision now.  
So, please do NOT add these two lines:
>+    } else if (length > 0) {
>+	goto alert_loser;

The r- is for that.

2. LOOOOONG symbol names.  I'd like to suggest that we adopt shorter
names for enumerated values and some functions.  We can use abbreviations for 
some things, and drop some redundant words.  

+/* Supported extensions. */
+typedef enum {
+    server_name_extension              = 0,
+#ifdef NSS_ENABLE_ECC
+    elliptic_curves_extension          = 10,
+    elliptic_point_formats_extension   = 11,
+#endif
+    session_ticket_extension           = 35
+} ExtensionType;

I'd suggest these alternatives:
  server_name
  supported_curves
  point_formats   (or supported_formats)
  session_ticket
Perhaps you might suffix them with _xtn if you think that helps.

Let's also shorten these LOOOOONG function names (which are probably 
names I coined, so mea culpa):

ssl3_HandleServerNameIndicationExtension
ssl3_HandleSupportedEllipticCurvesExtension
ssl3_HandleSupportedPointFormatsExtension

maybe 

ssl3_HandleServerNameXtn
ssl3_HandleSupportedCurvesXtn
ssl3_HandleSupportedPointsXtn

Oh, and please declare all those functions with "extern" in sslimpl.h

3. Excessive structure encapsulation.  background: libSSL was originally 
written back when systems had typically less than 16MB of RAM.  The SSL socket 
structure members were divided into many small structs, each of which was 
individually allocated from the heap, in the belief that this minimized memory 
usage (because structs were not allocated until needed).  But during the 
performance work for NSS 3.11, we found that huge amounts of time were spent 
allocating and freeing small structs.  So I did a project named "turning arrows 
into dots", which replaced many structure pointers with the actual structures,
causing the code to change appearance from ss->sec->something->else to
ss->sec.something.else.  This reduced the frequency of allocations, greatly 
reduced the number of pointer indirection fetches during execution, and solve a 
number of null pointer crashes.  But it left the code with these greatly 
encapsulated structures, which mostly serve no purpose, and which suggest 
meaning for structures that are really meaningless.  So, since then I have been 
introducing new variables directly into the sslSocket structure itself, rather 
than into subordinate structures.  Given time, I might slowly move things out 
of ss->sec or ss->ssl3 into the sslSocket structure.  I'd like to continue that 
trend, and not introduce new layers of encapsulation unnecesssarily, nor put 
new data items into lower levels of structures than necessary.  

At this point, I think the three reasons to keep additional levels of structure 
encapsulation within the sslsocket are:
a) cases where multiple copies of the variables exist, such as 
the two ssl3CipherSpec structs, current and pending.
b) cases where a union of structs that cannot simultaneously exist is used to 
reduce the total memory allocation,
c) when it is desirable to zero an entire group of variables with a 
single statement.

This brings me to these elements of this patch:
a) This change:

-    ssl3HelloExtensionSender *sender = &ss->serverExtensionSenders[0];
+    ssl3HelloExtensionSender *sender =
+        &ss->ssl3.extension_data.serverExtensionSenders[0];

This increases the amount of encapsulation for the array of 
secretExtensionSenders by two levels.  Is there a reason to move it into
the ssl3 struct, other than it seems related to ssl3?
The new extension_data struct seems to exist to be able to zero the set of 
related variables with a single statement, which is OK.  But I wonder: is 
there another existing structure that is already zeroed at approximately 
the same times, with approximately the same frequency as this new one?  
Could the members of extension_data be put into that other struct instead?  

If the lengthening of the name of the array (from serverExtensionSenders to ssl3.extension_data.serverExtensionSenders) is unavoidable, then let's shorten 
the name of serverExtensionSenders to compensate, e.g. maybe to svrXtnSenders.  

thanks again.
Attachment #290825 - Flags: review?(nelson) → review-
(In reply to comment #12)
> (From update of attachment 290825 [details] [diff] [review])
> First, I want to express thanks for the work being done on the patches 
> in this bug.  I'm going to suggest a number of changes for issues that are 
> mostly cosmetic, affecting the readability of the code.  These comments are 
> not in any order.  Forgive my verbosity.
> 
> 1. in function ssl3_HandleServerHello
> > #ifdef DISALLOW_SERVER_HELLO_EXTENSIONS
> >     if (length != 0) {	/* malformed */
> > 	goto alert_loser;
> >     }
> >+#else
> >+    if (isTLS && length > 0) {
> >+	SECItem extensions;
> >+	rv = ssl3_ConsumeHandshakeVariable(ss, &extensions, 2, &b, &length);
> >+	if (rv != SECSuccess || length != 0)
> >+	    goto alert_loser;
> >+         rv = ssl3_HandleHelloExtensions(ss, &extensions.data, &extensions.len);
> >+         if (rv != SECSuccess) goto alert_loser;
> >+    } else if (length > 0) {
> >+	goto alert_loser;
> >+    }
> > #endif
> 
> The code inside #ifdef DISALLOW_SERVER_HELLO_EXTENSIONS is dead and may 
> be deleted.  The code shown there was once not ifdef'ed and was wrong.
> Instead of deleting it, I ifdef'ed it to remind other libSSL developers
> what the effect of that code was, and not to make that mistake again.
> We have found that there are some old SSL 3.0 implementations that do send 
> stuff after the end of the server hello, and we have decided to 
> deliberately ignore such stuff in the interest of maximal interoperability 
> (being "generous in what you accept").  Let's not reverse that decision now.  
> So, please do NOT add these two lines:
> >+    } else if (length > 0) {
> >+	goto alert_loser;
> 
> The r- is for that.
> 

Done.

> 2. LOOOOONG symbol names.  I'd like to suggest that we adopt shorter
> names for enumerated values and some functions.  We can use abbreviations for 
> some things, and drop some redundant words.  
> 
> +/* Supported extensions. */
> +typedef enum {
> +    server_name_extension              = 0,
> +#ifdef NSS_ENABLE_ECC
> +    elliptic_curves_extension          = 10,
> +    elliptic_point_formats_extension   = 11,
> +#endif
> +    session_ticket_extension           = 35
> +} ExtensionType;
> 
> I'd suggest these alternatives:
>   server_name
>   supported_curves
>   point_formats   (or supported_formats)
>   session_ticket
> Perhaps you might suffix them with _xtn if you think that helps.

The reason I added _extension was because it is convenient to declare a variable called session_ticket. Replaced with _xtn.

> 
> Let's also shorten these LOOOOONG function names (which are probably 
> names I coined, so mea culpa):
> 
> ssl3_HandleServerNameIndicationExtension
> ssl3_HandleSupportedEllipticCurvesExtension
> ssl3_HandleSupportedPointFormatsExtension
> 
> maybe 
> 
> ssl3_HandleServerNameXtn
> ssl3_HandleSupportedCurvesXtn
> ssl3_HandleSupportedPointsXtn
> 
> Oh, and please declare all those functions with "extern" in sslimpl.h
> 

Done.

> 3. Excessive structure encapsulation.  background: libSSL was originally 
> written back when systems had typically less than 16MB of RAM.  The SSL socket 
> structure members were divided into many small structs, each of which was 
> individually allocated from the heap, in the belief that this minimized memory 
> usage (because structs were not allocated until needed).  But during the 
> performance work for NSS 3.11, we found that huge amounts of time were spent 
> allocating and freeing small structs.  So I did a project named "turning arrows 
> into dots", which replaced many structure pointers with the actual structures,
> causing the code to change appearance from ss->sec->something->else to
> ss->sec.something.else.  This reduced the frequency of allocations, greatly 
> reduced the number of pointer indirection fetches during execution, and solve a 
> number of null pointer crashes.  But it left the code with these greatly 
> encapsulated structures, which mostly serve no purpose, and which suggest 
> meaning for structures that are really meaningless.  So, since then I have been 
> introducing new variables directly into the sslSocket structure itself, rather 
> than into subordinate structures.  Given time, I might slowly move things out 
> of ss->sec or ss->ssl3 into the sslSocket structure.  I'd like to continue that 
> trend, and not introduce new layers of encapsulation unnecesssarily, nor put 
> new data items into lower levels of structures than necessary.  
> 
> At this point, I think the three reasons to keep additional levels of structure 
> encapsulation within the sslsocket are:
> a) cases where multiple copies of the variables exist, such as 
> the two ssl3CipherSpec structs, current and pending.
> b) cases where a union of structs that cannot simultaneously exist is used to 
> reduce the total memory allocation,
> c) when it is desirable to zero an entire group of variables with a 
> single statement.
> 

The reason for introducing additional structs is code readability.  Over the first iteration of the code, I had added variables directly to ssl3 or sslSession, but soon found that it was hard to quickly disambiguate between extension processing code and non-extension processing code.   I'm glad to follow whichever convention is appropriate for this codebase.

> This brings me to these elements of this patch:
> a) This change:
> 
> -    ssl3HelloExtensionSender *sender = &ss->serverExtensionSenders[0];
> +    ssl3HelloExtensionSender *sender =
> +        &ss->ssl3.extension_data.serverExtensionSenders[0];
> 
> This increases the amount of encapsulation for the array of 
> secretExtensionSenders by two levels.  Is there a reason to move it into
> the ssl3 struct, other than it seems related to ssl3?
> The new extension_data struct seems to exist to be able to zero the set of 
> related variables with a single statement, which is OK.  But I wonder: is 
> there another existing structure that is already zeroed at approximately 
> the same times, with approximately the same frequency as this new one?  
> Could the members of extension_data be put into that other struct instead?  
> 

I moved serverExtensionSenders to extension_data so that related data could be in the same place.  Otherwise, while writing code, I found that I had to keep in mind the  exceptions to where related data is located.  extension_data.ticket_timestamp_verified (and session ticket related vars.) don't seem like they would fit well under ssl3, though I don't mind flattening the struct in order to reduce indirection (though I suspect that more extension data that will show up when other extensions get implemented).

> If the lengthening of the name of the array (from serverExtensionSenders to
> ssl3.extension_data.serverExtensionSenders) is unavoidable, then let's shorten 
> the name of serverExtensionSenders to compensate, e.g. maybe to svrXtnSenders.  
> 

I could go either way; please let me know which you prefer.  Thanks for the quick turn-around.  I'm looking forward to having session ticket support in FF!

> thanks again.
>
Attached patch Proposed patch v2 (obsolete) — Splinter Review
This is a new patch from Nagendra, addressing Nelson's review comments.

I created the NSS_RFC4507BIS_BRANCH for SecurityServices and checked in
this patch there.  Nelson, feel free to make your suggested changes
directly there.

To check out this branch:

cvs -q co -r NSS_RFC4507BIS_BRANCH SecurityServices

To create a patch for code review:

cvs -q diff -pNU8 -r NSS_RFC4507BIS_BASE mozilla/security/nss

(NSS_RFC4507BIS_BASE is the snapshot before I checked in Nagendra's
code on the branch.)
Attachment #288414 - Attachment is obsolete: true
Attachment #294312 - Flags: review?(nelson)
Comment on attachment 294312 [details] [diff] [review]
Proposed patch v2

> Nelson, feel free to make your suggested changes directly there.

Wan-Teh, 
I do not plan to take on responsibility for further development of this patch.

Your latest patch appears to address all my previous review comments 
except two, which are:

a) the issue for which I gave r- in comment 12, where I wrote:

> So, please do NOT add these two lines:
> >+    } else if (length > 0) {
> >+	goto alert_loser;
> The r- is for that.

In comment 13, Nagendra wrote "Done.", but those lines remain in the patch.
r- for that.

b) The issue I previously described as "Excessive structure encapsulation."

Are you aware of any other previous review comments that are not addressed 
in this latest patch?
Attachment #294312 - Flags: review?(nelson) → review-
A structure was created that has only one instance, which is named
extension_data.  All the members of that structure have the word 
Extension in their names.  So all references to those members have
both strings "extension" and "Extension" in their names, e.g. 
extension_data.advertisedClientExtensions.  Then the arrays are 
indexed by a variable that also has the string "Extensions" in its name.
So ignoring case, references to a particular member of that array have 
the word "extension" in them at least THREE TIMES.  That's by no means
a readability improvement.  This leads to blocks of code like this one:

	    TLS1ExtensionData *ex_data = &ss->ssl3.extension_data;
	    ex_data->advertisedClientExtensions[
		    ex_data->numAdvertisedClientExtensions++] =
		server_name_xtn;

Even using ex_data as a shorthand for ss->ssl3.extension_data, that
statement takes up 3 lines because of all the redundancy.  
That's unreadable.

I suggest all these changes:

1) Remove the word Extension or Extensions from the names of all members 
of the TLS1ExtensionData struct.  The fact that they relate to extensions
is part of the struct name, and is redundant in member names.
2) Change "dvertisedClient" to "dvertised" everywhere. (Only clients
advertise, so it's redundant.)
3) Rename extension_data to xtnData, and move it out of ssl3 into ss.
4) Be consistent.  No underscores in the names of members of struct TLS1ExtensionData.  Use mixed case names instead. 

With those changes, the above block of code becomes:

	    TLS1ExtensionData *xtnData = &ss->xtnData;
	    xtnData->advertised[xtnData->numAdvertised++] =
		server_name_xtn;

which IMO is MUCH more readable.
(In reply to comment #16)
> A structure was created that has only one instance, which is named
> extension_data.  All the members of that structure have the word 
> Extension in their names.  So all references to those members have
> both strings "extension" and "Extension" in their names, e.g. 
> extension_data.advertisedClientExtensions.  Then the arrays are 
> indexed by a variable that also has the string "Extensions" in its name.
> So ignoring case, references to a particular member of that array have 
> the word "extension" in them at least THREE TIMES.  That's by no means
> a readability improvement.  This leads to blocks of code like this one:
> 
>             TLS1ExtensionData *ex_data = &ss->ssl3.extension_data;
>             ex_data->advertisedClientExtensions[
>                     ex_data->numAdvertisedClientExtensions++] =
>                 server_name_xtn;
> 
> Even using ex_data as a shorthand for ss->ssl3.extension_data, that
> statement takes up 3 lines because of all the redundancy.  
> That's unreadable.
> 
> I suggest all these changes:
> 
> 1) Remove the word Extension or Extensions from the names of all members 
> of the TLS1ExtensionData struct.  The fact that they relate to extensions
> is part of the struct name, and is redundant in member names.
> 2) Change "dvertisedClient" to "dvertised" everywhere. (Only clients
> advertise, so it's redundant.)
> 3) Rename extension_data to xtnData, and move it out of ssl3 into ss.
> 4) Be consistent.  No underscores in the names of members of struct
> TLS1ExtensionData.  Use mixed case names instead. 
> 
> With those changes, the above block of code becomes:
> 
>             TLS1ExtensionData *xtnData = &ss->xtnData;
>             xtnData->advertised[xtnData->numAdvertised++] =
>                 server_name_xtn;
> 
> which IMO is MUCH more readable.
> 

I moved TLS1ExtensionData to ss, and shortened member names as suggested.  Regarding comment #15(a) -- apologies, I had misread your comment.  The next version of the patch, which I'll upload shortly incorporates appropriate fixes.
Attached patch Proposed patch v3 (obsolete) — Splinter Review
Comment on attachment 296066 [details] [diff] [review]
Proposed patch v3

Previously, in comment 8, I wrote that there were some files I had not
yet reviewed.  I believe I have reviewed them all now in the patch v3.
Here are my remaining comments.

1. Does this patch compile?  
ssl3ecc.c still has the old struct member names.

2. in ssl3_AppendNumberToItem, change lines of this form:
	*p++ = (num >> NN) & 0xff;
to this form
	*p++ = (uint8)(num >> NN);

3. in ssl3_GenerateSessionTicketKeysPKCS11
>     slot = PK11_GetBestSlot(CKM_SHA256_HMAC, NULL);
>     /* no parameter, 256-bit key size */
>     session_ticket_mac_key_pkcs11 =
> 	PK11_KeyGen(slot, CKM_GENERIC_SECRET_KEY_GEN, NULL, 32, NULL);
>     PK11_FreeSlot(slot);

PK11_GetBestSlot assures that slot can do CKM_SHA256_HMAC but does not
assure that slot can do CKM_GENERIC_SECRET_KEY_GEN.  So, use 
PK11_GetBestSlotMultiple instead, and pass both mechanism types to it.

4. This is just a question.

In ssl3_SendNewSessionTicket, the ticket structure contents are 
"appended" (packed) into a secitem the same way that the unaligned 
members of a SSL record or message are appended.  
In ssl3_ServerHandleSessionTicketExt, after decrypting the received
ticket, it is "consumed" (parsed) in the same way that SSL records 
and messages, with their packed, unaligned members are parsed.  

Why?

For variable length items, such as a client cert, I can understand why.
But most of the fields in the SessionTicket struct are fixed length.
So, why not just "append" and "consume" the entire struct as a single
item, and then separately append/consume the variable length items 
after that?  Why do all the fields separately?  

5. Tomorrow I will look in more depth at the method by which you 
handle wrapped master secrets.  The thing that is not yet apparent to me
is how multiple servers (in a farm, say) are going to share the key that 
is used to wrap the master secrets.  If they don't share that key, then 
their tickets will not be interchangeable.  Maybe the answer is 
there, and I just don't see it yet.  But if not, then I think some 
design changes may be in order.
Attachment #296066 - Flags: review-
(In reply to comment #19)
> (From update of attachment 296066 [details] [diff] [review])
> Previously, in comment 8, I wrote that there were some files I had not
> yet reviewed.  I believe I have reviewed them all now in the patch v3.
> Here are my remaining comments.
> 
> 1. Does this patch compile?  
> ssl3ecc.c still has the old struct member names.

Fixed.

> 
> 2. in ssl3_AppendNumberToItem, change lines of this form:
>         *p++ = (num >> NN) & 0xff;
> to this form
>         *p++ = (uint8)(num >> NN);
> 

Changed.  (Note: the first form is also used in ssl3_AppendHandshakeNumber.)

> 3. in ssl3_GenerateSessionTicketKeysPKCS11
> >     slot = PK11_GetBestSlot(CKM_SHA256_HMAC, NULL);
> >     /* no parameter, 256-bit key size */
> >     session_ticket_mac_key_pkcs11 =
> > 	PK11_KeyGen(slot, CKM_GENERIC_SECRET_KEY_GEN, NULL, 32, NULL);
> >     PK11_FreeSlot(slot);
> 
> PK11_GetBestSlot assures that slot can do CKM_SHA256_HMAC but does not
> assure that slot can do CKM_GENERIC_SECRET_KEY_GEN.  So, use 
> PK11_GetBestSlotMultiple instead, and pass both mechanism types to it.

Done.

> 
> 4. This is just a question.
> 
> In ssl3_SendNewSessionTicket, the ticket structure contents are 
> "appended" (packed) into a secitem the same way that the unaligned 
> members of a SSL record or message are appended.  
> In ssl3_ServerHandleSessionTicketExt, after decrypting the received
> ticket, it is "consumed" (parsed) in the same way that SSL records 
> and messages, with their packed, unaligned members are parsed.  
> 
> Why?
> 
> For variable length items, such as a client cert, I can understand why.
> But most of the fields in the SessionTicket struct are fixed length.
> So, why not just "append" and "consume" the entire struct as a single
> item, and then separately append/consume the variable length items 
> after that?  Why do all the fields separately?  
> 

We don't have a struct defined that encapsulates all the values that constitute a session ticket.  The other alternative would be to allocate a buffer and serialize into/from it, before calling append/consume, so overall the doing each field individually didn't seem too bad.  I hope that answered your question.

> 5. Tomorrow I will look in more depth at the method by which you 
> handle wrapped master secrets.  The thing that is not yet apparent to me
> is how multiple servers (in a farm, say) are going to share the key that 
> is used to wrap the master secrets.  If they don't share that key, then 
> their tickets will not be interchangeable.  Maybe the answer is 
> there, and I just don't see it yet.  But if not, then I think some 
> design changes may be in order.
> 

We do not implement sharing of session keys between servers.

Next version of the patch on its way shortly.
Attached patch Proposed patch v4 (obsolete) — Splinter Review
Attachment #296066 - Attachment is obsolete: true
> We don't have a struct defined that encapsulates all the values that 
> constitute a session ticket.

I thought the SessionTicket struct was exactly that, a struct that 
encapsulates all the values that constitute a session ticket. 
What other purpose does it serve?

> We do not implement sharing of session keys between servers.

Not even among multi-process servers that share a common session cache?
If not, I think that's a show-stopper.  

I thought that one of the major motivations for session tickets was 
server scalability - the ability of multiple separate ("loosely coupled" :)
servers to share load.  What is the motivation for an implementation that doesn't accomplish that?  I think NSS must offer the ability to accomplish
that.  


Right, we didn't implement sharing of session keys among multi-process servers.
Our primary interest is in getting this feature into Firefox 3.  We have implemented
enough on the server side to to be able to test the client-side implementation.
The latest OpenSSL 0.9.8 releases already support session tickets, so having
a browser support session tickets will be a major milestone for this feature.
We can document that the server-side support of session tickets  in NSS has
serious limitations and should only be used for testing in single-process mode now.
(In reply to comment #19)
>
> PK11_GetBestSlot assures that slot can do CKM_SHA256_HMAC but does not
> assure that slot can do CKM_GENERIC_SECRET_KEY_GEN.  So, use 
> PK11_GetBestSlotMultiple instead, and pass both mechanism types to it.

CKM_GENERIC_SECRET_KEY_GEN is the key generation mechanism for HMAC-SHA-256,
so it is closely related to CKM_SHA256_HMAC.  Should we also use
PK11_GetBestSlotMultiple for CKM_AES_CBC and CKM_AES_KEY_GEN?
Comment on attachment 296240 [details] [diff] [review]
Proposed patch v4

Up until now, I have reviewed the code at a low level, without 
looking much at the design.  This patch v4 addresses all my 
previous code review comments, and I thank you for that.

But now I have two more review issues that are issues with the 
design.  I think these are relatively minor, but IMO, they must
be corrected. 

1) No key version information in ticket. 

Imagine this scenario: a browser user visits the server and gets
a ticket, then goes to a meeting that lasts a few hours, leaving
his browser running.  While he is in the meeting, the server is
restarted, causing it to get a new ticket encryption key and a new
ticket MAC key.  The user returns from his meeting and sends in 
another https transaction to the server.  His request bears a 
ticket that was encrypted with the old encryption key and MACed
with the old MAC key.  The server will detect this, but only 
after it has done all the work to decrypt the ticket (with the
new/wrong key) and to check the MAC (which will fail).  

There are also other scenarios in which a ticket can be received 
from a client that is encrypted with the wrong key, but I won't
detail them here. 

It seems desirable for the server to be able to detect that the 
ticket was produced with an old/wrong key, and hence to avoid 
all the work to decrypt and MAC-check the ticket.  To that end,
IIRC, the RFC suggests that a "key name" be sent along with the
ticket, unencrypted (or, equivalently, in the unencrypted part
of the ticket).  This "name" identifies the key in such a way
that if a different key is being used by the server than the one
with which the ticket was encrypted, the server can detect that
without/before decrypting the ticket.

This patch uses a constant, invariant string as the key name.

>+#define TLS1_EX_SESS_TICKET_KEY_NAME        "NSS_SESS_TICKET!"

That defeats the purpose.  The key name should be a random value
generated at the same time as the pair of keys used to mac and
encrypt the ticket.  It should be checked when received prior to 
doing all the decryption and MACing, IMO.  

The patch instead uses it as part of the MAC'ed data.  It adds 
no value to put this into the MAC'ed data, because it doesn't 
improve the efficiency, nor does it improve the probability of 
a ticket encrypted with the wrong key being detectected.  

So, issue #1 is: fix the key name.

2) For two server processes (in the same box or different boxes)
to be able to share load, they must have access to the same 
values of all these items:
 - the ticket encryption key
 - the ticket MAC key
 - the key name corresponding to the above two keys
 - the wrapping key used to wrap the master secrets (if used).

There must exist some means by which the server processes in a 
logical server session space (e.g. server farm) can all agree on 
the values of the above items.  It could be done by one process 
generating these values and then securely transmitting them to 
the other processes, or by some key-agreement protocol (e.g. 
such as some variant of Diffie-Hellman).   

In the case of multiple processes in the same box, NSS provides
a means by which those processes can share that information, 
using shared memory.  

When processes cannot share memory, obviously some other protocol 
must be used.  Developing that other protocol is outside of the 
scope of this enhancement.  But assuming that such a protocol is
developed elsewhere (say, in the application, or elsewhere in NSS
shared libraries), there needs to be a way for that other code 
 - to obtain the values listed above from the NSS library for the 
   purpose of sending them, and 
 - to set the values into the library when received from another
   process sharing the same logical server session space.

IOW, there must exist getters and setters for these values.

This patch does not provide those things, and consequently 
there is no way for an NSS-based server to effectively share
the server session space across multiple processes.  

>+static PK11SymKey    *session_ticket_enc_key_pkcs11 = NULL;
>+static PK11SymKey    *session_ticket_mac_key_pkcs11 = NULL;

That needs to be fixed.  

It might be instructive to understand how NSS presently accomplishes
this sharing of session key wrapping keys among processes that can 
share memory.  I once wrote a summary of how that works.  I'll see
if I can find it to add to this bug.

When these two remaining issues are resolved, I think this patch will
be ready for checkin.
Attachment #296240 - Flags: review-
In comment 25, I wrote:
> The key name should be a random value generated at the same time as the 
> pair of keys used to mac and encrypt the ticket. 

Alternatively, it could be a date/time stamp, such as a PRTime value,
generated when the pair of keys is generated.
(In reply to comment #25)
> (From update of attachment 296240 [details] [diff] [review])
> Up until now, I have reviewed the code at a low level, without 
> looking much at the design.  This patch v4 addresses all my 
> previous code review comments, and I thank you for that.
> 
> But now I have two more review issues that are issues with the 
> design.  I think these are relatively minor, but IMO, they must
> be corrected. 
> 
> 1) No key version information in ticket. 
> 
> Imagine this scenario: a browser user visits the server and gets
> a ticket, then goes to a meeting that lasts a few hours, leaving
> his browser running.  While he is in the meeting, the server is
> restarted, causing it to get a new ticket encryption key and a new
> ticket MAC key.  The user returns from his meeting and sends in 
> another https transaction to the server.  His request bears a 
> ticket that was encrypted with the old encryption key and MACed
> with the old MAC key.  The server will detect this, but only 
> after it has done all the work to decrypt the ticket (with the
> new/wrong key) and to check the MAC (which will fail).  
> 
> There are also other scenarios in which a ticket can be received 
> from a client that is encrypted with the wrong key, but I won't
> detail them here. 
> 
> It seems desirable for the server to be able to detect that the 
> ticket was produced with an old/wrong key, and hence to avoid 
> all the work to decrypt and MAC-check the ticket.  To that end,
> IIRC, the RFC suggests that a "key name" be sent along with the
> ticket, unencrypted (or, equivalently, in the unencrypted part
> of the ticket).  This "name" identifies the key in such a way
> that if a different key is being used by the server than the one
> with which the ticket was encrypted, the server can detect that
> without/before decrypting the ticket.
> 
> This patch uses a constant, invariant string as the key name.
> 
> >+#define TLS1_EX_SESS_TICKET_KEY_NAME        "NSS_SESS_TICKET!"
> 
> That defeats the purpose.  The key name should be a random value
> generated at the same time as the pair of keys used to mac and
> encrypt the ticket.  It should be checked when received prior to 
> doing all the decryption and MACing, IMO.  
> 

I found that using a fixed name made debugging easy (when using ethereal, which does not know how to parse the session ticket extension).  I'll change the name to be part fixed and part variable, thus achieving both goals. 

> The patch instead uses it as part of the MAC'ed data.  It adds 
> no value to put this into the MAC'ed data, because it doesn't 
> improve the efficiency, nor does it improve the probability of 
> a ticket encrypted with the wrong key being detectected.  
> 
> So, issue #1 is: fix the key name.
> 
> 2) For two server processes (in the same box or different boxes)
> to be able to share load, they must have access to the same 
> values of all these items:
>  - the ticket encryption key
>  - the ticket MAC key
>  - the key name corresponding to the above two keys
>  - the wrapping key used to wrap the master secrets (if used).
> 
> There must exist some means by which the server processes in a 
> logical server session space (e.g. server farm) can all agree on 
> the values of the above items.  It could be done by one process 
> generating these values and then securely transmitting them to 
> the other processes, or by some key-agreement protocol (e.g. 
> such as some variant of Diffie-Hellman).   
> 
> In the case of multiple processes in the same box, NSS provides
> a means by which those processes can share that information, 
> using shared memory.  
> 
> When processes cannot share memory, obviously some other protocol 
> must be used.  Developing that other protocol is outside of the 
> scope of this enhancement.  But assuming that such a protocol is
> developed elsewhere (say, in the application, or elsewhere in NSS
> shared libraries), there needs to be a way for that other code 
>  - to obtain the values listed above from the NSS library for the 
>    purpose of sending them, and 
>  - to set the values into the library when received from another
>    process sharing the same logical server session space.
> 
> IOW, there must exist getters and setters for these values.
> 
> This patch does not provide those things, and consequently 
> there is no way for an NSS-based server to effectively share
> the server session space across multiple processes.  
> 
> >+static PK11SymKey    *session_ticket_enc_key_pkcs11 = NULL;
> >+static PK11SymKey    *session_ticket_mac_key_pkcs11 = NULL;
> 
> That needs to be fixed.

Will upload a new patch once I have this addressed.
  
> 
> It might be instructive to understand how NSS presently accomplishes
> this sharing of session key wrapping keys among processes that can 
> share memory.  I once wrote a summary of how that works.  I'll see
> if I can find it to add to this bug.
> 
> When these two remaining issues are resolved, I think this patch will
> be ready for checkin.
> 

Sorry about the delay.  I have almost completed the implementation that supports sharing of session ticket keys between multiple processes (using the shared memory segment used by the session cache).  Keys are currently generated when a session ticket is first issued, however it will be fairly straightforward to add support for setting keys (rather than having them generated).  If keys are to be set, then  they must be set prior to any connections being received, as keys are cached per process.
Attached patch Proposed patch v5 (obsolete) — Splinter Review
Attachment #296240 - Attachment is obsolete: true
I uploaded v5 of the patch.  This version includes support for:
* automatic generation of session ticket encryption and mac keys when either the first ticket is sent or received.  These keys are shared, if multiple server processes are involved, via the shared memory segment used for session data.
* the implementation allows for: session ticket keys to be preset by populating the cache before any process accepts connections  (the API for setting keys has not been implemented, but is straightforward).
* session tickets are now constructed as follows: "NSS!" + 12 random bytes
* tickets are discarded prior to MAC verification if the presented name mis-matches the name for the current sent of keys

I'm hoping that this patch can be reviewed for FF Beta-4.
Blocks: 415033
Blocks: 415196
We shortened SSL_ENABLE_SESSION_TICKET_EXTENSION to
SSL_ENABLE_SESSION_TICKETS.  This fixes most of the code
formatting difficulties.  I also made some whitespace
and reformatting changes to make the patch smaller and
more readable.

Otherwise this is essentially Nagendra's patch v5.
Attachment #300599 - Attachment is obsolete: true
Attachment #301082 - Flags: review?(nelson)
Comment on attachment 301082 [details] [diff] [review]
Proposed patch v5.1 (by Nagendra Modadugu)

The following change in ssl3con.c changes ifdef to ifndef.
I'm not sure if that's correct.

>-#ifdef DISALLOW_SERVER_HELLO_EXTENSIONS
>-    if (length != 0) {	/* malformed */
>-	goto alert_loser;
>+#ifndef DISALLOW_SERVER_HELLO_EXTENSIONS
>+    if (isTLS && length > 0) {
>+	SECItem extensions;
>+	rv = ssl3_ConsumeHandshakeVariable(ss, &extensions, 2, &b, &length);
>+	if (rv != SECSuccess || length != 0)
>+	    goto alert_loser;
>+	rv = ssl3_HandleHelloExtensions(ss, &extensions.data, &extensions.len);
>+	if (rv != SECSuccess) goto alert_loser;
>     }
> #endif
(In reply to comment #32)
> (From update of attachment 301082 [details] [diff] [review])
> The following change in ssl3con.c changes ifdef to ifndef.
> I'm not sure if that's correct.
> 
> >-#ifdef DISALLOW_SERVER_HELLO_EXTENSIONS
> >+#ifndef DISALLOW_SERVER_HELLO_EXTENSIONS

Yes, it's correct.  See comment 12 above for more explanation.
Can we delete the #ifndef DISALLOW_SERVER_HELLO_EXTENSIONS test?
The DISALLOW_SERVER_HELLO_EXTENSIONS macro is only used there.

Since the original code would be deleted by this patch, should we
add a comment to "remind other libSSL developers what the effect
of that code was, and not to make that mistake again"?
In reply to comment 34, surely.  Go for it.
Attached patch Proposed patch v5.2 (obsolete) — Splinter Review
Updates in this version:

* added a counter for ticket parse failures
* strsclnt keeps track of failed connect() calls, in order to avoid failing  tests due to the Winsock bug described in bug 334961
* a server will fail a connection if session tickets are enabled, and the 
  server is unable to retrieve session ticket keys
I made some minor changes to Nagendra's proposed patch v5.2.1.
Nelson, please review this patch.
Attachment #294312 - Attachment is obsolete: true
Attachment #301082 - Attachment is obsolete: true
Attachment #302255 - Attachment is obsolete: true
Attachment #303181 - Flags: review?(nelson)
Attachment #301082 - Flags: review?(nelson)
These diffs are intended to help code reviewers see what changes were made
to the code moved from ssl3ecc.c to ssl3ext.c.
Attachment #290825 - Attachment is obsolete: true
Attachment #290884 - Attachment is obsolete: true
Comment on attachment 303181 [details] [diff] [review]
Proposed patch v5.2.1 (by Nagendra Modadugu)

Wan-Teh, I spent some time reviewing the differences between patch v4
(which I previously reviewed) and your patch 5.2.1, using this URL:
<https://bugzilla.mozilla.org/attachment.cgi?oldid=296240&action=interdiff&newid=303181&headers=1>

Due to the warning that appears at the top of that page, I suspect, but am 
not sure, that the diffs between the patches shown there are incomplete.  

Can you tell me if the differences shown on that page are the only differences 
from the previous patch?  If not, please help me identify any new changes
missing from that page, so that I can review just those parts.
Attached patch normalized patch v5.2.1 (obsolete) — Splinter Review
Attachment #304535 - Attachment is obsolete: true
Attachment #304537 - Attachment is patch: true
Attachment #304537 - Attachment mime type: application/octet-stream → text/plain
Nelson, patch 5.2.1 has a new file sslsnce.c (for sharing the keys
between processes in a multi-process server), and indeed that URL is
missing the changes to sslsnce.c.

It seems that the sslsnce.c changes are the only changes missing in
that URL.  But to be safe, I will create a new patch v4 with a dummy
change to to sslsnce.c so that interdiffs will work.  Until then you
should be able to just look at the sslsnce.c changes in patch 5.2.1.
You can also look at the interdiffs that Kai just attached, which
has the warning but seems complete, with the sslsnce.c changes.

Here are the size increases in ssl3.dll/libssl3.so/libssl3.dylib
you asked for:
Win32: exactly 4K (4096) bytes
Linux and Mac OS X: ~9.5K bytes
I hope you don't mind that I quickly jump in to assist you with getting this reviewed.

I found that neither v4 nor v5.2.1 apply to the latest trunk.
I found that both apply to the nss 3.12 alpha2 snapshot (only the latter has a minor one-line context conflict.)

Then I produced two patches that were made with the same diff options.
Then I found that the latter patch changes an additional file.
So, I split the patch into two portions. First portion with same list of files, and the changes to the additional file as a second portion.


Nelson, you can do your interdiff between "normalized patch v4" and "normalized patch v5.2.1 without sslsnce.c" and review that.
Then you must look at attachment "sslsnce.c subset of v5.2.1" in addition.

I hope this helps.
Attachment #303181 - Attachment is obsolete: true
Attachment #304549 - Flags: review?(nelson)
Attachment #303181 - Flags: review?(nelson)
Kai, thanks for jumping in.  

To my surprise, when I viewed the interdiff between your 
"Proposed patch v4, with dummy change to sslsnce.c" and the
"Proposed patch v5.2.1 (by Nagendra Modadugu), attached again"
the warnings were gone (as expected) but the changes to sslsnce.c were 
completely hidden, nowhere to be seen.  This really lowers my confidence
in bugzilla's interdiff tool, which is very unfortunate for reviewers.

But I can simply review the interdiff between your two "normalized" patches, 
and also separately review the patch to sslsnce.c.  Thanks for making those.

Nelson, my new patch v4 with dummy change sslsnce.c still can't
make patch interdiffs work correctly, as you noted.  I gave up.
I hope you can do the code review using Kai's "normalized" patches.
Comment on attachment 304549 [details] [diff] [review]
Proposed patch v5.2.1 (by Nagendra Modadugu), attached again

First I reviewed all the parts of this patch EXCEPT the new file ssl3ext.c
and the patch to sslsnce.c.  I found those parts to be basically all OK 
except for some rather minor nits.  

Then I reviewed the new file ssl3ext.c and the patch to sslsnce.c, and I 
found a few minor issues and one issue that I think is major and probably will result in r-.  So the purpose of this bug comment is to begin discussion of 
that major issue.  It's an issue that might not have come up a few months ago, but now is a hot topic.  It is: 
  Late (or "lazy") initialization of server cache contents.

You may know about Julien's campaign to remove all late/lazy initialization 
from libSSL.  We have some disagreements about doing that for the client side 
of libSSL, but we agree that there is no need for late/lazy initialization in any server code, since there exist server cache initialization functions that must be called "early".  

Formerly libSSL had a lot of things that were initialized in a lazy fashion.  But now we're trying to eliminate the lazy initialization of anything having 
to do with servers, especially server cache contents/data/keys.  One of our objectives is to eliminate the use of PR_CallOnce from code paths that are (potentially) executed one or more times per socket.  I *think* that lazy
initialization of server cache values (especially locks and keys that protect it) has now been eliminated, and so a patch that reintroduces such lazy initialization will now be seen as a problem.  

This patch initializes the keys used for the session tickets in a late/lazy fashion.  The new functions ssl_GetSessionTicketKeysPKCS11 and ssl_GetSessionTicketKeys do lazy initialization, and the two functions are inconsistent with their use of locks to protect and synchronize the value of *(cache->ticketKeysValid) and related key values.  Function ssl3_GetSessionTicketKeysPKCS11 calls PR_CallOnceWithArg, and function.  
ssl3_GetSessionTicketKeys calls PR_CallOnce.  It appears that at least one 
of those functions may get called per server socket (when session tickets 
are in use), from functions ssl3_ServerHandleSessionTicketExt or   
ssl3_SendNewSessionTicket.

I think this is a problem. I expect Julien will agree.  I'm not giving r- yet
but I want to start the discussion on this issue.  If you disagree that lazy initialization of server cache locks and keys has been eliminated (except for this new patch), please feel free to say so.
Re: comment 50,

Indeed, we should always try to initialize all the globals, including locks and keys, as early as possible for the server case. I think we can greatly simplify the libssl code by creating an ssl_Init function. This will probably be ssl_InitLocks renamed and modified to include the new initialization calls needed for the session ticket. My only concern is about the use of PR_CallOnceWithArg here. Why is the argument needed , and where does it come from? Does it mean it is not possible to do all the initialization early on ?
I'm not entertaining the idea of inventing a new SSL library initialization function at this time.  All existing servers that use libSSL must perform one
or more initialization calls now.  I merely propose to additionally burden 
some of those existing functions with the task of initializing whatever is 
needed for the server cache keys.  I think we could initialize those keys 
unconditionally, whether we're going to use TLSSTE or not.  

This idea is complicated by the fact that at least one of the libSSL cache 
initialization functions is expected to be called BEFORE NSS is initialized,
at least in multi-process servers.  That function cannot make calls that 
use keys, because it has no access to PKCS11 modules yet.  But cache initialization of a multi-process server is a multi-step task.  If any of
those steps is performed after NSS is initialized, that step can be used to
initialize the SSL server cache keys.  

I'm not certain, at the moment, whether the single-process server cache 
initialization function can be called before NSS_Init, or not.  I suspect
that it can be. That could be a problem...
Nelson, I wasn't proposing to invent a new public function - just merely group all the internal initialization that happens today under the hood in one place, to make changes such as this one easier in the future. 

The initialization of the single-process SSL server session cache can and usually does happen before NSS_Init, for example in selfserv. So we won't be able to create the ticket keys early, except for the bypass case :( It sounds like that generation will need to remain separate from the early initialization of locks. The use of PR_CallOnce may thus be appropriate.

Note that the multi-process case doesn't create additional problems with initialization. The function that creates the MP session cache today does not perform any early initialization of locks/keys. Only the processes that inherit the cache do that.
(In reply to comment #51)
> Re: comment 50,
> 
> Indeed, we should always try to initialize all the globals, including locks and
> keys, as early as possible for the server case. I think we can greatly simplify
> the libssl code by creating an ssl_Init function. This will probably be
> ssl_InitLocks renamed and modified to include the new initialization calls
> needed for the session ticket. My only concern is about the use of
> PR_CallOnceWithArg here. Why is the argument needed , and where does it come
> from? Does it mean it is not possible to do all the initialization early on ?
> 

When in non-bypass mode (i.e. PKCS11), the session ticket keys are stored wrapped in the shared memory in order to accommodate FIPS mode servers.  This code path for generating (or reading) session ticket keys needs a handle to the server's private key, which is the argument passed to PR_CallOnceWithArg.

I will comment on lazy vs. early initialization in a separate message.
(In reply to comment #53)
> Nelson, I wasn't proposing to invent a new public function - just merely group
> all the internal initialization that happens today under the hood in one place,
> to make changes such as this one easier in the future. 
> 
> The initialization of the single-process SSL server session cache can and
> usually does happen before NSS_Init, for example in selfserv. So we won't be
> able to create the ticket keys early, except for the bypass case :( It sounds
> like that generation will need to remain separate from the early initialization
> of locks. The use of PR_CallOnce may thus be appropriate.
> 

Yes, this is the problem (circular dependency on initialization) that I ran into.  In order to generate or unwrap keys that have been introduced to the cache: (1) PKCS11 needs to have been initialized, and (2) the server's private key needs to be available for wrapping/unwrapping of session ticket keys.

Nelson, there is one more location (which I think Julien calls  "initialization of locks/keys") where lazy initialization exists: ssl_SetWrappingKey.
If you pass this option to SSL_OptionSet, it'll fail with
SEC_ERROR_INVALID_ARGS.
Attachment #304819 - Flags: review?(nelson)
Comment on attachment 304819 [details] [diff] [review]
Reserve an SSL socket option for TLS session tickets

r=nelson
There's precedent for doing this (reserving the option number in advance of implementing the option).  There was another option for which we did this once.  Don't recall which one.
Attachment #304819 - Flags: review?(nelson) → review+
Comment on attachment 304549 [details] [diff] [review]
Proposed patch v5.2.1 (by Nagendra Modadugu), attached again

Nagendra,

I reviewed portions of this patch last weekend.  I've made some changes on the
NSS_RFC4507BIS_BRANCH.  Here are the things that I don't know how to fix.

1. We have two abbreviations for extension: xtn and Ext.  Ideally we should
standardize on one.

2. There are two occurrences of &key_name[4].  What is the magic value 4?

3. The code that does numAdvertised++ and numNegotiated++ never increments
the index beyond the end of array, but it takes a lot of work to verify
that.  We may want to have explicit array bound checks to help code reviewers
eliminate this doubt.

4. Question for both you and Nelson: in ssl3_HandleServerHello, we have:

-#ifdef DISALLOW_SERVER_HELLO_EXTENSIONS
-    if (length != 0) {	/* malformed */
-	goto alert_loser;
+    /* Note that if !isTLS && length != 0, we do NOT goto alert_loser.
+     * There are some old SSL 3.0 implementations that do send stuff
+     * after the end of the server hello, and we deliberately ignore
+     * such stuff in the interest of maximal interoperability (being
+     * "generous in what you accept").
+     */
+    if (isTLS && length != 0) {
+	SECItem extensions;
+	rv = ssl3_ConsumeHandshakeVariable(ss, &extensions, 2, &b, &length);
+	if (rv != SECSuccess || length != 0)
+	    goto alert_loser;               <===== HERE
+	rv = ssl3_HandleHelloExtensions(ss, &extensions.data, &extensions.len);
+	if (rv != SECSuccess)
+	    goto alert_loser;
     }
-#endif

Note that if there is extra stuff after the extensions, we still go to
alert_loser.  This is stricter than the original code.  Is this OK?
(In reply to comment #58)
> (From update of attachment 304549 [details] [diff] [review])
> Nagendra,
> 
> I reviewed portions of this patch last weekend.  I've made some changes on the
> NSS_RFC4507BIS_BRANCH.  Here are the things that I don't know how to fix.
> 
> 1. We have two abbreviations for extension: xtn and Ext.  Ideally we should
> standardize on one.

I'll leave it to Nelson to comment on this.  I'll make the changes once a decision has been made. 

> 
> 2. There are two occurrences of &key_name[4].  What is the magic value 4?
> 

'4' is the length of the string "NSS!", which prefixes every session ticket.
I have changed this to a #define.

> 3. The code that does numAdvertised++ and numNegotiated++ never increments
> the index beyond the end of array, but it takes a lot of work to verify
> that.  We may want to have explicit array bound checks to help code reviewers
> eliminate this doubt.
> 

I have updated the code with bounds checks wherever there one of the variables is incremented.

> 4. Question for both you and Nelson: in ssl3_HandleServerHello, we have:
> 
> -#ifdef DISALLOW_SERVER_HELLO_EXTENSIONS
> -    if (length != 0) { /* malformed */
> -       goto alert_loser;
> +    /* Note that if !isTLS && length != 0, we do NOT goto alert_loser.
> +     * There are some old SSL 3.0 implementations that do send stuff
> +     * after the end of the server hello, and we deliberately ignore
> +     * such stuff in the interest of maximal interoperability (being
> +     * "generous in what you accept").
> +     */
> +    if (isTLS && length != 0) {
> +       SECItem extensions;
> +       rv = ssl3_ConsumeHandshakeVariable(ss, &extensions, 2, &b, &length);
> +       if (rv != SECSuccess || length != 0)
> +           goto alert_loser;               <===== HERE
> +       rv = ssl3_HandleHelloExtensions(ss, &extensions.data, &extensions.len);
> +       if (rv != SECSuccess)
> +           goto alert_loser;
>      }
> -#endif
> 
> Note that if there is extra stuff after the extensions, we still go to
> alert_loser.  This is stricter than the original code.  Is this OK?
> 

I'd prefer to be strict so as to avoid the need for possible work-arounds in the future.  But that said, I don't know which, or how many, servers behave this way already.
Comment on attachment 304549 [details] [diff] [review]
Proposed patch v5.2.1 (by Nagendra Modadugu), attached again

Nelson, I reviewed the basic TLS extension handling portions of this patch.
I'd like to call your attention to the following changes.  These change
what we allow/ignore and what we disallow.

1. In ssl3_HandleClientHello, the original code handles extensions in SSL3.
The new code ignores extensions in SSL3.  Search for the string
"OpenSSL 0.9.8g sends TLS extensions even when negotiating SSL3" to review
this change.

2. In the original code, clients ignore all extensions sent by the server.
In the new code, clients call ssl3_HandleHelloExtensions in TLS to handle
the extensions.  In addition, clients reject any unadvertised extension.
Search for "#ifdef DISALLOW_SERVER_HELLO_EXTENSIONS" and
ssl3_HandleHelloExtensions to review this change.

Do you think clients should verify that the ec_point_formats extension in
ServerHello contains the uncompressed point format?

Do you think clients should verify that the server_name extension in
ServerHello has an empty "extension_data" field?

3. In the original code, servers allow the same extension to appear more
than once in ClientHello.  In the new code, servers (and clients) disallow
the same extension to appear more than once.
Comment on attachment 304549 [details] [diff] [review]
Proposed patch v5.2.1 (by Nagendra Modadugu), attached again

I'm definitely suffering from reviewer's fatigue.  I've looked through variants
of this monster patch too many times and can no longer see it with "fresh eyes"
or review it with the enthusiasm I had the first time.

We previously agreed that we'd go ahead and commit this (or something close to it), 
and then work on the lock and cache key value initialization issues.  Let's 
proceed with that plan.  

Here are some review comments on this patch.  All but one may be deferred, and 
fixed after the big checkin.  The one that I want to see fixed in the comment
about the new code that handles address in use errors in strsclnt.  I definitely
don't want that change being mixed in with the rest of the STE changes.  
It's not an STE change, and it really needs to be considered separately and 
committed separately.  

I'm going to reply to this bug's comments 58-60 separately in a comment to follow.

strsclnt
1. addr_in_use_errors on Windows.  I have issues with this.  
I've raised them before, by email.  It should be a separate patch with a separate 
bug #, separately committed.  If we want to change it or back it out, we need
to be able to identify it separately from the monster STE patch.

2. This expression makes my head hurt.  Some comments would help.

+	exitVal = (enableSessionTickets &&
+                (ssl3stats->hsh_sid_stateless_resumes !=
+                connections - addr_in_use_errors - 1)) ||
+                (!enableSessionTickets &&
+                ((ssl3stats->hsh_sid_cache_misses > 1) ||
+                (ssl3stats->hsh_sid_stateless_resumes != 0))) ||
                 (ssl3stats->hsh_sid_cache_not_ok != 0) ||
                 (certsTested > 1);

Please turn that into an if-then-else-if... block that computes different
values of exitVal, e.g. something like this.

if (enableSessionTickets) 
    exitVal = (ssl3stats->hsh_sid_stateless_resumes !=
                (connections - addr_in_use_errors - 1));
else 
    exitVal = ((ssl3stats->hsh_sid_cache_misses > 1) ||
              (ssl3stats->hsh_sid_stateless_resumes != 0));
if (!exitVal) 
    exitVal = (ssl3stats->hsh_sid_cache_not_ok != 0) || (certsTested > 1);

tstclnt
3. Isn't the -r feature going to cause much repeated renegotiation on the 
same socket?  

4. Are there unexpected interactions between selfserv -r and tstclnt -r ?

5. You add new errors to nss/lib/ssl/sslerr.h, but not to the corresponding
nss/cmd/lib/SSLerrs.h.  Gotta fix that.

6. New function declarations in sslimpl.h don't follow indentation style
found elsewhere in that file.  The continuation needs more indentation.

extern SECStatus ssl3_HandleServerNameExt(sslSocket * ss,
    PRUint16 ex_type, SECItem *data);
Attachment #304549 - Flags: review?(nelson) → review+
(In reply to comment 58 and comment 60)
> 1. We have two abbreviations for extension: xtn and Ext.  Ideally we should
> standardize on one.

Let's pick one.  If either string is already exposed in public header files
then let's use that one.  Otherwise, I'd prefer xtn.  

> 4. Question for both you and Nelson: in ssl3_HandleServerHello, 
> Note that if there is extra stuff after the extensions, we still go to
> alert_loser.  This is stricter than the original code.  Is this OK?

With this patch, if the client has attempted to negotiate TLS, it processes
extensions according to the TLS rules, which are different from the SSL3
rules.  

Under SSL3, there was no formal definition of any extensions, and it was 
assumed that extensions would be defined in the future, so the spec advised 
implementers to not reject client hellos if extra stuff was present.  The
spec didn't prohibit handling known extensions (once they became defined).
It merely advised not to automatically reject client hellos if extensions
were present.

The TLS rules are different.  They define a new extensible scheme to which 
all newly defined and future extensions must conform.  It defines how 
extensions are packed in the hello, how the end of the extensions is found,
etc.  It explicitly prohibits any extra data after the TLS extension area.

So I think it is right to disallow extra trailing stuff IFF the client is 
negotiating TLS (SSL 3.1+), and NEVER right to disallow extra trailing stuff
if the client is negotiating SSL 3.0.

> 1. In ssl3_HandleClientHello, the original code handles extensions in SSL3.
> The new code ignores extensions in SSL3. 

(Big qualifier here: The following comments are based solely on faulty 
recollection.)

This is potentially a problem.  IIRC, the code in NSS 3.11.x does not prevent any SSL3 or TLS cipher suite from being negotiated with SSL 3.0.  So, even though there are many cipher suties that have been defined after SSL 3.0 was defined, NSS is happy to negotiate them, even if the client is negotiating SSL 3.0.  

It would be wrong to ignore the client hello extensions but then permit the
negotiation of a cipher suite that depends on them (such as an ECC cipher
suite).  So, when the client advertises SSL 3.0 but also includes TLS-style
hello extensions, the code must either
a) process the hello extensions as it would for TLS
b) ignore the hello extensions and also not allow cipher suites that depend 
on those extensions to be negotiated. 

If (as I recall) NSS is already doing choice a today, then I'm quite insistent that NSS must continue to do that.  If my recollection is mistaken, and NSS is not allowing ECC to be negotiated with SSL 3.0, then I think choice B will be OK.  I do NOT want to try to introduce logic into libSSL at this time that attempts to control the set of cipher suitse negotiated based on the negotiated version number.  We will have to do that eventually, because some new cipher suites that are now defined but not yet implemented in NSS require that they only be implemented in TLS 1.2 or later. But the code to do that should be carefully designed when the time comes.

> 2. In the original code, clients ignore all extensions sent by the server.
> In the new code, clients call ssl3_HandleHelloExtensions in TLS to handle
> the extensions.  

It's OK to handle server hello extensions when they are present.  We should
not REQUIRE them to be present.  But we can require them to be correct when
they are present.

> In addition, clients reject any unadvertised extension.

I'm fine with that.

> Do you think clients should verify that the ec_point_formats extension in
> ServerHello contains the uncompressed point format?

If that extension is present, yes, that would be ideal.  But it's not a big 
deal because if the client and server disagree about the port formats, the handshake will fail.  I would not expect a vulnerability to arise from that.
We should not require that this extension be present in the server hello.

> Do you think clients should verify that the server_name extension in
> ServerHello has an empty "extension_data" field?

If that extension is present, yes.  That would be nice.  But not a biggie. 
We should not require that this extension be present in the server hello.

> 3. In the original code, servers allow the same extension to appear more
> than once in ClientHello.  In the new code, servers (and clients) disallow
> the same extension to appear more than once.

There is some slight risk that this will introduce an incompatibility with 
some existing servers that send duplicated extensions, servers with which 
NSS 3.11.x works OK now.  But IIRC, the TLS extension spec disallows 
dupliate extensions, and I'll bet that at least some other browsers or 
clients already disallow it.  So, I think this would only "break" operation
with servers that previously worked ONLY with FF, and I doubt there are many
of those.

(In reply to comment #62)

Hi Nelson, thanks for being patient with the patch.  I'll discuss with Wan-Teh and submit an update to the patch within the next few days.  Some comments below: 

> (From update of attachment 304549 [details] [diff] [review])
> I'm definitely suffering from reviewer's fatigue.  I've looked through variants
> of this monster patch too many times and can no longer see it with "fresh eyes"
> or review it with the enthusiasm I had the first time.
> 
> We previously agreed that we'd go ahead and commit this (or something close to
> it), 
> and then work on the lock and cache key value initialization issues.  Let's 
> proceed with that plan.  
> 
> Here are some review comments on this patch.  All but one may be deferred, and 
> fixed after the big checkin.  The one that I want to see fixed in the comment
> about the new code that handles address in use errors in strsclnt.  I
> definitely
> don't want that change being mixed in with the rest of the STE changes.  
> It's not an STE change, and it really needs to be considered separately and 
> committed separately.  
> 
> I'm going to reply to this bug's comments 58-60 separately in a comment to
> follow.
> 
> strsclnt
> 1. addr_in_use_errors on Windows.  I have issues with this.  
> I've raised them before, by email.  It should be a separate patch with a
> separate 
> bug #, separately committed.  If we want to change it or back it out, we need
> to be able to identify it separately from the monster STE patch.
> 
> 2. This expression makes my head hurt.  Some comments would help.
> 
> +       exitVal = (enableSessionTickets &&
> +                (ssl3stats->hsh_sid_stateless_resumes !=
> +                connections - addr_in_use_errors - 1)) ||
> +                (!enableSessionTickets &&
> +                ((ssl3stats->hsh_sid_cache_misses > 1) ||
> +                (ssl3stats->hsh_sid_stateless_resumes != 0))) ||
>                  (ssl3stats->hsh_sid_cache_not_ok != 0) ||
>                  (certsTested > 1);
> 
> Please turn that into an if-then-else-if... block that computes different
> values of exitVal, e.g. something like this.
> 
> if (enableSessionTickets) 
>     exitVal = (ssl3stats->hsh_sid_stateless_resumes !=
>                 (connections - addr_in_use_errors - 1));
> else 
>     exitVal = ((ssl3stats->hsh_sid_cache_misses > 1) ||
>               (ssl3stats->hsh_sid_stateless_resumes != 0));
> if (!exitVal) 
>     exitVal = (ssl3stats->hsh_sid_cache_not_ok != 0) || (certsTested > 1);
> 

I'll discuss with Wan-Teh about how to issue this portion of the patch -- I agree that the fix for addr_in_use_errors should not be part of STE.

> tstclnt
> 3. Isn't the -r feature going to cause much repeated renegotiation on the 
> same socket?  
> 

The '-r' flag takes a count of the number of renegotiations as a parameter.  Yes the renegotiations occur over the same socket. 

> 4. Are there unexpected interactions between selfserv -r and tstclnt -r ?
> 

These are the four cases of interaction between tstclnt -r and selfserv -r:

* 1 '-r' flag to selfserv: request client cert on first handshake -- does not affect renegotiation or resumption on renegotiation
* 2 '-r' flags to selfserv: require client cert on first handshake -- does not affect renegotiation or resumption on renegotiation
* 3 -'r' flags to selfserv: request client cert on first and second handshake -- does not affect resumption on renegotiation (which is what tstclnt -r tries to do)
* 4 '-r' flags to selfserv: require client cert on second handshake -- session resumption is not allowed on the second handshake

The purpose of the '-r' option to tstclnt is to be able to do a quick check that session resumption is happening as expected.  It was much simpler to get tstclnt to renegotiate than drop a connection and open a new one (also this would change the meaning of tstclnt, which is expected to make a single TCP connection to a specified server).

> 5. You add new errors to nss/lib/ssl/sslerr.h, but not to the corresponding
> nss/cmd/lib/SSLerrs.h.  Gotta fix that.
> 

Done.

> 6. New function declarations in sslimpl.h don't follow indentation style
> found elsewhere in that file.  The continuation needs more indentation.
> 
> extern SECStatus ssl3_HandleServerNameExt(sslSocket * ss,
>     PRUint16 ex_type, SECItem *data);
> 

Done.
(In reply to comment #63)
> (In reply to comment 58 and comment 60)
> > 1. We have two abbreviations for extension: xtn and Ext.  Ideally we should
> > standardize on one.
> 
> Let's pick one.  If either string is already exposed in public header files
> then let's use that one.  Otherwise, I'd prefer xtn.  
> 

Switched to Xtn.  Neither ssl.h nor sslproto.h included references to names including "Ext".

> > 4. Question for both you and Nelson: in ssl3_HandleServerHello, 
> > Note that if there is extra stuff after the extensions, we still go to
> > alert_loser.  This is stricter than the original code.  Is this OK?
> 
> With this patch, if the client has attempted to negotiate TLS, it processes
> extensions according to the TLS rules, which are different from the SSL3
> rules.  
> 
> Under SSL3, there was no formal definition of any extensions, and it was 
> assumed that extensions would be defined in the future, so the spec advised 
> implementers to not reject client hellos if extra stuff was present.  The
> spec didn't prohibit handling known extensions (once they became defined).
> It merely advised not to automatically reject client hellos if extensions
> were present.
> 
> The TLS rules are different.  They define a new extensible scheme to which 
> all newly defined and future extensions must conform.  It defines how 
> extensions are packed in the hello, how the end of the extensions is found,
> etc.  It explicitly prohibits any extra data after the TLS extension area.
> 
> So I think it is right to disallow extra trailing stuff IFF the client is 
> negotiating TLS (SSL 3.1+), and NEVER right to disallow extra trailing stuff
> if the client is negotiating SSL 3.0.
> 
> > 1. In ssl3_HandleClientHello, the original code handles extensions in SSL3.
> > The new code ignores extensions in SSL3. 
> 
> (Big qualifier here: The following comments are based solely on faulty 
> recollection.)
> 
> This is potentially a problem.  IIRC, the code in NSS 3.11.x does not prevent
> any SSL3 or TLS cipher suite from being negotiated with SSL 3.0.  So, even
> though there are many cipher suties that have been defined after SSL 3.0 was
> defined, NSS is happy to negotiate them, even if the client is negotiating SSL
> 3.0.  
> 
> It would be wrong to ignore the client hello extensions but then permit the
> negotiation of a cipher suite that depends on them (such as an ECC cipher
> suite).  So, when the client advertises SSL 3.0 but also includes TLS-style
> hello extensions, the code must either
> a) process the hello extensions as it would for TLS
> b) ignore the hello extensions and also not allow cipher suites that depend 
> on those extensions to be negotiated. 
> 
> If (as I recall) NSS is already doing choice a today, then I'm quite insistent
> that NSS must continue to do that.  If my recollection is mistaken, and NSS is
> not allowing ECC to be negotiated with SSL 3.0, then I think choice B will be
> OK.  I do NOT want to try to introduce logic into libSSL at this time that
> attempts to control the set of cipher suitse negotiated based on the negotiated
> version number.  We will have to do that eventually, because some new cipher
> suites that are now defined but not yet implemented in NSS require that they
> only be implemented in TLS 1.2 or later. But the code to do that should be
> carefully designed when the time comes.
> 

I reverted the patch to do (a).  In summary:  servers will accept TLS extensions when negotiating SSL3, while clients only parse extensions if the negotiated protocol is TLS (clients previously ignored extensions sent by servers altogether).

> > 2. In the original code, clients ignore all extensions sent by the server.
> > In the new code, clients call ssl3_HandleHelloExtensions in TLS to handle
> > the extensions.  
> 
> It's OK to handle server hello extensions when they are present.  We should
> not REQUIRE them to be present.  But we can require them to be correct when
> they are present.
> 

Agreed.  This is the implemented behavior.

> > In addition, clients reject any unadvertised extension.
> 
> I'm fine with that.
> 
> > Do you think clients should verify that the ec_point_formats extension in
> > ServerHello contains the uncompressed point format?
> 
> If that extension is present, yes, that would be ideal.  But it's not a big 
> deal because if the client and server disagree about the port formats, the
> handshake will fail.  I would not expect a vulnerability to arise from that.
> We should not require that this extension be present in the server hello.
> 
> > Do you think clients should verify that the server_name extension in
> > ServerHello has an empty "extension_data" field?
> 
> If that extension is present, yes.  That would be nice.  But not a biggie. 
> We should not require that this extension be present in the server hello.

I left an additional TODO for servers to respond with an empty ServerNameExtension in the ServerHello (servers currently ignore an incoming ServerNameExtension, so the response can be implemented when once servers support the extension).

> 
> > 3. In the original code, servers allow the same extension to appear more
> > than once in ClientHello.  In the new code, servers (and clients) disallow
> > the same extension to appear more than once.
> 
> There is some slight risk that this will introduce an incompatibility with 
> some existing servers that send duplicated extensions, servers with which 
> NSS 3.11.x works OK now.  But IIRC, the TLS extension spec disallows 
> dupliate extensions, and I'll bet that at least some other browsers or 
> clients already disallow it.  So, I think this would only "break" operation
> with servers that previously worked ONLY with FF, and I doubt there are many
> of those.
> 

I tagged the NSS trunk before my landing as NSS_HEAD_BEFORE_RFC4507BIS.
I checked in the patch on the NSS trunk for NSS 3.12.

Checking in cmd/lib/SSLerrs.h;
/cvsroot/mozilla/security/nss/cmd/lib/SSLerrs.h,v  <--  SSLerrs.h
new revision: 1.7; previous revision: 1.6
done
Checking in cmd/selfserv/selfserv.c;
/cvsroot/mozilla/security/nss/cmd/selfserv/selfserv.c,v  <--  selfserv.c
new revision: 1.84; previous revision: 1.83
done
Checking in cmd/strsclnt/strsclnt.c;
/cvsroot/mozilla/security/nss/cmd/strsclnt/strsclnt.c,v  <--  strsclnt.c
new revision: 1.61; previous revision: 1.60
done
Checking in cmd/tstclnt/tstclnt.c;
/cvsroot/mozilla/security/nss/cmd/tstclnt/tstclnt.c,v  <--  tstclnt.c
new revision: 1.50; previous revision: 1.49
done
Checking in lib/ssl/manifest.mn;
/cvsroot/mozilla/security/nss/lib/ssl/manifest.mn,v  <--  manifest.mn
new revision: 1.17; previous revision: 1.16
done
Checking in lib/ssl/ssl.h;
/cvsroot/mozilla/security/nss/lib/ssl/ssl.h,v  <--  ssl.h
new revision: 1.28; previous revision: 1.27
done
Checking in lib/ssl/ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.110; previous revision: 1.109
done
Checking in lib/ssl/ssl3ecc.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3ecc.c,v  <--  ssl3ecc.c
new revision: 1.21; previous revision: 1.20
done
Checking in lib/ssl/ssl3ext.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3ext.c,v  <--  ssl3ext.c
new revision: 1.2; previous revision: 1.1
done
Checking in lib/ssl/ssl3prot.h;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3prot.h,v  <--  ssl3prot.h
new revision: 1.13; previous revision: 1.12
done
Checking in lib/ssl/sslerr.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslerr.h,v  <--  sslerr.h
new revision: 1.6; previous revision: 1.5
done
Checking in lib/ssl/sslimpl.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslimpl.h,v  <--  sslimpl.h
new revision: 1.65; previous revision: 1.64
done
Checking in lib/ssl/sslnonce.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslnonce.c,v  <--  sslnonce.c
new revision: 1.24; previous revision: 1.23
done
Checking in lib/ssl/sslsnce.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslsnce.c,v  <--  sslsnce.c
new revision: 1.44; previous revision: 1.43
done
Checking in lib/ssl/sslsock.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslsock.c,v  <--  sslsock.c
new revision: 1.55; previous revision: 1.54
done
Checking in lib/ssl/sslt.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslt.h,v  <--  sslt.h
new revision: 1.11; previous revision: 1.10
done
Checking in tests/ssl/sslstress.txt;
/cvsroot/mozilla/security/nss/tests/ssl/sslstress.txt,v  <--  sslstress.txt
new revision: 1.14; previous revision: 1.13
done
Assignee: wtc → ngm+mozilla
For code review only.  This patch omits the new code that won't be
executed by NSS customers disabling TLS session tickets (which is
the default).

This patch does not include the new file ssl3ext.c, which will be
in the next patch (part 2 of the series).

Nelson, I requested review just to help you find this patch.  You
don't need to review it.
Attachment #307792 - Flags: review?(nelson)
The new file ssl3ext.c is based on TLS extension code moved from
ssl3ecc.c.  This hand-constructed patch shows the diffs between
the original code in ssl3ecc.c and the new file ssl3ext.c.
Attachment #307794 - Flags: review?(nelson)
Except for build or all.sh test failures resulting from my checkin,
please do not reopen this bug.  Let's open new bugs to address the
remaining issues or other fallouts of this patch.  You can make
the new bugs depend on this bug.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Comment on attachment 307792 [details] [diff] [review]
Effective patch for NSS customers disabling TLSSTE, part 1, changes to existing files


>Index: mozilla/security/nss/lib/ssl/ssl3con.c

You're still planning on fixing this after the checkin, right?

>-#ifdef DISALLOW_SERVER_HELLO_EXTENSIONS
>-    if (length != 0) {	/* malformed */
>-	goto alert_loser;
>+    /* Note that if !isTLS && length != 0, we do NOT goto alert_loser.
>+     * There are some old SSL 3.0 implementations that do send stuff
>+     * after the end of the server hello, and we deliberately ignore
>+     * such stuff in the interest of maximal interoperability (being
>+     * "generous in what you accept").
>+     */
>+    if (isTLS && length != 0) {
>+	SECItem extensions;
>+	rv = ssl3_ConsumeHandshakeVariable(ss, &extensions, 2, &b, &length);
>+	if (rv != SECSuccess || length != 0)
>+	    goto alert_loser;
>+	rv = ssl3_HandleHelloExtensions(ss, &extensions.data, &extensions.len);
>+	if (rv != SECSuccess)
>+	    goto alert_loser;
>     }
>-#endif
Attachment #307792 - Attachment description: Effective patch for NSS customers disabling TLSSTE, part 1 → Effective patch for NSS customers disabling TLSSTE, part 1, changes to existing files
Wan-Teh, you've marked this bug resolved/fixed. 
Are you planning to file a separate bug about the remaining issues?
D'oh!  I just read comment 69. :-/
Attachment #307792 - Flags: review?(nelson)
Attachment #307794 - Flags: review?(nelson)
It occurs to me that, since we change "ext" to "xtn" in all the code, 
we should have done that in the name of the new file, also.  ssl3ext.c. :(
(In reply to comment #70)
I don't know what needs to be fixed in that code.
We do exactly what you wanted -- SSL3 clients ignore
extensions in ServerHello; TLS clients handle extensions
in ServerHello, and reject extra stuff after the extensions.
Blocks: 421378
Blocks: 421383
Blocks: 421389
Blocks: 421391
Maybe I'm confused.  I'll look at it again.
My concern is entirely about how servers process client hellos.
That's where we have the biggest potential for broken compatibility, IMO.
So, if the code I cited is in client code, then I need to take a fresh 
look at it.  
(In reply to comment #73)
Nelson, I can rename the new file ssl3xtn.c or even tlsxtn.c.
Just let me know.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: