540304 - Implement SSL_HandshakeNegotiatedExtension

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Kai Engert [:KaiE:]]

Implement SSL_PeerSupportsSafeRenegotiation

PSM would like to implement visual chrome that tells a user whether the peer server supports (or doesn't support) safe renegotiation.

Please see attached small patch, which I implemented on top of code from bug 537356.

Comment 1

[Kai Engert [:KaiE:]]

Attachment: Patch v1

Comment 2

[Kai Engert [:KaiE:]]

Nelson convinced me to name this function:
  SSL_PeerSentRenegotiationExtension

I'll rename it and attach a new patch.

Comment 3

[Kai Engert [:KaiE:]]

Attachment: Patch v2

Comment 4

[Kai Engert [:KaiE:]]

FYI, why did I initially propose to name this function SSL_PeerSupportsSafeRenegotiation ?
Because of this definition in ssl.h:

#define SSL_REQUIRE_SAFE_NEGOTIATION   21 /* Peer must use renegotiation    */
                                          /* extension in ALL handshakes.   */
                                          /* default: off                   */

My conclusion was:
"If peer used the renego ext in the handshake, this peer on this socket uses safe negotiation."

I had assumed that
  ssl3_ExtensionNegotiated(sslsocket, renegotiation_info_xtn) == PR_TRUE
indicates such a safe socket.

I don't care whether the peer will actually ever perform or not perform a renego. My only worry is that the following statement is true: "Should the peer ever allow or perform a renego on this socket, it will ensure it's a safe renego."

Based on the comment for SSL_REQUIRE_SAFE_NEGOTIATION I concluded it would be fine to use a similar name for a function that gives status information about a socket.

But I'm fine to use the more direct function name SSL_PeerSentRenegotiationExtension, which doesn't imply whether it's safe or not safe. This decision / conclusion is up to the application code that uses it.

I assume that
  ssl3_ExtensionNegotiated(sslsocket, renegotiation_info_xtn)
will return PR_TRUE,
regardless whether the real extension or the SCSV was used.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 422112 [details] [diff] [review]
Patch v2

Numerous thoughts here.

1. This approach has serious security vulnerability implications for 
the user.  If the client connects to a vulnerable server and allows 
the handshake to proceed to completion, even when the server does not 
send back the Renegotiation Info response, then even if the client 
does nothing further beyond that, that act alone may trigger the 
vulnerability and may be damaging to the user.  

In other words, it is NOT SAFE for a browser user to connect and 
handshake with a server, and then ask "did this handshake get an RI
back from the server"?  

If we're really trying to protect the user from this vulnerability,
the *only* safe thing to do is to set the option that forces the 
handshake to ABORT immediately if the server's hello does not contain
the RI.  Then PSM can detect that, and present that to the user, and
tell the user "you're trying to talk to a vulnerable server, do you 
really want to do this?", and let the user choose to go ahead.  If the
user says "Yes, I'll take the risk", then PSM can try again without
setting the flag that causes NSS to abort the handshake in the absence 
of an RI in the server hello.

So, my general advice is: DON'T DO IT THIS WAY.

The rest of the comments below are relevant only if you decide not to
take that advice.  My r- actually applies to the comments below.

2. This function purports to return a SECStatus, but doesn't.
SSL_ERROR_RENEGOTIATION_NOT_ALLOWED is not an allowable value for a 
SECStatus enumeration.  

3. I suggest you generalize this function.  Instead of making it 
specific for this particular extension, have it take the extension 
value as an input, and answer yes or no.  In other words, make it 
be an externally callable form of ssl3_ExtensionNegotiated.

3. There are some other tests that need to be done in this function
before calling ssl3_ExtensionNegotiated, in order to be sure that 
the socket is an SSL3 socket, etc.  Some structures in an SSL socket
are unions that have different meanings for SSL2 than SSL3/TLS.  
You may also need to acquire one or more locks.  Not sure.

Comment 6

[Kai Engert [:KaiE:]]

Nelson, please, this bug is simply about a flag that returns information.

Re 1:   Yes, I know!

I'll send you some more comments regarding (1) in a personal message.
But I believe this discussion should not happen in this bug.


Thanks for your other review comments, I'll work on them.

Comment 7

[Kai Engert [:KaiE:]]

I'm fine to generalize, but the parameter ExtensionType to function ssl3_ExtensionNegotiated is defined in function ssl3prot.h, which is not an exported header file.

I could add an enum in the public generalized header function and have the implementation map the enum values to the ExtensionType values defined in ssl3prot.h

Comment 8

[Kai Engert [:KaiE:]]

Attachment: Patch v3

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Just to clarify...
- I don't object to have a public SSL_ExtensionWasNegotiated function, and
- I don't object to moving the definitions of the extension name symbols 
into a public SSL header file. 

- I DO object to claiming to provide client users with protection from the 
renegotiation vulnerability while promiscuously negotiating and using this 
means to determine if it caused damage, after the fact.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Kai, I suggest you move these declarations from ssl3prot.h, which is private
to sslproto.h, which is public.

346 /* Supported extensions. */
347 /* Update MAX_EXTENSIONS whenever a new extension type is added. */
348 typedef enum {
349     server_name_xtn              = 0,
350 #ifdef NSS_ENABLE_ECC
351     elliptic_curves_xtn          = 10,
352     ec_point_formats_xtn         = 11,
353 #endif
354     session_ticket_xtn           = 35
355 } ExtensionType;
356 
357 #define MAX_EXTENSIONS             4

and just use them rather than inventing a new enumerated type that must 
somehow be kept in sync.  

Bob, Wan-Teh,  Does that seem agreeable to you?

Comment 11

[Robert Relyea]

Hmm, My I must not have hit submit for my comment.

The purpose of the UI indicator is to shame sites into updating. (Why doesn't firefox show that my site is secure? Answer: because it isn't until you upgrade your server).

Obviously when we turn on that indicator will depend on how fast sites upgrade their servers. It doesn't do any good if the indicator is on for 90% of the websites.

Re moving the declarations to sslproto.h.... sounds good to me.

bob

Comment 12

[Kai Engert [:KaiE:]]

Are you sure you want to move the type as is?

It seems that, usually in NSS, all externally visible types have some kind of prefix, while the existing internal "ExtensionType" and its member values like "server_name_xtn" don't have any common prefix.

I guess the task to move the types would also require some renaming?

I'm fine with moving the type as is, if you are fine with it.

Comment 13

[Wan-Teh Chang]

Comment on attachment 422242 [details] [diff] [review]
Patch v3

This function should return PRBool instead of SECSuccess.

The ssl_ext_id_size constant is not useful.  We don't
need it.  The other ssl_ext_id_ constants should have
their original values to avoid confusion.  So you keep
the switch statement in the function to allow just those
three extension types as the input.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

> Are you sure you want to move the type as is?

Oh, good point.  Yeah, it does need a prefix.  :-/

Comment 15

[Kai Engert [:KaiE:]]

I see file sslproto.h contains #define statements, only.
Would you rather want me to add the type to file sslt.h, which is also public, and which already has several structs and enums?

Ok, I will add prefixes.

I see
/* Update MAX_EXTENSIONS whenever a new extension type is added. */

so I think we should move this #define, too, and a prefix, too.

And I'll rename it everywhere (good bye idea of having a small patch) :-)

Comment 16

[Kai Engert [:KaiE:]]

(In reply to comment #13)
> This function should return PRBool instead of SECSuccess.

Right now the function is able to return 3 states:
- yes
- no
- don't know, for various reasons.

Are you sure you want to return "no" if the answer is unknown? (for example, because there hasn't been a handshake yet).


> The ssl_ext_id_size constant is not useful.  We don't
> need it.  The other ssl_ext_id_ constants should have
> their original values to avoid confusion.  So you keep
> the switch statement in the function to allow just those
> three extension types as the input.

I'll avoid the new type and move the existing type.

Comment 17

[Kai Engert [:KaiE:]]

Sigh, now I'm in merging hell, because the renaming touches the same code as Nelson's not-yet-commited patch from bug 537356.

Comment 18

[Kai Engert [:KaiE:]]

Attachment: Patch v4

new patch that avoids the new type, moves the existing type

moving to sslt.h, not to sslproto.h
but if you reconfirm you prefer sslproto.h, I'll do that

not having changed function signature to return bool,
hoping that Wan-Teh may change this mind, but I'll change it if Wan-Teh reconfirms his request

not yet renaming, in order to avoid the merging hell, while the work on bug 537356 and this bug happens in parallel.

I propose to perform a rename task (and I'm willing to do it) once bug 537356 gets checked in, and I propose to rename as this:

Existing:
typedef enum {
    server_name_xtn              = 0,
#ifdef NSS_ENABLE_ECC
    elliptic_curves_xtn          = 10,
    ec_point_formats_xtn         = 11,
#endif
    session_ticket_xtn           = 35,
    renegotiation_info_xtn       = 0xff01	/* experimental number */
} ExtensionType;

#define MAX_EXTENSIONS             5


Change to:
typedef enum {
    ssl_server_name_xtn              = 0,
#ifdef NSS_ENABLE_ECC
    ssl_elliptic_curves_xtn          = 10,
    ssl_ec_point_formats_xtn         = 11,
#endif
    ssl_session_ticket_xtn           = 35,
    ssl_renegotiation_info_xtn       = 0xff01	/* experimental number */
} SSLExtensionType;

#define SSL_MAX_EXTENSIONS             5

Comment 19

[Kai Engert [:KaiE:]]

Comment on attachment 422454 [details] [diff] [review]
Patch v4

Asking Bob as an alternative reviewer, in case Nelson doesn't have the time.

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 422454 [details] [diff] [review]
Patch v4

Kai,
I think your patch is conceptually right. 
You definitely should do the big rename that you suggested,
AFTER the renegotiation patch is also committed.
I'd request that you let the renegotiation patch go in first,
then this patch, (which will have a merge problem).  
If Alexei's server SNI patch has not yet gone it, I'd let it 
go in next after that.  THEN do the global search and replace 
of those symbols.  I'd suggest you do that as a separate patch.  
that makes no other changes than the renaming, so that reviewing
it can be purely mechanical.

Comment 21

[Kai Engert [:KaiE:]]

Attachment: Patch v6

updated patch after Nelson's recent checkin, fixes the merge issue

Comment 22

[Kai Engert [:KaiE:]]

Attachment: action script for renaming

I'll do this for renaming.
This illustrates a series of commands and their output.
Don't worry too much about it.

It shows only files in security/nss/lib/ssl are affected by the rename.

It shows all matching lines before and after the rename.

Comment 23

[Kai Engert [:KaiE:]]

Attachment: alternative patch v7

I know, Nelson, you propose to do things in steps. Fine with me.

I actually propose you review patch v6, then glance at the "action script" to see what will be changed.

I'm willing to wait for a patch from Alexei to go in, if you prefer that or expect merging issues. However, I think that Alexei could simply run the same find/replace commands in his tree (if he uses Unix).

Anyway, I'm attach a patch v7 that combines my patch v6 and already includes the renaming.

Please r+ either v6 or v7. If you r+ v6, I'll do the rename later.
Thanks

Comment 24

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 423981 [details] [diff] [review]
alternative patch v7

The symbol renaming made 3 lines too long.  Please wrap them, then commit.
r=nelson

>@@ -4017,7 +4017,7 @@
>     if (ss->ssl3.hs.sendingSCSV) {
> 	/* Since we sent the SCSV, pretend we sent empty RI extension. */
> 	TLSExtensionData *xtnData = &ss->xtnData;
>-	xtnData->advertised[xtnData->numAdvertised++] = renegotiation_info_xtn;
>+	xtnData->advertised[xtnData->numAdvertised++] = ssl_renegotiation_info_xtn;
..12345678901234567890123456789012345678901234567890123456789012345678901234567890


>@@ -328,14 +328,14 @@
>             if (rv != SECSuccess) return -1;
>             if (!ss->sec.isServer) {
>                 TLSExtensionData *xtnData = &ss->xtnData;
>-                xtnData->advertised[xtnData->numAdvertised++] = server_name_xtn;
>+                xtnData->advertised[xtnData->numAdvertised++] = ssl_server_name_xtn;
..12345678901234567890123456789012345678901234567890123456789012345678901234567890

>@@ -503,7 +503,7 @@
> 
> 	if (!ss->sec.isServer) {
> 	    TLSExtensionData *xtnData = &ss->xtnData;
>-	    xtnData->advertised[xtnData->numAdvertised++] = session_ticket_xtn;
>+	    xtnData->advertised[xtnData->numAdvertised++] = ssl_session_ticket_xtn;
..12345678901234567890123456789012345678901234567890123456789012345678901234567890

Comment 25

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 423976 [details] [diff] [review]
Patch v6

I gave r+ to v7, so I'll cancel the review request for v6.

Comment 26

[Kai Engert [:KaiE:]]

Attachment: Patch 7 wrapped

Thanks for r+ ! I've checked in this patch, which is v7 + wrapping.

Checking in ssl/ssl.def;
/cvsroot/mozilla/security/nss/lib/ssl/ssl.def,v  <--  ssl.def
new revision: 1.22; previous revision: 1.21
done
Checking in ssl/ssl.h;
/cvsroot/mozilla/security/nss/lib/ssl/ssl.h,v  <--  ssl.h
new revision: 1.34; previous revision: 1.33
done
Checking in ssl/ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.130; previous revision: 1.129
done
Checking in ssl/ssl3ecc.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3ecc.c,v  <--  ssl3ecc.c
new revision: 1.23; previous revision: 1.22
done
Checking in ssl/ssl3ext.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3ext.c,v  <--  ssl3ext.c
new revision: 1.8; previous revision: 1.7
done
Checking in ssl/ssl3prot.h;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3prot.h,v  <--  ssl3prot.h
new revision: 1.17; previous revision: 1.16
done
Checking in ssl/sslimpl.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslimpl.h,v  <--  sslimpl.h
new revision: 1.74; previous revision: 1.73
done
Checking in ssl/sslreveal.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslreveal.c,v  <--  sslreveal.c
new revision: 1.6; previous revision: 1.5
done
Checking in ssl/sslt.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslt.h,v  <--  sslt.h
new revision: 1.15; previous revision: 1.14
done

Comment 27

[Wan-Teh Chang]

Kai, please write a patch to change ExtensionType to SSLExtensionType
as you described at the bottom of comment 18.  Thanks.

Comment 28

[Kai Engert [:KaiE:]]

Attachment: Additional rename Patch

Thanks for catching this.

Comment 29

[Wan-Teh Chang]

Comment on attachment 424469 [details] [diff] [review]
Additional rename Patch

r=wtc.

Comment 30

[Wan-Teh Chang]

I checked in Kai's "Additional rename Patch" (attachment 424469 [details] [diff] [review]).

Checking in ssl.h;
/cvsroot/mozilla/security/nss/lib/ssl/ssl.h,v  <--  ssl.h
new revision: 1.35; previous revision: 1.34
done
Checking in sslreveal.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslreveal.c,v  <--  sslreveal.c
new revision: 1.7; previous revision: 1.6
done
Checking in sslt.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslt.h,v  <--  sslt.h
new revision: 1.16; previous revision: 1.15
done