1562538 - Remove handshake/handshaking members from SSL sockets

Status: RESOLVED FIXED

(NSS :: Libraries, task, P3)

Description

[Martin Thomson [:mt:]]

ssl_Accept sets handshaking value on the wrong socket:

https://searchfox.org/nss/rev/a3005148611567dda2b2e1284391b1e2e3217e8d/lib/ssl/sslsock.c#2835-2843

That this doesn't result in a bug is because the handshaking member is only ever read in ssl_Poll(), which, conveniently, falls through to server behaviour in the case where the value is unset. And ssl_Accept is a server function (even if we, in theory, allow the server to handshake as a client).

I suspect that the right outcome here is to change to using ss->sec.isServer for ssl_Poll, just like the rest of the code. That would require moving the assignment of that variable up into SSL_ResetHandshake(), or it wouldn't be set when needed.

A more thorough fix might be to stop using ss->handshake entirely. Aside from the little bit of setup that is done in the client setup function, all the code eventually routes to ssl3_GatherCompleteHandshake (which is badly named because it runs always). The necessary socket setup could be moved to those places where the handshake function is set (SSL_ResetHandshake, ssl_SecureConnect, and ssl_Accept).

Comment 1

[Lars Eggert [:lars]]

Firefox is affected: TLSServerSocket imports its listen socket as SSL (netwerk/base/TLSServerSocket.cpp) and nsServerSocket accepts with PR_Accept (netwerk/base/nsServerSocket.cpp), and nothing calls SSL_ResetHandshake on the accepted socket. Its handshaking therefore stays sslHandshakingUndetermined (0 from PORT_ZNew), since ssl_Accept still writes ss->handshaking on the listen socket while setting ns->handshake on the accepted one (lib/ssl/sslsock.c, ssl_Accept; unchanged on current master). NSS's own servers (selfserv, the ssl_gtest TlsAgent) call SSL_ResetHandshake(fd, PR_TRUE) and are unaffected.

A small correction to comment 0: ssl_Poll does not fall through to server behaviour for such a socket; its gate (ss->handshaking != sslHandshakingUndetermined) skips the whole handshake-aware branch. Two of the skipped cases only cost spurious writable wakeups. The third, turning a would-block write during the handshake into PR_POLL_WRITE interest, is skipped too, so a server polling only for read could in principle stall while its handshake flight is blocked. We have not verified that; an ssl_gtest that accepts without SSL_ResetHandshake and forces a short write of the server's flight would settle it.

Comment 2

[Anna Weine]

Attachment: Bug 1562538 - Remove handshake/handshaking members from SSL sockets

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/nss/rev/d2b3d8a7d837
Remove handshake/handshaking members from SSL sockets r=leggert