Skip to content

Fixed fragmentation bugs for secure channel, alpn no longer fails the… #111

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

Merged
justinboswell merged 4 commits into from
Mar 16, 2019
Merged

Fixed fragmentation bugs for secure channel, alpn no longer fails the… #111

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
23f0c50
Fixed fragmentaiton bugs for secure channel, alpn no longer fails the…
JonathanHenson Mar 15, 2019
c5ccff6
Fixed bug in iocp socket callback, logging stuff it shouldn't and mor…
JonathanHenson Mar 16, 2019
925786b
Addressed PR feedback.
JonathanHenson Mar 16, 2019
c3d962a
Fixed warning on windows about append_failed
Mar 16, 2019
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
4 changes: 2 additions & 2 deletions source/windows/iocp/socket.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -835,8 +835,8 @@ void s_socket_connection_completion(
AWS_LOGF_ERROR(
AWS_LS_IO_SOCKET,
"id=%p handle=%p: connect completion triggered with error %d",
(void *)socket_args->socket,
(void *)socket_args->socket->io_handle.data.handle,
(void *)socket,
(void *)socket->io_handle.data.handle,
status_code);
int error = s_determine_socket_error(status_code);
socket_impl->vtable->connection_error(socket, error);
Expand Down
162 changes: 98 additions & 64 deletions source/windows/secure_channel_tls_handler.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,14 +87,10 @@ struct secure_channel_handler {
struct aws_byte_buf server_name;
TimeStamp sspi_timestamp;
int (*s_connection_state_fn)(struct aws_channel_handler *handler);
/* TODO: this is twice the size it needs to be because we can get fragments like:
1 incomplete message of 15kb
1 message with the remaining 1kb + another 15kb (another incomplete message).
Doubling it makes sure we don't run out of space. I suspect the RIGHT fix is to
improve the part that uses this buffer to only do small copies at a time. For now
this fixes the issue, but it needs to be reworked.
*/
uint8_t buffered_read_in_data[2 * READ_IN_SIZE];
/*
* Give a little bit of extra head room, for split records.
*/
uint8_t buffered_read_in_data[READ_IN_SIZE + KB_1];
struct aws_byte_buf buffered_read_in_data_buf;
size_t estimated_incomplete_size;
size_t read_extra;
Expand Down Expand Up @@ -939,73 +935,105 @@ static int s_do_client_side_negotiation_step_2(struct aws_channel_handler *handl
static int s_do_application_data_decrypt(struct aws_channel_handler *handler) {
struct secure_channel_handler *sc_handler = handler->impl;

/* 4 buffers are needed, only one is input, the others get zeroed out for the output operation. */
SecBuffer input_buffers[4];
AWS_ZERO_ARRAY(input_buffers);
input_buffers[0] = (SecBuffer){
.cbBuffer = (unsigned long)sc_handler->buffered_read_in_data_buf.len,
.pvBuffer = sc_handler->buffered_read_in_data_buf.buffer,
.BufferType = SECBUFFER_DATA,
};
bool has_extra = false;
int error = AWS_OP_SUCCESS;
/* when we get an Extra buffer we have to move the pointer and replay the buffer, so we loop until we don't have
any extra buffers left over, in the last phase, we then go ahead and send the output. This state function will
always say BLOCKED_ON_READ or SUCCESS. There will never be left over reads.*/
do {
has_extra = false;
Copy link
Contributor

@graebm graebm Mar 16, 2019

Choose a reason for hiding this comment

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

[trivial debatable] while loops are easier to grok, could be:

bool has_more = true;
while (has_more) {
    has_more = false;

/* 4 buffers are needed, only one is input, the others get zeroed out for the output operation. */
SecBuffer input_buffers[4];
AWS_ZERO_ARRAY(input_buffers);

size_t read_len = sc_handler->read_extra ? sc_handler->read_extra : sc_handler->buffered_read_in_data_buf.len;
size_t offset = sc_handler->read_extra ? sc_handler->buffered_read_in_data_buf.len - sc_handler->read_extra : 0;
sc_handler->read_extra = 0;

input_buffers[0] = (SecBuffer){
.cbBuffer = (unsigned long)(read_len),
.pvBuffer = sc_handler->buffered_read_in_data_buf.buffer + offset,
.BufferType = SECBUFFER_DATA,
};

SecBufferDesc buffer_desc = {
.ulVersion = SECBUFFER_VERSION,
.cBuffers = 4,
.pBuffers = input_buffers,
};
SecBufferDesc buffer_desc = {
.ulVersion = SECBUFFER_VERSION,
.cBuffers = 4,
.pBuffers = input_buffers,
};

SECURITY_STATUS status = DecryptMessage(&sc_handler->sec_handle, &buffer_desc, 0, NULL);
sc_handler->read_extra = 0;

if (status == SEC_E_OK) {
/* if SECBUFFER_DATA is the buffer type of the second buffer, we have decrypted data to process.
If SECBUFFER_DATA is the type for the fourth buffer we need to keep track of it so we can shift
everything before doing another decrypt operation.
As far as I can tell, we don't care what's in the third buffer for TLS usage.*/
if (input_buffers[1].BufferType == SECBUFFER_DATA) {
size_t decrypted_length = input_buffers[1].cbBuffer;
AWS_LOGF_TRACE(
AWS_LS_IO_TLS,
"id=%p: Decrypted message with length %llu.",
(void *)handler,
(unsigned long long)decrypted_length);
SECURITY_STATUS status = DecryptMessage(&sc_handler->sec_handle, &buffer_desc, 0, NULL);

if (input_buffers[3].BufferType == SECBUFFER_EXTRA) {
sc_handler->read_extra = input_buffers[3].cbBuffer;
if (status == SEC_E_OK) {
/* if SECBUFFER_DATA is the buffer type of the second buffer, we have decrypted data to process.
If SECBUFFER_DATA is the type for the fourth buffer we need to keep track of it so we can shift
everything before doing another decrypt operation.
As far as I can tell, we don't care what's in the third buffer for TLS usage.*/
if (input_buffers[1].BufferType == SECBUFFER_DATA) {
size_t decrypted_length = input_buffers[1].cbBuffer;
AWS_LOGF_TRACE(
AWS_LS_IO_TLS,
"id=%p: Extra (incomplete) message received with length %llu.",
"id=%p: Decrypted message with length %llu.",
(void *)handler,
(unsigned long long)sc_handler->read_extra);
(unsigned long long)decrypted_length);

assert(
decrypted_length <=
sc_handler->buffered_read_out_data_buf.capacity - sc_handler->buffered_read_out_data_buf.len);
/* take what was decrypted and append it to the output buffers.*/
memcpy(
sc_handler->buffered_read_out_data_buf.buffer + sc_handler->buffered_read_out_data_buf.len,
(uint8_t *)input_buffers[1].pvBuffer,
decrypted_length);
sc_handler->buffered_read_out_data_buf.len += decrypted_length;

/* if we have extra we have to move the pointer and do another Decrypt operation. */
if (input_buffers[3].BufferType == SECBUFFER_EXTRA) {
sc_handler->read_extra = input_buffers[3].cbBuffer;
AWS_LOGF_TRACE(
AWS_LS_IO_TLS,
"id=%p: Extra (incomplete) message received with length %llu.",
(void *)handler,
(unsigned long long)sc_handler->read_extra);
has_extra = true;
} else {
/* this means we processed everything in the buffer. */
sc_handler->buffered_read_in_data_buf.len = 0;
AWS_LOGF_TRACE(
AWS_LS_IO_TLS,
"id=%p: Decrypt ended exactly on the end of the record, resetting buffer.",
(void *)handler);
}
}

assert(
decrypted_length <=
sc_handler->buffered_read_out_data_buf.capacity - sc_handler->buffered_read_out_data_buf.len);
memcpy(
sc_handler->buffered_read_out_data_buf.buffer + sc_handler->buffered_read_out_data_buf.len,
(uint8_t *)input_buffers[1].pvBuffer,
decrypted_length);
sc_handler->buffered_read_out_data_buf.len += decrypted_length;
}

return AWS_OP_SUCCESS;
/* SEC_E_INCOMPLETE_MESSAGE means the message we tried to decrypt isn't a full record and we need to
append our next read to it and try again. */
} else if (status == SEC_E_INCOMPLETE_MESSAGE) {
sc_handler->estimated_incomplete_size = input_buffers[1].cbBuffer;
AWS_LOGF_TRACE(
AWS_LS_IO_TLS,
"id=%p: (incomplete) message received. Expecting remaining portion of size %llu.",
(void *)handler,
(unsigned long long)sc_handler->estimated_incomplete_size);
return aws_raise_error(AWS_IO_READ_WOULD_BLOCK);
} else {
AWS_LOGF_ERROR(
AWS_LS_IO_TLS, "id=%p: Error decypting message. SECURITY_STATUS is %d.", (void *)handler, (int)status);
int aws_error = s_determine_sspi_error(status);
aws_raise_error(aws_error);
return AWS_OP_ERR;
}
else if (status == SEC_E_INCOMPLETE_MESSAGE) {
sc_handler->estimated_incomplete_size = input_buffers[1].cbBuffer;
AWS_LOGF_TRACE(
AWS_LS_IO_TLS,
"id=%p: (incomplete) message received. Expecting remaining portion of size %llu.",
(void *)handler,
(unsigned long long)sc_handler->estimated_incomplete_size);
memmove(
sc_handler->buffered_read_in_data_buf.buffer,
sc_handler->buffered_read_in_data_buf.buffer + offset,
read_len);
sc_handler->buffered_read_in_data_buf.len = read_len;
aws_raise_error(AWS_IO_READ_WOULD_BLOCK);
error = AWS_OP_ERR;
} else {
AWS_LOGF_ERROR(
AWS_LS_IO_TLS, "id=%p: Error decypting message. SECURITY_STATUS is %d.", (void *)handler, (int)status);
int aws_error = s_determine_sspi_error(status);
aws_raise_error(aws_error);
return AWS_OP_ERR;
}
} while (has_extra);

return error;
}

static int s_process_pending_output_messages(struct aws_channel_handler *handler) {
Expand Down Expand Up @@ -1137,6 +1165,12 @@ static int s_process_read_message(
/* throw this one as a protocol error. */
aws_raise_error(AWS_IO_TLS_ERROR_WRITE_FAILURE);
} else {
if (sc_handler->buffered_read_out_data_buf.len) {
err = s_process_pending_output_messages(handler);
if (err) {
break;
}
}
/* prevent a deadlock due to downstream handlers wanting more data, but we have an incomplete
record, and the amount they're requesting is less than the size of a tls record. */
size_t window_size = slot->window_size;
Expand Down