Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve QUIC defragmentation of TLS packets. #158

Closed
wants to merge 1 commit into from
Closed

Improve QUIC defragmentation of TLS packets. #158

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion ssl/ssl_lib.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -611,7 +611,6 @@ int SSL_clear_quic(SSL *s)
s->quic_input_data_tail = NULL;
BUF_MEM_free(s->quic_buf);
s->quic_buf = NULL;
s->quic_next_record_start = 0;
memset(s->client_hand_traffic_secret, 0, EVP_MAX_MD_SIZE);
memset(s->server_hand_traffic_secret, 0, EVP_MAX_MD_SIZE);
memset(s->client_early_traffic_secret, 0, EVP_MAX_MD_SIZE);
Expand Down
3 changes: 1 addition & 2 deletions ssl/ssl_local.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1226,8 +1226,8 @@ typedef struct cert_pkey_st CERT_PKEY;
struct quic_data_st {
struct quic_data_st *next;
OSSL_ENCRYPTION_LEVEL level;
size_t start; /* offset into quic_buf->data */
size_t length;
/* char data[]; should be here but C90 VLAs not allowed here */
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Paragraph 6.7.3.2.20 of n3220 permits flexible arrays as last element which would be allowed. However I think we need to double check what century our compilers are from. This is probably the clean way to do what you want, as I'm not sure if what you're doing is undefined behavior or not in all places

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also be inclined to use real C99 flexible arrays.
On a different project investigating the question of flexible array support, my colleague wrote "Using the godbolt.org side, I couldn't find a gcc or clang level that didn't support flex arrays.". That project ended up using an autoconf test with some probably-undefined-behavior alternative for systems that don't support flexible arrays (e.g., AIX), whose compilers can probably safely be assumed to not be trying to do anything with that undefined behavior.
quictls inherits its configure script from openssl, and does not have real facility for autoconf-style "try a build and see if it works" checks, but our target audience is also probably for modern enough systems that we could just assume support for it. (That same other project I mentioned also concluded that Microsoft's compilers supported it as an extension well before it was standardized.)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have any strong preference. That part of the change was simply restoring code from 075ede1.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm good with flexible arrays. @nibanks ?

};
typedef struct quic_data_st QUIC_DATA;
int quic_set_encryption_secrets(SSL *ssl, OSSL_ENCRYPTION_LEVEL level);
Expand Down Expand Up @@ -1735,7 +1735,6 @@ struct ssl_st {
int quic_transport_version;
QUIC_DATA *quic_input_data_head;
QUIC_DATA *quic_input_data_tail;
size_t quic_next_record_start;
const SSL_QUIC_METHOD *quic_method;
#endif
/*
Expand Down
91 changes: 60 additions & 31 deletions ssl/ssl_quic.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,8 @@ OSSL_ENCRYPTION_LEVEL SSL_quic_write_level(const SSL *ssl)
int SSL_provide_quic_data(SSL *ssl, OSSL_ENCRYPTION_LEVEL level,
const uint8_t *data, size_t len)
{
size_t l, offset;
size_t l;
size_t fragment_length;

if (!SSL_IS_QUIC(ssl)) {
ERR_raise(ERR_LIB_SSL, ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED);
Expand All @@ -146,70 +147,98 @@ int SSL_provide_quic_data(SSL *ssl, OSSL_ENCRYPTION_LEVEL level,
if (len == 0)
return 1;

if (ssl->quic_buf == NULL) {
BUF_MEM *buf;
if ((buf = BUF_MEM_new()) == NULL) {
ERR_raise(ERR_LIB_SSL, ERR_R_INTERNAL_ERROR);
return 0;
}
if (!BUF_MEM_grow(buf, SSL3_RT_MAX_PLAIN_LENGTH)) {
ERR_raise(ERR_LIB_SSL, ERR_R_INTERNAL_ERROR);
BUF_MEM_free(buf);
return 0;
}
ssl->quic_buf = buf;
/* We preallocated storage, but there's still no *data*. */
ssl->quic_buf->length = 0;
buf = NULL;
fragment_length = 0;
if (ssl->quic_buf != NULL) {
fragment_length = ssl->quic_buf->length;
}

/* A TLS message must not cross an encryption level boundary */
if (ssl->quic_buf->length != ssl->quic_next_record_start
&& level != ssl->quic_latest_level_received) {
if (fragment_length != 0 && level != ssl->quic_latest_level_received) {
ERR_raise(ERR_LIB_SSL, SSL_R_WRONG_ENCRYPTION_LEVEL_RECEIVED);
return 0;
}
ssl->quic_latest_level_received = level;

offset = ssl->quic_buf->length;
if (!BUF_MEM_grow(ssl->quic_buf, offset + len)) {
ERR_raise(ERR_LIB_SSL, ERR_R_INTERNAL_ERROR);
return 0;
if (fragment_length != 0) {
/*
* If we have a partial record, copy the data into the buffer, and
* parse records from it.
*/
if (!BUF_MEM_grow(ssl->quic_buf, fragment_length + len)) {
ERR_raise(ERR_LIB_SSL, ERR_R_INTERNAL_ERROR);
return 0;
}
memcpy(ssl->quic_buf->data + fragment_length, data, len);
data = (const uint8_t *)ssl->quic_buf->data;
len = ssl->quic_buf->length;
}
memcpy(ssl->quic_buf->data + offset, data, len);

/* Split on handshake message boundaries */
while (ssl->quic_buf->length > ssl->quic_next_record_start
+ SSL3_HM_HEADER_LENGTH) {
while (len >= SSL3_HM_HEADER_LENGTH) {
QUIC_DATA *qd;
const uint8_t *p;

/* TLS Handshake message header has 1-byte type and 3-byte length */
p = (const uint8_t *)ssl->quic_buf->data
+ ssl->quic_next_record_start + 1;
p = data + 1;
n2l3(p, l);
l += SSL3_HM_HEADER_LENGTH;
/* Don't allocate a QUIC_DATA if we don't have a full record */
if (l > ssl->quic_buf->length - ssl->quic_next_record_start)
if (l > len)
break;

qd = OPENSSL_zalloc(sizeof(*qd));
qd = OPENSSL_zalloc(sizeof(*qd) + l);
if (qd == NULL) {
ERR_raise(ERR_LIB_SSL, ERR_R_INTERNAL_ERROR);
return 0;
}

qd->next = NULL;
qd->length = l;
qd->start = ssl->quic_next_record_start;
qd->level = level;
memcpy((void *)(qd + 1), data, l);

if (ssl->quic_input_data_tail != NULL)
ssl->quic_input_data_tail->next = qd;
else
ssl->quic_input_data_head = qd;
ssl->quic_input_data_tail = qd;
ssl->quic_next_record_start += l;

/* Remove the now-consumed data. */
data += l;
len -= l;
}

/*
* If we have any unread data, we need to save it.
*/
if (len > 0) {
if (ssl->quic_buf != NULL) {
if (data == (const uint8_t *)ssl->quic_buf->data) {
/* It's already in the buffer; no need to do anything.. */
return 1;
}
} else {
BUF_MEM *buf;
if ((buf = BUF_MEM_new()) == NULL) {
ERR_raise(ERR_LIB_SSL, ERR_R_INTERNAL_ERROR);
return 0;
}
ssl->quic_buf = buf;
}
/*
* Note: 'data' might be in this buffer, but if that's the case,
* we're going to be shrinking the buffer, so the pointer will not be
* invalidated. We do need to use memmove() and not memcpy(), though.
*/
if (!BUF_MEM_grow(ssl->quic_buf, len)) {
ERR_raise(ERR_LIB_SSL, ERR_R_INTERNAL_ERROR);
return 0;
}
memmove(ssl->quic_buf->data, data, len);
} else {
if (ssl->quic_buf != NULL) {
ssl->quic_buf->length = 0;
}
}

return 1;
Expand Down
2 changes: 1 addition & 1 deletion ssl/statem/statem_quic.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ int quic_get_message(SSL *s, int *mt)
}

/* Copy buffered data */
memcpy(s->init_buf->data, s->quic_buf->data + qd->start, qd->length);
memcpy(s->init_buf->data, (void *)(qd + 1), qd->length);
s->init_buf->length = qd->length;
s->quic_input_data_head = qd->next;
if (s->quic_input_data_head == NULL)
Expand Down