Deprecate SSL_*_read_ahead and enforce DTLS packet boundaries.

Now that WebRTC honors packet boundaries (https://crbug.com/447431), we
can start enforcing them correctly. Configuring read-ahead now does
nothing. Instead DTLS will always set "read-ahead" and also correctly
enforce packet boundaries when reading records. Add tests to ensure that
badly fragmented packets are ignored. Because such packets don't fail
the handshake, the tests work by injecting an alert in the front of the
handshake stream and ensuring the DTLS implementation ignores them.

ssl3_read_n can be be considerably unraveled now, but leave that for
future cleanup. For now, make it correct.

BUG=468889

Change-Id: I800cfabe06615af31c2ccece436ca52aed9fe899
Reviewed-on: https://boringssl-review.googlesource.com/4820
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index a7e49c9..8b69613 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -942,7 +942,6 @@
   uint32_t max_cert_list;
 
   struct cert_st /* CERT */ *cert;
-  int read_ahead;
 
   /* callback that allows applications to peek at protocol messages */
   void (*msg_callback)(int write_p, int version, int content_type,
@@ -1334,9 +1333,6 @@
   struct ssl3_state_st *s3;  /* SSLv3 variables */
   struct dtls1_state_st *d1; /* DTLSv1 variables */
 
-  int read_ahead; /* Read as many input bytes as possible
-                   * (for non-blocking reads) */
-
   /* callback that allows applications to peek at protocol messages */
   void (*msg_callback)(int write_p, int version, int content_type,
                        const void *buf, size_t len, SSL *ssl, void *arg);
@@ -1859,7 +1855,6 @@
 OPENSSL_EXPORT int SSL_get_rfd(const SSL *s);
 OPENSSL_EXPORT int SSL_get_wfd(const SSL *s);
 OPENSSL_EXPORT const char *SSL_get_cipher_list(const SSL *s, int n);
-OPENSSL_EXPORT int SSL_get_read_ahead(const SSL *s);
 OPENSSL_EXPORT int SSL_pending(const SSL *s);
 OPENSSL_EXPORT int SSL_set_fd(SSL *s, int fd);
 OPENSSL_EXPORT int SSL_set_rfd(SSL *s, int fd);
@@ -1868,7 +1863,6 @@
 OPENSSL_EXPORT BIO *SSL_get_rbio(const SSL *s);
 OPENSSL_EXPORT BIO *SSL_get_wbio(const SSL *s);
 OPENSSL_EXPORT int SSL_set_cipher_list(SSL *s, const char *str);
-OPENSSL_EXPORT void SSL_set_read_ahead(SSL *s, int yes);
 OPENSSL_EXPORT int SSL_get_verify_mode(const SSL *s);
 OPENSSL_EXPORT int SSL_get_verify_depth(const SSL *s);
 OPENSSL_EXPORT int (*SSL_get_verify_callback(const SSL *s))(int,
@@ -2166,11 +2160,6 @@
  * |ctx| */
 OPENSSL_EXPORT int SSL_CTX_get_session_cache_mode(const SSL_CTX *ctx);
 
-/* TODO(davidben): Deprecate read_ahead functions after https://crbug.com/447431
- * is resolved. */
-OPENSSL_EXPORT int SSL_CTX_get_read_ahead(const SSL_CTX *ctx);
-OPENSSL_EXPORT void SSL_CTX_set_read_ahead(SSL_CTX *ctx, int yes);
-
 /* SSL_CTX_get_max_cert_list returns the maximum length, in bytes, of a peer
  * certificate chain accepted by |ctx|. */
 OPENSSL_EXPORT size_t SSL_CTX_get_max_cert_list(const SSL_CTX *ctx);
@@ -2403,6 +2392,18 @@
 /* SSL_set_tmp_rsa returns one. */
 OPENSSL_EXPORT int SSL_set_tmp_rsa(SSL *ssl, const RSA *rsa);
 
+/* SSL_CTX_get_read_head returns zero. */
+OPENSSL_EXPORT int SSL_CTX_get_read_ahead(const SSL_CTX *ctx);
+
+/* SSL_CTX_set_read_ahead does nothing. */
+OPENSSL_EXPORT void SSL_CTX_set_read_ahead(SSL_CTX *ctx, int yes);
+
+/* SSL_get_read_head returns zero. */
+OPENSSL_EXPORT int SSL_get_read_ahead(const SSL *s);
+
+/* SSL_set_read_ahead does nothing. */
+OPENSSL_EXPORT void SSL_set_read_ahead(SSL *s, int yes);
+
 
 /* Android compatibility section.
  *
diff --git a/ssl/d1_pkt.c b/ssl/d1_pkt.c
index 45759f3..da2e414 100644
--- a/ssl/d1_pkt.c
+++ b/ssl/d1_pkt.c
@@ -261,7 +261,7 @@
  * used only by dtls1_read_bytes */
 int dtls1_get_record(SSL *s) {
   uint8_t ssl_major, ssl_minor;
-  int i, n;
+  int n;
   SSL3_RECORD *rr;
   uint8_t *p = NULL;
   uint16_t version;
@@ -344,14 +344,9 @@
 
   if (rr->length > s->packet_length - DTLS1_RT_HEADER_LENGTH) {
     /* now s->packet_length == DTLS1_RT_HEADER_LENGTH */
-    i = rr->length;
-    n = ssl3_read_n(s, i, 1);
-    if (n <= 0) {
-      return n; /* error or non-blocking io */
-    }
-
-    /* this packet contained a partial record, dump it */
-    if (n != i) {
+    n = ssl3_read_n(s, rr->length, 1);
+    /* This packet contained a partial record, dump it. */
+    if (n != rr->length) {
       rr->length = 0;
       s->packet_length = 0;
       goto again;
diff --git a/ssl/s3_pkt.c b/ssl/s3_pkt.c
index 18cf47b..6f86751 100644
--- a/ssl/s3_pkt.c
+++ b/ssl/s3_pkt.c
@@ -129,9 +129,13 @@
    * if |extend| is 1, increase packet by another n bytes.
    *
    * The packet will be in the sub-array of |s->s3->rbuf.buf| specified by
-   * |s->packet| and |s->packet_length|. (If |s->read_ahead| is set and |extend|
-   * is 0, additional bytes may be read into |rbuf|, up to the size of the
-   * buffer.) */
+   * |s->packet| and |s->packet_length|. (If DTLS and |extend| is 0, additional
+   * bytes will be read into |rbuf|, up to the size of the buffer.)
+   *
+   * TODO(davidben): |dtls1_get_record| and |ssl3_get_record| have very
+   * different needs. Separate the two record layers. In DTLS, |BIO_read| is
+   * called at most once, and only when |extend| is 0. In TLS, the buffer never
+   * contains more than one record. */
   int i, len, left;
   uintptr_t align = 0;
   uint8_t *pkt;
@@ -175,8 +179,9 @@
 
   /* For DTLS/UDP reads should not span multiple packets because the read
    * operation returns the whole packet at once (as long as it fits into the
-   * buffer). */
-  if (SSL_IS_DTLS(s) && left > 0 && n > left) {
+   * buffer). Moreover, if |extend| is true, we must not read another packet,
+   * even if the entire packet was consumed. */
+  if (SSL_IS_DTLS(s) && ((left > 0 && n > left) || extend)) {
     n = left;
   }
 
@@ -207,7 +212,7 @@
   }
 
   int max = n;
-  if (s->read_ahead && !extend) {
+  if (SSL_IS_DTLS(s) && !extend) {
     max = rb->len - rb->offset;
   }
 
diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index 276d0fd..3a2e0dd 100644
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -275,7 +275,6 @@
     goto err;
   }
 
-  s->read_ahead = ctx->read_ahead;
   s->msg_callback = ctx->msg_callback;
   s->msg_callback_arg = ctx->msg_callback_arg;
   s->verify_mode = ctx->verify_mode;
@@ -756,20 +755,15 @@
   X509_VERIFY_PARAM_set_depth(s->param, depth);
 }
 
-int SSL_CTX_get_read_ahead(const SSL_CTX *ctx) { return ctx->read_ahead; }
+int SSL_CTX_get_read_ahead(const SSL_CTX *ctx) { return 0; }
 
-int SSL_get_read_ahead(const SSL *s) { return s->read_ahead; }
+int SSL_get_read_ahead(const SSL *s) { return 0; }
 
-void SSL_CTX_set_read_ahead(SSL_CTX *ctx, int yes) { ctx->read_ahead = !!yes; }
+void SSL_CTX_set_read_ahead(SSL_CTX *ctx, int yes) { }
 
-void SSL_set_read_ahead(SSL *s, int yes) { s->read_ahead = !!yes; }
+void SSL_set_read_ahead(SSL *s, int yes) { }
 
 int SSL_pending(const SSL *s) {
-  /* SSL_pending cannot work properly if read-ahead is enabled
-   * (SSL_[CTX_]ctrl(..., SSL_CTRL_SET_READ_AHEAD, 1, NULL)), and it is
-   * impossible to fix since SSL_pending cannot report errors that may be
-   * observed while scanning the new data. (Note that SSL_pending() is often
-   * used as a boolean value, so we'd better not return -1.). */
   return s->method->ssl_pending(s);
 }
 
@@ -1673,7 +1667,6 @@
   ret->app_verify_arg = NULL;
 
   ret->max_cert_list = SSL_MAX_CERT_LIST_DEFAULT;
-  ret->read_ahead = 0;
   ret->msg_callback = 0;
   ret->msg_callback_arg = NULL;
   ret->verify_mode = SSL_VERIFY_NONE;
diff --git a/ssl/test/bssl_shim.cc b/ssl/test/bssl_shim.cc
index a9d21eb..71c5bf0 100644
--- a/ssl/test/bssl_shim.cc
+++ b/ssl/test/bssl_shim.cc
@@ -406,14 +406,6 @@
     return nullptr;
   }
 
-  if (config->is_dtls) {
-    // DTLS needs read-ahead to function on a datagram BIO.
-    //
-    // TODO(davidben): this should not be necessary. DTLS code should only
-    // expect a datagram BIO.
-    SSL_CTX_set_read_ahead(ssl_ctx.get(), 1);
-  }
-
   if (!SSL_CTX_set_cipher_list(ssl_ctx.get(), "ALL")) {
     return nullptr;
   }
diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go
index e1479e9..57524a7 100644
--- a/ssl/test/runner/common.go
+++ b/ssl/test/runner/common.go
@@ -681,6 +681,11 @@
 	// fragments in DTLS.
 	SendEmptyFragments bool
 
+	// SendSplitAlert, if true, causes an alert to be sent with the header
+	// and record body split across multiple packets. The peer should
+	// discard these packets rather than process it.
+	SendSplitAlert bool
+
 	// FailIfResumeOnRenego, if true, causes renegotiations to fail if the
 	// client offers a resumption or the server accepts one.
 	FailIfResumeOnRenego bool
diff --git a/ssl/test/runner/conn.go b/ssl/test/runner/conn.go
index fd198ca..ec7a4a0 100644
--- a/ssl/test/runner/conn.go
+++ b/ssl/test/runner/conn.go
@@ -1260,6 +1260,15 @@
 		return nil
 	}
 
+	if c.isDTLS && c.config.Bugs.SendSplitAlert {
+		c.conn.Write([]byte{
+			byte(recordTypeAlert), // type
+			0xfe, 0xff, // version
+			0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, // sequence
+			0x0, 0x2, // length
+		})
+		c.conn.Write([]byte{alertLevelError, byte(alertInternalError)})
+	}
 	if c.isClient {
 		c.handshakeErr = c.clientHandshake()
 	} else {
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index bb21847..e4a3f9a 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -1086,6 +1086,25 @@
 		},
 		expectedCipher: TLS_DHE_RSA_WITH_AES_128_GCM_SHA256,
 	},
+	{
+		protocol: dtls,
+		name:     "SendSplitAlert-Sync",
+		config: Config{
+			Bugs: ProtocolBugs{
+				SendSplitAlert: true,
+			},
+		},
+	},
+	{
+		protocol: dtls,
+		name:     "SendSplitAlert-Async",
+		config: Config{
+			Bugs: ProtocolBugs{
+				SendSplitAlert: true,
+			},
+		},
+		flags: []string{"-async"},
+	},
 }
 
 func doExchange(test *testCase, config *Config, conn net.Conn, messageLen int, isResume bool) error {