127960 - SSL force handshake function should take a timeout

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P2)

Description

[Julien Pierre]

In the SSL code, when forcing an SSL handshake to occur through
SSL_ForceHandshake / SSL_ReHandshake / SSL_ReDoHanshake, the SSL library uses
the timeout argument from the previous PR_Recv or PR_Send operation done on the
socket. In some cases, such as for the client auth in the web server, we want to
control the handshake timeout separately from the regular socket I/O timeout. We
would like to have a new API to force the handshake to occur, taking an extra
timeout argument.

Comment 1

[Julien Pierre]

*** Bug 124337 has been marked as a duplicate of this bug. ***

Comment 2

[Julien Pierre]

Actually, this argument would not truly accomplish what's needed here. It would
only work for forced handshakes. In the case of the first full handshake, there
would be no way to control it. The initial PR_Recv timeout would probably be
used. I don't know if this is desireable or not.
I think an option set via SSL_OptionSet that would work for all cases would be
better. If that option is not set, the current behavior would apply. Otherwise
the set timeout would be used.

Comment 3

[Wan-Teh Chang]

Changed the QA contact to Bishakha.

Comment 4

[Wan-Teh Chang]

Target 3.6, priority P2.

Comment 5

[Julien Pierre]

Unsetting target.

Comment 6

[Julien Pierre]

This bug has been open for years. We have several internal customers interested
in the fix .

There are two problems with the approaches I suggested earlier

1) The type for the value of SSL socket options is declared as a PRBool.

This makes it somewhat ugly to implement an SSL option with a timeout value,
which would need to be a PRIntervalTime, itself declared as PRUint32 . 

A PRBool is a PRIntn, which is a platform-specific int, and has a minimum
requirement of only 2 bytes in prtypes.h . I believe there are no longer any
useful platforms that have less than 32-bit integers, but it would still be
preferable to have an API that took the right type for the value of the SSL
option, or maybe simply a void*, which would always be good enough for any kind
of option we set in the future.

2) the rTimeout and wTimeout within the SSL socket structure are constantly
being reset by NSPR I/O calls . If we had a global timeout property exclusively
for handshakes, we would need to change the SSL I/O code to figure out if the
I/O is happening within a handshake. This is quite complicated to do.

Because of these issues, I'm taking the simpler approach of just adding two new
ReHandshake / ForceHandshake APIs with the additional PRIntervalTime argument.
The 2 new APIs  will simply set the timeout in the SSL structure prior to
calling the old functions. I'm not providing an enhancement version of
SSL_ReDoHandshake, since that API has been marked as obsolete.

Patch forthcoming.

Comment 7

[Julien Pierre]

Attachment: Add SSL_ReHandshakeWithTimeout and SSL_ForceHandshakeWithTimeout

Comment 8

[Wan-Teh Chang]

If the interface of SSL_OptionSet is not approriate for
non-Boolean options, you can add new functions to set the
option, e.g., SSL_SetHandshakeTimeout.

In any case, I'll let you and Nelson decide the proper fix.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 194729 [details] [diff] [review]
Add SSL_ReHandshakeWithTimeout and SSL_ForceHandshakeWithTimeout

This patch adds two new public SSL functions.  I'd like more time to think
about the correctness of the implementation, but I believe the function names
and signatures are fine as is, so I'm OK with adding them to libSSL for 3.11.

Comment 10

[Julien Pierre]

Thanks for the review, Nelson. I have checked in the patch so that these new
APIs don't miss the beta . It would be harder to add them afterwards. If there
is a problem with the implementation, I can produce another patch.

Checking in sslimpl.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslimpl.h,v  <--  sslimpl.h
new revision: 1.41; previous revision: 1.40
done
Checking in sslsecur.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslsecur.c,v  <--  sslsecur.c
new revision: 1.33; previous revision: 1.32
done
Checking in sslsock.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslsock.c,v  <--  sslsock.c
new revision: 1.41; previous revision: 1.40
done
Checking in ssl.h;
/cvsroot/mozilla/security/nss/lib/ssl/ssl.h,v  <--  ssl.h
new revision: 1.24; previous revision: 1.23
done

Comment 11

[Julien Pierre]

The initial checkin broke the build. I have checked in an update. Sorry about that.

Checking in sslimpl.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslimpl.h,v  <--  sslimpl.h
new revision: 1.42; previous revision: 1.41
done
Checking in sslsecur.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslsecur.c,v  <--  sslsecur.c
new revision: 1.34; previous revision: 1.33
done
Checking in sslsock.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslsock.c,v  <--  sslsock.c
new revision: 1.42; previous revision: 1.41
done

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Julien, Is this bug now resolved/fixed?  

Comment 13

[Julien Pierre]

Nelson,

You said in comment #10 that you wanted to provide comments about the
implementation. If you have none, you can mark the bug FIXED.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Reopening.  This new feature implementation was/is incomplete.
The new functions were not exported from the shared libraries. 
The new symbols need to be added to ssl.def.
When that is done, this bug can be marked resolved/fixed again.

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch to nss.def for the 3.11 branch

I set the release version number to 3.11.4, but maybe it should be 3.11.5 ?

Comment 16

[Wan-Teh Chang]

Comment on attachment 244649 [details] [diff] [review]
patch to nss.def for the 3.11 branch

NSS 3.11.4 is the right version number for NSS_3_11_BRANCH.
I guess the export of these two functions can't wait until
NSS 3.12.

Comment 17

[Wan-Teh Chang]

Comment on attachment 194729 [details] [diff] [review]
Add SSL_ReHandshakeWithTimeout and SSL_ForceHandshakeWithTimeout

The use of locks in ssl_SetTimeout is strange.

By studying the use of SSL_LOCK_READER and
SSL_LOCK_WRITER, I concluded that they seem to
be intended for serializing ss->ops->recv and
ss->ops->send calls, respectively.

ssl_Recv may modify ss->wTimeout with only the
SSL_LOCK_READER held.

ssl_Send may modify ss->rTimeout with only the
SSL_LOCK_WRITER held.

So whether SSL_LOCK_READER and SSL_LOCK_WRITER
also protect ss->rTimeout and ss->wTimeout is
not clear and cannot be inferred by existing
usage.

Also, since ssl_SetTimeout doesn't call ss->ops->recv
or ss->ops->send, Julien's decision to always call
SSL_LOCK_READER and call SSL_LOCK_WRITER if ss->fdx
is true seems ad hoc rather than following some
rule or existing usage.

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

Checked in on trunk and NSS_3_11_BRANCH for NSS 3.11.4
Checking in ssl.def; new revision: 1.18;      previous revision: 1.17
Checking in ssl.def; new revision: 1.17.28.1; previous revision: 1.17