Closed
Bug 412833
Opened 17 years ago
Closed 16 years ago
TLS intolerant server condition is concluded prematurely
Categories
(Core :: Security: PSM, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: nelson, Assigned: mayhemer)
References
Details
Attachments
(1 file, 1 obsolete file)
8.52 KB,
patch
|
mayhemer
:
review+
|
Details | Diff | Splinter Review |
When an attempt to connect to a server using a TLS client hello fails,
PSM marks the server as "TLS intolerant", which causes all future handshake
attempts to use an alternative "fall back" handshake (currently an SSL v3
handshake in SSLv2 compatible format). PSM decides that all future
handshake attempts will use the alternative handshake, EVEN IF the
alternative handshake never succeeds.
Consequently, if the user is trying to visit a server that (say) only
understands TLS client hellos and does not understand the alternative
fall back handshake, and the user is unlucky enough that the first TLS
handshake attempt fails, the user must restart the browser to get it to
ever try a TLS handshake again.
I can imagine these solutions to this problem:
1) Have a way to try an alternative handshake once, and don't mark the
server as always requiring the alternative handshake until an alternative
handshake succeeds. Kai points out that this requires changes to the
code path for successful handshakes, as well as in the error paths.
2) Toggle the "TLS intolerant" flag on every failure. That is, if an
attempt to connect with TLS fails, set the TLS intolerant flag and retry.
If an attempt to connect with SSL3 fails, clear the TLS intolerant flag
and retry. Limit the number of retries to 1 (may be difficult).
3) Like 2, but if an attempt to connect with SSL3 fails, clear the TLS
intolerant flag, but do not immediately retry. This solves the issue
with limiting the number of retries, I think.
![]() |
Assignee | |
Comment 1•16 years ago
|
||
I like option 3. There is no need for a counter. Natural is to check TLS and then SSL on and on (on user demand by pressing F5) until user decides to give up. The option 3 will give us this behavior. When we fail to connect with TLS, try with SSL. If we fail with SSL as well, tell a user about it with easy "Try Again" option. User tries again to connect with TLS and then SSL if that fails until we connect if it was just a link congestion.
Assignee: kaie → honzab.moz
Status: NEW → ASSIGNED
![]() |
Assignee | |
Comment 2•16 years ago
|
||
Implemented the option 3. The logic is:
- TLS connection succeeded => remember the site as TLS tolerant
or
- TLS connection failed with an intolerant err => remember the site as intolerant but only if that site has not already been marked as tolerant
- SSL connection (for TLS intolerant server) failed with an intolerant err => unmark that site as TLS intolerant
Attachment #386844 -
Flags: review?(nelson)
Reporter | ||
Comment 3•16 years ago
|
||
Comment on attachment 386844 [details] [diff] [review]
v1
First, my usual disclaimer: There's so much about Firefox's C++
conventions and classes that is unknown to me that I scarcely feel
qualified to give a full r+ to this Firefox code. (I feel more
qualified to give r- than r+ :)
Now some comments:
1. This new function is unused.
>+void
>+nsSSLIOLayerHelpers::forgetPossibleTLSProblemSite(PRFileDesc* ssl_layer_fd, nsNSSSocketInfo *socketInfo)
>+{
>+ nsCAutoString key;
>+ getSiteKey(socketInfo, key);
>+
>+ removeIntolerantSite(key);
>+}
2. There is only one caller of this new function, and it passes (si->mFd, si).
Is there any foreseeable circumstance in which the first argument would be
a value other than the second argument's mFd variable?
If not, maybe this method should just take one argument, the nsNSSSocketInfo*.
>+void
>+nsSSLIOLayerHelpers::rememberTolerantSite(PRFileDesc* ssl_layer_fd, nsNSSSocketInfo *socketInfo)
3. The new code below uses a magic number. Why 16? Why not 1 or 64?
At a minimum, a comment should provide a rationale.
> mTLSIntolerantSites = new nsCStringHashSet();
> if (!mTLSIntolerantSites)
> return NS_ERROR_OUT_OF_MEMORY;
>
> mTLSIntolerantSites->Init(1);
>
>+ mTLSTolerantSites = new nsCStringHashSet();
>+ if (!mTLSTolerantSites)
>+ return NS_ERROR_OUT_OF_MEMORY;
>+
>+ mTLSTolerantSites->Init(16);
None of these issues are "must be changed" problems, so I won't give r-.
Attachment #386844 -
Flags: review?(nelson) → review+
![]() |
Assignee | |
Comment 4•16 years ago
|
||
(In reply to comment #3)
> (From update of attachment 386844 [details] [diff] [review])
> First, my usual disclaimer: There's so much about Firefox's C++
> conventions and classes that is unknown to me that I scarcely feel
> qualified to give a full r+ to this Firefox code. (I feel more
> qualified to give r- than r+ :)
You mean brace indention and so? :)
>
> Now some comments:
>
> 1. This new function is unused.
>
> >+void
> >+nsSSLIOLayerHelpers::forgetPossibleTLSProblemSite(PRFileDesc* ssl_layer_fd, nsNSSSocketInfo *socketInfo)
> >+{
> >+ nsCAutoString key;
> >+ getSiteKey(socketInfo, key);
> >+
> >+ removeIntolerantSite(key);
> >+}
Removed.
>
> 2. There is only one caller of this new function, and it passes (si->mFd, si).
> Is there any foreseeable circumstance in which the first argument would be
> a value other than the second argument's mFd variable?
> If not, maybe this method should just take one argument, the nsNSSSocketInfo*.
>
> >+void
> >+nsSSLIOLayerHelpers::rememberTolerantSite(PRFileDesc* ssl_layer_fd, nsNSSSocketInfo *socketInfo)
>
I leave it as is because nsSSLIOLayerHelpers cannot access mFd private member of nsNSSSocketInfo. Caller does.
> 3. The new code below uses a magic number. Why 16? Why not 1 or 64?
> At a minimum, a comment should provide a rationale.
>
> > mTLSIntolerantSites = new nsCStringHashSet();
> > if (!mTLSIntolerantSites)
> > return NS_ERROR_OUT_OF_MEMORY;
> >
> > mTLSIntolerantSites->Init(1);
> >
> >+ mTLSTolerantSites = new nsCStringHashSet();
> >+ if (!mTLSTolerantSites)
> >+ return NS_ERROR_OUT_OF_MEMORY;
> >+
> >+ mTLSTolerantSites->Init(16);
>
Hmm.. I just want to preallocate the hash table array because most secure servers will be TLS tolerant and we don't want to reallocate the table on each connection to a new server. There is no rule for this, just 16 seems reasonable.
> None of these issues are "must be changed" problems, so I won't give r-.
Attachment #386844 -
Attachment is obsolete: true
Attachment #387847 -
Flags: review+
![]() |
Assignee | |
Comment 5•16 years ago
|
||
Comment on attachment 387847 [details] [diff] [review]
v1.1 [Checkin comment 5]
http://hg.mozilla.org/mozilla-central/rev/5f27eedf78c9
Attachment #387847 -
Attachment description: v1.1 → v1.1 [Checkin comment 5]
![]() |
Assignee | |
Updated•16 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•