Prune some dead quirks and document the SSL_OP_ALL ones.

Update SSL_OP_ALL to account for SSL_OP_CRYPTOPRO_TLSEXT_BUG being gone,
and update ssl3_setup_write_buffer to account for SSL_MODE_CBC_RECORD_SPLITTING
rather than the now defunct SSL_OP_DONT_INSERT_EMPTY_FRAGMENTS.

Also remove SSL_OP_TLS_BLOCK_PADDING_BUG. This is to allow for a buggy peer
which pads CBC with N bytes of value N rather than N+1 bytes of value N. This
quirk has been broken since CBC padding checks became constant-time, as
demonstrated by this attempt at a test. (Instead of just decrementing
padding_length, it needs to also keep track of a separate padding_value and not
decrement that one.)

https://boringssl-review.googlesource.com/#/c/1690/

(The quirk would also fall over anyway if the buggy client ever did a session
resumption; then the server speaks first rather than the client, and the quirk
triggered on reading the first encrypted record from the peer.)

Change-Id: I19942dc629a47832aead77a46bb50e0b0a9780b3
Reviewed-on: https://boringssl-review.googlesource.com/1694
Reviewed-by: Adam Langley <agl@google.com>

7 files changed, 15 insertions, 52 deletions

diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index da8e55e..66904fb 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -471,33 +471,21 @@ struct ssl_session_st
 
 #endif
 
-#define SSL_OP_MICROSOFT_SESS_ID_BUG			0x00000001L
-#define SSL_OP_NETSCAPE_CHALLENGE_BUG			0x00000002L
-/* Allow initial connection to servers that don't support RI */
+/* SSL_OP_LEGACY_SERVER_CONNECT allows initial connection to servers
+ * that don't support RI */
 #define SSL_OP_LEGACY_SERVER_CONNECT			0x00000004L
-#define SSL_OP_SSLREF2_REUSE_CERT_TYPE_BUG		0x00000010L
+
+/* SSL_OP_MICROSOFT_BIG_SSLV3_BUFFER allows for record sizes
+ * SSL3_RT_MAX_EXTRA bytes above the maximum record size. */
 #define SSL_OP_MICROSOFT_BIG_SSLV3_BUFFER		0x00000020L
-#define SSL_OP_SAFARI_ECDHE_ECDSA_BUG			0x00000040L
-#define SSL_OP_TLS_D5_BUG				0x00000100L
-#define SSL_OP_TLS_BLOCK_PADDING_BUG			0x00000200L
 
-/* Hasn't done anything since OpenSSL 0.9.7h, retained for compatibility */
-#define SSL_OP_MSIE_SSLV2_RSA_PADDING			0x0
+/* SSL_OP_TLS_D5_BUG accepts an RSAClientKeyExchange in TLS encoded as
+ * SSL3, without a length prefix. */
+#define SSL_OP_TLS_D5_BUG				0x00000100L
 
-/* SSL_OP_DONT_INSERT_EMPTY_FRAGMENTS is vestigial. Previously it disabled the
- * insertion of empty records in CBC mode, but the empty records were commonly
- * misinterpreted as EOF by other TLS stacks and so this was disabled by
- * SSL_OP_ALL.
- *
- * This has been replaced by 1/n-1 record splitting, which is enabled by
- * SSL_MODE_CBC_RECORD_SPLITTING in SSL_set_mode. This involves sending a
- * one-byte record rather than an empty record and has much better
- * compatibility. */
-#define SSL_OP_DONT_INSERT_EMPTY_FRAGMENTS              0x00000800L /* added in 0.9.6e */
-
-/* SSL_OP_ALL: various bug workarounds that should be rather harmless.
- *             This used to be 0x000FFFFFL before 0.9.7. */
-#define SSL_OP_ALL					0x80000BFFL
+/* SSL_OP_ALL enables the above bug workarounds that should be rather
+ * harmless. */
+#define SSL_OP_ALL					0x00000BFFL
 
 /* DTLS options */
 #define SSL_OP_NO_QUERY_MTU                 0x00001000L
diff --git a/include/openssl/ssl3.h b/include/openssl/ssl3.h
index 44f9367..3aea752 100644
--- a/include/openssl/ssl3.h
+++ b/include/openssl/ssl3.h
@@ -340,7 +340,6 @@ typedef struct ssl3_buffer_st
 
 #define SSL3_FLAGS_NO_RENEGOTIATE_CIPHERS	0x0001
 #define SSL3_FLAGS_POP_BUFFER			0x0004
