1273677 - TLSServerSocket doesn't properly set up the session cache

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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.

Comment 1

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: MozReview Request: bug 1273677 - ensure session cache is properly configured and torn down for TLSServerSocket r?mcmanus

Review commit: https://reviewboard.mozilla.org/r/53352/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53352/

Comment 2

[Patrick McManus [:mcmanus]]

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

Comment 3

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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/

Comment 4

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1b8f35a4774e

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This caused valgrind leaks like https://treeherder.mozilla.org/logviewer.html#?job_id=28298947&repo=mozilla-inbound

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/7c847046bb5e

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Just for the record, this also leaked outside of valgrind jobs: https://treeherder.mozilla.org/logviewer.html#?job_id=28310651&repo=mozilla-inbound

Comment 8

[:Cykesiopka]

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.

Comment 9

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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

Comment 10

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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/

Comment 11

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b6a065dc1103

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b6a065dc1103

Comment 14

[Eric Tsai (no more review request accepted)]

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

Comment 15

[Gary Chen [:xeonchen]]

please ni me again after approval to make sure I can see this in time.

Comment 16

[Josh Cheng [:josh]]

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

Comment 17

[Gary Chen [:xeonchen]]

https://github.com/mozilla-b2g/gecko-b2g/commit/00121045e713f47c3d440d87911632c58fec1146