From f3fe024dc72452f10cd4144f259408b48e4e4285 Mon Sep 17 00:00:00 2001 From: Filippo Valsorda Date: Mon, 5 Dec 2016 13:00:53 -0500 Subject: [PATCH] crypto/tls: do not drain 0-RTT data on Close There is no reason a server can't just send a CloseNotify in its first flight, and then close the connection without reading the 0-RTT data. Also, it's not expected of Close to block on reading, and interlocking with a Read can cause a deadlock. Fixes NCC-2016-001 --- .travis.yml | 4 +- _dev/tstclnt/Dockerfile | 2 +- _dev/tstclnt/retransmit.patch | 136 ---------------------------------- conn.go | 12 +-- 4 files changed, 5 insertions(+), 149 deletions(-) delete mode 100644 _dev/tstclnt/retransmit.patch diff --git a/.travis.yml b/.travis.yml index 58e31c1..485f386 100644 --- a/.travis.yml +++ b/.travis.yml @@ -10,10 +10,11 @@ go: env: - MODE=interop CLIENT=boring - MODE=interop CLIENT=bogo - - MODE=interop CLIENT=tstclnt ZRTT=1 + - MODE=interop CLIENT=tstclnt - MODE=interop CLIENT=picotls ZRTT=1 - MODE=interop CLIENT=mint - MODE=gotest + - MODE=interop CLIENT=tstclnt ZRTT=1 - MODE=interop CLIENT=boring REVISION=origin/master - MODE=interop CLIENT=tstclnt REVISION=default ZRTT=1 @@ -22,6 +23,7 @@ matrix: allow_failures: - env: MODE=interop CLIENT=boring REVISION=origin/master - env: MODE=interop CLIENT=tstclnt REVISION=default ZRTT=1 + - env: MODE=interop CLIENT=tstclnt ZRTT=1 # crashes on close_notify in 0.5RTT install: - if [ "$MODE" = "interop" ]; then ./_dev/tris-localserver/start.sh -d && docker ps -a; fi diff --git a/_dev/tstclnt/Dockerfile b/_dev/tstclnt/Dockerfile index 46a5692..64ffbe4 100644 --- a/_dev/tstclnt/Dockerfile +++ b/_dev/tstclnt/Dockerfile @@ -18,7 +18,7 @@ ENV USE_64=1 NSS_ENABLE_TLS_1_3=1 # ARG REVISION=b6dfef6d0ff0 # tstclnt resumption -ARG REVISION=de6e7c3a39d5 +ARG REVISION=2ed8aef0b360 RUN cd nss && hg pull RUN cd nss && hg checkout -C $REVISION diff --git a/_dev/tstclnt/retransmit.patch b/_dev/tstclnt/retransmit.patch deleted file mode 100644 index d05441d..0000000 --- a/_dev/tstclnt/retransmit.patch +++ /dev/null @@ -1,136 +0,0 @@ -diff --git a/cmd/tstclnt/tstclnt.c b/cmd/tstclnt/tstclnt.c -index eb114e9..419e96f 100644 ---- a/cmd/tstclnt/tstclnt.c -+++ b/cmd/tstclnt/tstclnt.c -@@ -169,19 +169,6 @@ printSecurityInfo(PRFileDesc *fd) - } - } - --void --handshakeCallback(PRFileDesc *fd, void *client_data) --{ -- const char *secondHandshakeName = (char *)client_data; -- if (secondHandshakeName) { -- SSL_SetURL(fd, secondHandshakeName); -- } -- printSecurityInfo(fd); -- if (renegotiationsDone < renegotiationsToDo) { -- SSL_ReHandshake(fd, (renegotiationsToDo < 2)); -- ++renegotiationsDone; -- } --} - - static void - PrintUsageHeader(const char *progName) -@@ -923,13 +910,19 @@ PRUint16 portno = 443; - int override = 0; - char *requestString = NULL; - PRInt32 requestStringLen = 0; -+PRBool requestSent = PR_FALSE; - PRBool enableZeroRtt = PR_FALSE; - - static int --writeBytesToServer(PRFileDesc *s, PRPollDesc *pollset, const char *buf, int nb) -+writeBytesToServer(PRFileDesc *s, const char *buf, int nb) - { - SECStatus rv; - const char *bufp = buf; -+ PRPollDesc pollset[1]; -+ -+ pollset[SSOCK_FD].in_flags = PR_POLL_WRITE | PR_POLL_EXCEPT; -+ pollset[SSOCK_FD].out_flags = 0; -+ pollset[SSOCK_FD].fd = s; - - FPRINTF(stderr, "%s: Writing %d bytes to server\n", - progName, nb); -@@ -975,6 +968,36 @@ writeBytesToServer(PRFileDesc *s, PRPollDesc *pollset, const char *buf, int nb) - return 0; - } - -+void -+handshakeCallback(PRFileDesc *fd, void *client_data) -+{ -+ const char *secondHandshakeName = (char *)client_data; -+ if (secondHandshakeName) { -+ SSL_SetURL(fd, secondHandshakeName); -+ } -+ printSecurityInfo(fd); -+ if (renegotiationsDone < renegotiationsToDo) { -+ SSL_ReHandshake(fd, (renegotiationsToDo < 2)); -+ ++renegotiationsDone; -+ } -+ if (requestString && requestSent) { -+ /* This data was sent in 0-RTT. */ -+ SSLChannelInfo info; -+ SECStatus rv; -+ -+ rv = SSL_GetChannelInfo(fd, &info, sizeof(info)); -+ if (rv != SECSuccess) -+ return; -+ -+ if (!info.earlyDataAccepted) { -+ FPRINTF(stderr, "Early data rejected. Re-sending\n"); -+ writeBytesToServer(fd, requestString, requestStringLen); -+ } -+ } -+} -+ -+#define REQUEST_WAITING (requestString && !requestSent) -+ - static int - run_client(void) - { -@@ -988,7 +1011,8 @@ run_client(void) - PRFileDesc *std_out; - PRPollDesc pollset[2]; - PRBool wrStarted = PR_FALSE; -- char *requestStringInt = requestString; -+ -+ requestSent = PR_FALSE; - - /* Create socket */ - s = PR_OpenTCPSocket(addr.raw.family); -@@ -1245,7 +1269,7 @@ run_client(void) - pollset[SSOCK_FD].in_flags = PR_POLL_EXCEPT | - (clientSpeaksFirst ? 0 : PR_POLL_READ); - pollset[STDIN_FD].fd = PR_GetSpecialFD(PR_StandardInput); -- if (!requestStringInt) { -+ if (!REQUEST_WAITING) { - pollset[STDIN_FD].in_flags = PR_POLL_READ; - npds = 2; - } else { -@@ -1295,7 +1319,7 @@ run_client(void) - */ - FPRINTF(stderr, "%s: ready...\n", progName); - while ((pollset[SSOCK_FD].in_flags | pollset[STDIN_FD].in_flags) || -- requestStringInt) { -+ REQUEST_WAITING) { - char buf[4000]; /* buffer for stdin */ - int nb; /* num bytes read from stdin. */ - -@@ -1333,13 +1357,12 @@ run_client(void) - "%s: PR_Poll returned 0x%02x for socket out_flags.\n", - progName, pollset[SSOCK_FD].out_flags); - } -- if (requestStringInt) { -- error = writeBytesToServer(s, pollset, -- requestStringInt, requestStringLen); -+ if (REQUEST_WAITING) { -+ error = writeBytesToServer(s, requestString, requestStringLen); - if (error) { - goto done; - } -- requestStringInt = NULL; -+ requestSent = PR_TRUE; - pollset[SSOCK_FD].in_flags = PR_POLL_READ; - } - if (pollset[STDIN_FD].out_flags & PR_POLL_READ) { -@@ -1356,7 +1379,7 @@ run_client(void) - /* EOF on stdin, stop polling stdin for read. */ - pollset[STDIN_FD].in_flags = 0; - } else { -- error = writeBytesToServer(s, pollset, buf, nb); -+ error = writeBytesToServer(s, buf, nb); - if (error) { - goto done; - } diff --git a/conn.go b/conn.go index 9223f1a..27acbef 100644 --- a/conn.go +++ b/conn.go @@ -1423,17 +1423,7 @@ func (c *Conn) Close() error { var alertErr error c.handshakeMutex.Lock() - for c.phase == readingEarlyData { - if err := c.readRecord(recordTypeApplicationData); err != nil { - alertErr = err - } - } - if alertErr == nil && c.phase == waitingClientFinished { - if err := c.hs.readClientFinished13(); err != nil { - alertErr = err - } - } - if alertErr == nil && c.phase != handshakeRunning { + if c.phase != handshakeRunning { alertErr = c.closeNotify() } c.handshakeMutex.Unlock()