-#define TLS1_FLAGS_TLS_PADDING_BUG		0x0008
 /* TODO(davidben): This flag can probably be merged into s3->change_cipher_spec
  * to something tri-state. (Normal / Expect CCS / Between CCS and Finished). */
 #define SSL3_FLAGS_EXPECT_CCS			0x0080
diff --git a/ssl/d1_enc.c b/ssl/d1_enc.c
index 70b74b8..dec0ea5 100644
--- a/ssl/d1_enc.c
+++ b/ssl/d1_enc.c
@@ -202,11 +202,6 @@ int dtls1_enc(SSL *s, int send)
 
 			/* we need to add 'i' padding bytes of value j */
 			j=i-1;
-			if (s->options & SSL_OP_TLS_BLOCK_PADDING_BUG)
-				{
-				if (s->s3->flags & TLS1_FLAGS_TLS_PADDING_BUG)
-					j++;
-				}
 			for (k=(int)l; k<(int)(l+i); k++)
 				rec->input[k]=j;
 			l+=i;
diff --git a/ssl/s3_both.c b/ssl/s3_both.c
index c494a4a..6604fc7 100644
--- a/ssl/s3_both.c
+++ b/ssl/s3_both.c
@@ -696,8 +696,9 @@ int ssl3_setup_write_buffer(SSL *s)
 		len = s->max_send_fragment
 			+ SSL3_RT_SEND_MAX_ENCRYPTED_OVERHEAD
 			+ headerlen + align;
-		if (!(s->options & SSL_OP_DONT_INSERT_EMPTY_FRAGMENTS))
-			len += headerlen + align
+		/* Account for 1/n-1 record splitting. */
+		if (s->mode & SSL_MODE_CBC_RECORD_SPLITTING)
+			len += headerlen + align + 1
 				+ SSL3_RT_SEND_MAX_ENCRYPTED_OVERHEAD;
 
 		if ((p=OPENSSL_malloc(len)) == NULL)
diff --git a/ssl/s3_cbc.c b/ssl/s3_cbc.c
index e1b18af..6a0de9c 100644
--- a/ssl/s3_cbc.c
+++ b/ssl/s3_cbc.c
@@ -165,21 +165,6 @@ int tls1_cbc_remove_padding(const SSL* s,
 
 	padding_length = rec->data[rec->length-1];
 
-	if (s->options & SSL_OP_TLS_BLOCK_PADDING_BUG)
-		{
-		/* First packet is even in size, so check */
-		if ((memcmp(s->s3->read_sequence, "\0\0\0\0\0\0\0\0",8) == 0) &&
-		    !(padding_length & 1))
-			{
-			s->s3->flags|=TLS1_FLAGS_TLS_PADDING_BUG;
-			}
-		if ((s->s3->flags & TLS1_FLAGS_TLS_PADDING_BUG) &&
-		    padding_length > 0)
-			{
-			padding_length--;
-			}
-		}
-
 	good = constant_time_ge(rec->length, overhead+padding_length);
 	/* The padding consists of a length byte at the end of the record and
 	 * then that many bytes of padding, all with the same value as the
diff --git a/ssl/s3_pkt.c b/ssl/s3_pkt.c
index 864e48d..f5079a1 100644
--- a/ssl/s3_pkt.c
+++ b/ssl/s3_pkt.c
@@ -311,7 +311,7 @@ static int ssl3_get_record(SSL *s)
 		extra=0;
 	if (extra && !s->s3->init_extra)
 		{
-		/* An application error: SLS_OP_MICROSOFT_BIG_SSLV3_BUFFER
+		/* An application error: SSL_OP_MICROSOFT_BIG_SSLV3_BUFFER
 		 * set after ssl3_setup_buffers() was done */
 		OPENSSL_PUT_ERROR(SSL, ssl3_get_record, ERR_R_INTERNAL_ERROR);
 		return -1;
diff --git a/ssl/t1_enc.c b/ssl/t1_enc.c
index abe5183..2f92524 100644
--- a/ssl/t1_enc.c
+++ b/ssl/t1_enc.c
@@ -903,11 +903,6 @@ int tls1_enc(SSL *s, int send)
 
 			/* we need to add 'i' padding bytes of value j */
 			j=i-1;
-			if (s->options & SSL_OP_TLS_BLOCK_PADDING_BUG)
-				{
-				if (s->s3->flags & TLS1_FLAGS_TLS_PADDING_BUG)
-					j++;
-				}
 			for (k=(int)l; k<(int)(l+i); k++)
 				rec->input[k]=j;
 			l+=i;