SSL force handshake function should take a timeout

RESOLVED FIXED in 3.11.4

Status

NSS
Libraries
P2
enhancement
RESOLVED FIXED
16 years ago
11 years ago

People

(Reporter: Julien Pierre, Assigned: Nelson Bolyard (seldom reads bugmail))

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

16 years ago
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.
(Reporter)

Updated

16 years ago
Severity: normal → enhancement
(Reporter)

Comment 1

16 years ago
*** Bug 124337 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 2

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

16 years ago
Changed the QA contact to Bishakha.
QA Contact: sonja.mirtitsch → bishakhabanerjee

Comment 4

16 years ago
Target 3.6, priority P2.
Assignee: wtc → jpierre
Priority: -- → P2
Target Milestone: --- → 3.6

Updated

15 years ago
Target Milestone: 3.6 → Future
(Reporter)

Updated

13 years ago
Keywords: sun-orion4
Target Milestone: Future → 3.10
(Reporter)

Comment 5

13 years ago
Unsetting target.
Target Milestone: 3.10 → ---
(Assignee)

Updated

12 years ago
QA Contact: bishakhabanerjee → jason.m.reid
(Reporter)

Comment 6

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

Comment 7

12 years ago
Created attachment 194729 [details] [diff] [review]
Add SSL_ReHandshakeWithTimeout and SSL_ForceHandshakeWithTimeout
Attachment #194729 - Flags: review?(nelson)
(Reporter)

Updated

12 years ago
Keywords: sun-orion4
Target Milestone: --- → 3.11

Comment 8

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

Comment 9

12 years ago
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.
Attachment #194729 - Flags: review?(nelson) → review+
(Reporter)

Comment 10

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

Comment 11

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

Comment 12

12 years ago
Julien, Is this bug now resolved/fixed?  
(Reporter)

Comment 13

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

Updated

12 years ago
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Assignee)

Comment 14

11 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: 3.11 → 3.11.4
(Assignee)

Comment 15

11 years ago
Created attachment 244649 [details] [diff] [review]
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 ?
Assignee: bugzilla → nelson
Status: REOPENED → ASSIGNED
Attachment #244649 - Flags: superreview?(wtchang)
Attachment #244649 - Flags: review?(alexei.volkov.bugs)

Comment 16

11 years ago
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.
Attachment #244649 - Flags: superreview?(wtchang) → superreview+

Comment 17

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

Updated

11 years ago
Attachment #244649 - Flags: review?(alexei.volkov.bugs) → review+
(Assignee)

Comment 18

11 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.