Closed Bug 1273677 Opened 5 years ago Closed 5 years ago

TLSServerSocket doesn't properly set up the session cache

Categories

(Core :: Networking, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
blocking-b2g 2.6+
Tracking Status
firefox49 --- fixed
b2g-v2.6 --- fixed

People

(Reporter: keeler, Assigned: keeler)

References

Details

(Whiteboard: [ft:conndevices][necko-active])

Attachments

(1 file)

TLSServerSocket (implementation of nsITLSServerSocket) doesn't set up the session cache, which is a problem as the cache defaults to being enabled. This issue can be exposed by running a server and having a client attempt to connect to it more than once.
Attachment #8753565 - Flags: review?(mcmanus) → review+
Comment on attachment 8753565 [details]
MozReview Request: bug 1273677 - ensure session cache is properly configured and torn down for TLSServerSocket r?mcmanus

https://reviewboard.mozilla.org/r/53352/#review50368
Whiteboard: [necko-active]
Comment on attachment 8753565 [details]
MozReview Request: bug 1273677 - ensure session cache is properly configured and torn down for TLSServerSocket r?mcmanus

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53352/diff/1-2/
Thanks for the review. I forgot to add SSL_ShutdownServerSessionIDCache to nss.symbols, and I decreased the maximum cache size from 10000 (the default) to 1000 to save a bit of memory. Here's the try run:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5124406b3a4c
https://reviewboard.mozilla.org/r/53352/#review51074

::: security/manager/ssl/nsNSSComponent.cpp:1604
(Diff revision 2)
> +  // TLSServerSocket may be run with the session cache enabled. It is necessary
> +  // to call this once before that can happen. This specifies a maximum of 1000
> +  // cache entries (the default number of cache entries is 10000, which seems a
> +  // little excessive as there probably won't be that many clients connecting to
> +  // any TLSServerSockets the browser runs.)
> +  SSL_ConfigServerSessionIDCache(1000, 0, 0, nullptr);

Drive by since this got backed out: the return value should probably be checked or explicitly ignored.

::: security/manager/ssl/nsNSSComponent.cpp:1658
(Diff revision 2)
>      ShutdownSmartCardThreads();
>  #endif
>      SSL_ClearSessionCache();
> +    // TLSServerSocket may be run with the session cache enabled. This ensures
> +    // those resources are cleaned up.
> +    SSL_ShutdownServerSessionIDCache();

Same here.
It looks like SSL_ConfigServerSessionIDCache has to occur before all calls to SSL_ClearSessionCache. This should do it:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=19df616e6738
Flags: needinfo?(dkeeler)
Comment on attachment 8753565 [details]
MozReview Request: bug 1273677 - ensure session cache is properly configured and torn down for TLSServerSocket r?mcmanus

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53352/diff/2-3/
https://reviewboard.mozilla.org/r/53352/#review51074

> Drive by since this got backed out: the return value should probably be checked or explicitly ignored.

Good call.
https://hg.mozilla.org/mozilla-central/rev/b6a065dc1103
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Comment on attachment 8753565 [details]
MozReview Request: bug 1273677 - ensure session cache is properly configured and torn down for TLSServerSocket r?mcmanus

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 1273677
User impact if declined: tls reconnection speed will be a little bit slow
Testing completed: self-tested
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: N/A
Flags: needinfo?(xeonchen)
Attachment #8753565 - Flags: approval-mozilla-b2g48?
please ni me again after approval to make sure I can see this in time.
Flags: needinfo?(xeonchen)
Comment on attachment 8753565 [details]
MozReview Request: bug 1273677 - ensure session cache is properly configured and torn down for TLSServerSocket r?mcmanus

Approve for TV 2.6
Attachment #8753565 - Flags: approval-mozilla-b2g48? → approval-mozilla-b2g48+
blocking-b2g: --- → 2.6+
Whiteboard: [necko-active] → [ft:conndevices][necko-active]
You need to log in before you can comment on or make changes to this bug.