kris/boringssl
1
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

Only read 5 bytes (record header length) in sniffing for V2ClientHello.

Browse Source 
This guarantees that we never read beyond the first record, even if the
first record is empty. Between removing SSL_set_read_ahead and DTLS
enforcing record boundaries, this means the buffer need never memmove
data.

The memmove isn't really much of a burden and we can probably just put
SSL_set_read_ahead back after the cleanup if desired. But while the
non-existant read_ahead is off, we should avoid reading more than needed.

(Also the current memmove logic is completely wrong for TLS. Checking
align != 0 doesn't make sense. The real reason to memmove is that the
next record may still be full size. So now line 209 of s3_pkt.c should
*actually* be unreachable.)

SSL_R_HTTPS_PROXY_REQUEST detection is now slightly less accurate, but
OpenSSL was already not parsing HTTP completely. We could asynchronously
read the extra 3 bytes once the first 5 match, but that seems
unnecessary. (Shall we just get rid of all these HTTP detectors? The
only consumer of those error codes is some diagnostics logic.)

BUG=468889

Change-Id: Ie3bf148ae7274795e1d048d78282d1d8063278ea
Reviewed-on: https://boringssl-review.googlesource.com/5714
Reviewed-by: Adam Langley <agl@google.com>
This commit is contained in:
David Benjamin 2015-07-24 23:48:52 -04:00 committed by Adam Langley
parent 45c6c3e8ef
commit 894f48c6b3
1 changed files with 14 additions and 17 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

31
ssl/s3_srvr.c
View File

@ -172,10 +172,6 @@
#include "../crypto/dh/internal.h"
/* INITIAL_SNIFF_BUFFER_SIZE is the number of bytes read in the initial sniff
* buffer. */
#define INITIAL_SNIFF_BUFFER_SIZE 8
int ssl3_accept(SSL *s) {
BUF_MEM *buf = NULL;
uint32_t alg_a;
@ -590,13 +586,14 @@ end:
}
int ssl3_get_initial_bytes(SSL *s) {
/* Read the first 8 bytes. To recognize a V2ClientHello only needs the first 4
* bytes, but 8 is needed to recognize CONNECT below. */
int ret = ssl3_read_n(s, INITIAL_SNIFF_BUFFER_SIZE, 0 /* new packet */);
/* Read the first 5 bytes, the size of the TLS record header. This is
* sufficient to detect a V2ClientHello and ensures that we never read beyond
* the first record. */
int ret = ssl3_read_n(s, SSL3_RT_HEADER_LENGTH, 0 /* new packet */);
if (ret <= 0) {
return ret;
}
assert(s->packet_length == INITIAL_SNIFF_BUFFER_SIZE);
assert(s->packet_length == SSL3_RT_HEADER_LENGTH);
const uint8_t *p = s->packet;
/* Some dedicated error codes for protocol mixups should the application wish
@ -609,7 +606,7 @@ int ssl3_get_initial_bytes(SSL *s) {
OPENSSL_PUT_ERROR(SSL, SSL_R_HTTP_REQUEST);
return -1;
}
if (strncmp("CONNECT ", (const char *)p, 8) == 0) {
if (strncmp("CONNE", (const char *)p, 5) == 0) {
OPENSSL_PUT_ERROR(SSL, SSL_R_HTTPS_PROXY_REQUEST);
return -1;
}
@ -625,9 +622,9 @@ int ssl3_get_initial_bytes(SSL *s) {
/* Fall through to the standard logic. Unread what's been read to re-process
* it. */
assert(s->rstate == SSL_ST_READ_HEADER);
assert(s->s3->rbuf.offset >= INITIAL_SNIFF_BUFFER_SIZE);
s->s3->rbuf.offset -= INITIAL_SNIFF_BUFFER_SIZE;
s->s3->rbuf.left += INITIAL_SNIFF_BUFFER_SIZE;
assert(s->s3->rbuf.offset >= SSL3_RT_HEADER_LENGTH);
s->s3->rbuf.offset -= SSL3_RT_HEADER_LENGTH;
s->s3->rbuf.left += SSL3_RT_HEADER_LENGTH;
s->packet = NULL;
s->packet_length = 0;
@ -646,24 +643,24 @@ int ssl3_get_v2_client_hello(SSL *s) {
uint8_t random[SSL3_RANDOM_SIZE];
/* Determine the length of the V2ClientHello. */
assert(s->packet_length >= INITIAL_SNIFF_BUFFER_SIZE);
assert(s->packet_length >= SSL3_RT_HEADER_LENGTH);
p = (const uint8_t *)s->packet;
msg_length = ((p[0] & 0x7f) << 8) | p[1];
if (msg_length > (1024 * 4)) {
OPENSSL_PUT_ERROR(SSL, SSL_R_RECORD_TOO_LARGE);
return -1;
}
if (msg_length < INITIAL_SNIFF_BUFFER_SIZE - 2) {
if (msg_length < SSL3_RT_HEADER_LENGTH - 2) {
/* Reject lengths that are too short early. We have already read
* |INITIAL_SNIFF_BUFFER_SIZE| bytes, so we should not attempt to process an
* |SSL3_RT_HEADER_LENGTH| bytes, so we should not attempt to process an
* (invalid) V2ClientHello which would be shorter than that. */
OPENSSL_PUT_ERROR(SSL, SSL_R_RECORD_LENGTH_MISMATCH);
return -1;
}
/* Read the remainder of the V2ClientHello. We have previously read
* |INITIAL_SNIFF_BUFFER_SIZE| bytes in ssl3_get_initial_bytes. */
ret = ssl3_read_n(s, msg_length - (INITIAL_SNIFF_BUFFER_SIZE - 2),
* |SSL3_RT_HEADER_LENGTH| bytes in ssl3_get_initial_bytes. */
ret = ssl3_read_n(s, msg_length - (SSL3_RT_HEADER_LENGTH - 2),
1 /* extend */);
if (ret <= 0) {
return ret;