302803 - Changes to enabled SSL versions do not take immediate effect

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Nelson Bolyard (seldom reads bugmail)]

This was formerly bug 194141, filed in Feb 2003.

Any time that the user changes the set of enabled SSL2/SSL3/TLS versions,
or changes the set of ciphersuites permitted for any of those versions, 
PSM should call SSL_ClearSessionCache after making the change.  This 
ensures that ALL SSL sessions used after the change follow the newly 
established preferences.

Comment 1

[timeless]

Attachment: like this?

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 229796 [details] [diff] [review]
like this?

This seems like the right idea, yes.  
I don't know enough about PSM to know if this is the only place that needs to be fixed, and/or whether any other works needs to be done (e.g. locks acquired/released) so I can't give this patch r+ or r-, but I see nothing obviously wrong with it.

Comment 3

[Kai Engert (:KaiE:)]

Comment on attachment 229796 [details] [diff] [review]
like this?

If I understand correctly, this patch will cause the SSL session cache to get flushed on *any* pref change.

I believe this is not what we want?

I believe we only want to clear the cache if at least one pref related to SSL has changed.

I propose this code uses a temporary boolean value, that is false by default. Within each block, where we found a match, set that boolean to true.

Only call the SSL_Clear function if at least one other SSL_ function got called.

Comment 4

[timeless]

Attachment: how about this?

Comment 5

[Kai Engert (:KaiE:)]

Comment on attachment 241694 [details] [diff] [review]
how about this?

r-

You are changing the pref identifiers? I think that's a bad idea for compatibility reasons.

Comment 6

[timeless]

Comment on attachment 241694 [details] [diff] [review]
how about this?

i'm not changing them.

Comment 7

[Kai Engert (:KaiE:)]

Timeless, I do not understand why you claim that you're NOT changing the pref strings.

Looking at your patch, it seems obvious to me you're changing them.

Can you please point me to the the place in your patch where you ensure that a change from "security.ssl3.rsa_aes_256_sha" to "ssl3.rsa_aes_256_sha" effectively does not remove the "security." prefix?

Thanks.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Kai, perhaps Timeless's claim is due to this addition:

   if (!mPrefBranch) {
-    mPrefBranch = do_GetService(NS_PREFSERVICE_CONTRACTID);
+    nsCOMPtr<nsIPrefService> ps = do_GetService(NS_PREFSERVICE_CONTRACTID);
+    if (ps) {
+      ps->GetBranch("security.", getter_AddRefs(mPrefBranch));
+    }

Timeless, I think it would be desirable to separate the fix for this bug 
from the factoring-out of the "security." prefix into separate patches.  
The factoring patch probably belongs to a different bug/RFE.

Comment 9

[Kai Engert (:KaiE:)]

Attachment: Patch v3

like this

Comment 10

[Kai Engert (:KaiE:)]

(In reply to comment #8)
> Timeless, I think it would be desirable to separate the fix for this bug 
> from the factoring-out of the "security." prefix into separate patches.  
> The factoring patch probably belongs to a different bug/RFE.

I absolutely agree. 
Timeless, you are hiding a few lines of code that fix this bug amongst many other changes. This seems confusing when trying to read your patch. It makes things more difficult to track.

I have extracted your real fix for this bug and attached Patch v3.
I think this patch is good.
Asking Bob for review.

Comment 11

[Robert Relyea]

Before I r+ kai, can you explain the expected scope of clearSessionCache? It's usage appears to be local, but it is neither declared nor initialized in the local scope (which should produce compile errors). If it's a class or global, then it's not clear what the side effect of Setting, but not clearing the variable is. (how does clearSessionCache eventually get cleared again). I want to make sure we aren't falling into a case where we are perpetually clearing the SSL session cache (which will work for a single browser, albeit slowly, but would cause a bit of havoc to web servers if a large fraction of browsers are doing this).

bob

Comment 12

[Kai Engert (:KaiE:)]

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

(In reply to comment #11)
> Before I r+ kai, can you explain the expected scope of clearSessionCache?

Sorry, i accidentially omitted the declaration of this boolean.

Comment 13

[Kai Engert (:KaiE:)]

Attachment: Patch v4

Bob, this patch revision adds the boolean variable that I missed earlier.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

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

Looks right to me.

Comment 15

[Kai Engert (:KaiE:)]

Thanks Nelson, fixed checked in.