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/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 {