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.

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

Utf8 validation for topics, topic filters, username and client id #324

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
26 changes: 21 additions & 5 deletions source/client.c
Original file line number Diff line number Diff line change
Expand Up @@ -942,18 +942,18 @@ static int s_aws_mqtt_client_connection_311_set_will(
return aws_raise_error(AWS_ERROR_INVALID_STATE);
}

if (!aws_mqtt_is_valid_topic(topic)) {
AWS_LOGF_ERROR(AWS_LS_MQTT_CLIENT, "id=%p: Will topic is invalid", (void *)connection);
return aws_raise_error(AWS_ERROR_MQTT_INVALID_TOPIC);
}

int result = AWS_OP_ERR;
AWS_LOGF_TRACE(
AWS_LS_MQTT_CLIENT,
"id=%p: Setting last will with topic \"" PRInSTR "\"",
(void *)connection,
AWS_BYTE_CURSOR_PRI(*topic));

if (!aws_mqtt_is_valid_topic(topic)) {
AWS_LOGF_ERROR(AWS_LS_MQTT_CLIENT, "id=%p: Will topic is invalid", (void *)connection);
return aws_raise_error(AWS_ERROR_MQTT_INVALID_TOPIC);
}

struct aws_byte_buf local_topic_buf;
struct aws_byte_buf local_payload_buf;
AWS_ZERO_STRUCT(local_topic_buf);
Expand Down Expand Up @@ -1007,6 +1007,12 @@ static int s_aws_mqtt_client_connection_311_set_login(
return aws_raise_error(AWS_ERROR_INVALID_STATE);
}

if (username != NULL && aws_mqtt_validate_utf8_text(*username) == AWS_OP_ERR) {
AWS_LOGF_DEBUG(
AWS_LS_MQTT_CLIENT, "id=%p: Invalid utf8 or forbidden codepoints in username", (void *)connection);
return aws_raise_error(AWS_ERROR_INVALID_ARGUMENT);
}

int result = AWS_OP_ERR;
AWS_LOGF_TRACE(AWS_LS_MQTT_CLIENT, "id=%p: Setting username and password", (void *)connection);

Expand Down Expand Up @@ -1428,6 +1434,16 @@ static int s_aws_mqtt_client_connection_311_connect(

struct aws_mqtt_client_connection_311_impl *connection = impl;

if (connection_options == NULL) {
return aws_raise_error(AWS_ERROR_INVALID_ARGUMENT);
}

if (aws_mqtt_validate_utf8_text(connection_options->client_id) == AWS_OP_ERR) {
AWS_LOGF_DEBUG(
AWS_LS_MQTT_CLIENT, "id=%p: Invalid utf8 or forbidden codepoints in client id", (void *)connection);
return aws_raise_error(AWS_ERROR_INVALID_ARGUMENT);
}

/* TODO: Do we need to support resuming the connection if user connect to the same connection & endpoint and the
* clean_session is false?
* If not, the broker will resume the connection in this case, and we pretend we are making a new connection, which
Expand Down
4 changes: 4 additions & 0 deletions source/mqtt.c
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@ static bool s_is_valid_topic(const struct aws_byte_cursor *topic, bool is_filter
return false;
}

if (aws_mqtt_validate_utf8_text(*topic) == AWS_OP_ERR) {
return false;
}

/* [MQTT-4.7.3-2] Check for the null character */
if (memchr(topic->ptr, 0, topic->len)) {
return false;
Expand Down
3 changes: 3 additions & 0 deletions tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,9 @@ add_test_case(mqtt_connection_reconnection_backoff_reset_after_disconnection)
add_test_case(mqtt_validation_failure_publish_qos)
add_test_case(mqtt_validation_failure_subscribe_empty)
add_test_case(mqtt_validation_failure_unsubscribe_null)
add_test_case(mqtt_validation_failure_connect_invalid_client_id_utf8)
add_test_case(mqtt_validation_failure_invalid_will_topic_utf8)
add_test_case(mqtt_validation_failure_invalid_username_utf8)

# Operation statistics tests
add_test_case(mqtt_operation_statistics_simple_publish)
Expand Down
File renamed without changes.
74 changes: 73 additions & 1 deletion tests/v3/connection_state_test.c
Original file line number Diff line number Diff line change
Expand Up @@ -3868,4 +3868,76 @@ AWS_TEST_CASE_FIXTURE(
s_setup_mqtt_server_fn,
s_test_mqtt_validation_failure_unsubscribe_null_fn,
s_clean_up_mqtt_server_fn,
&test_data)
&test_data)

static struct aws_byte_cursor s_bad_client_id_utf8 = AWS_BYTE_CUR_INIT_FROM_STRING_LITERAL("\x41\xED\xA0\x80\x41");
static struct aws_byte_cursor s_bad_username_utf8 = AWS_BYTE_CUR_INIT_FROM_STRING_LITERAL("\x41\x00\x41");
static struct aws_byte_cursor s_bad_will_topic_utf8 = AWS_BYTE_CUR_INIT_FROM_STRING_LITERAL("\x41\xED\xBF\xBF");

static int s_test_mqtt_validation_failure_connect_invalid_client_id_utf8_fn(
struct aws_allocator *allocator,
void *ctx) {
(void)allocator;
struct mqtt_connection_state_test *state_test_data = ctx;

struct aws_mqtt_connection_options connection_options = {
.user_data = state_test_data,
.clean_session = false,
.client_id = s_bad_client_id_utf8,
.host_name = aws_byte_cursor_from_c_str(state_test_data->endpoint.address),
.socket_options = &state_test_data->socket_options,
.on_connection_complete = s_on_connection_complete_fn,
};

ASSERT_FAILS(aws_mqtt_client_connection_connect(state_test_data->mqtt_connection, &connection_options));
int error_code = aws_last_error();
ASSERT_INT_EQUALS(AWS_ERROR_INVALID_ARGUMENT, error_code);

return AWS_OP_SUCCESS;
}

AWS_TEST_CASE_FIXTURE(
mqtt_validation_failure_connect_invalid_client_id_utf8,
s_setup_mqtt_server_fn,
s_test_mqtt_validation_failure_connect_invalid_client_id_utf8_fn,
s_clean_up_mqtt_server_fn,
&test_data)

static int s_test_mqtt_validation_failure_invalid_will_topic_utf8_fn(struct aws_allocator *allocator, void *ctx) {
(void)allocator;
struct mqtt_connection_state_test *state_test_data = ctx;

struct aws_byte_cursor will_topic_cursor = s_bad_will_topic_utf8;
ASSERT_FAILS(aws_mqtt_client_connection_set_will(
state_test_data->mqtt_connection, &will_topic_cursor, AWS_MQTT_QOS_AT_MOST_ONCE, false, &will_topic_cursor));
int error_code = aws_last_error();
ASSERT_INT_EQUALS(AWS_ERROR_MQTT_INVALID_TOPIC, error_code);

return AWS_OP_SUCCESS;
}

AWS_TEST_CASE_FIXTURE(
mqtt_validation_failure_invalid_will_topic_utf8,
s_setup_mqtt_server_fn,
s_test_mqtt_validation_failure_invalid_will_topic_utf8_fn,
s_clean_up_mqtt_server_fn,
&test_data)

static int s_test_mqtt_validation_failure_invalid_username_utf8_fn(struct aws_allocator *allocator, void *ctx) {
(void)allocator;
struct mqtt_connection_state_test *state_test_data = ctx;

struct aws_byte_cursor login_cursor = s_bad_username_utf8;
ASSERT_FAILS(aws_mqtt_client_connection_set_login(state_test_data->mqtt_connection, &login_cursor, NULL));
int error_code = aws_last_error();
ASSERT_INT_EQUALS(AWS_ERROR_INVALID_ARGUMENT, error_code);

return AWS_OP_SUCCESS;
}

AWS_TEST_CASE_FIXTURE(
mqtt_validation_failure_invalid_username_utf8,
s_setup_mqtt_server_fn,
s_test_mqtt_validation_failure_invalid_username_utf8_fn,
s_clean_up_mqtt_server_fn,
&test_data)
4 changes: 4 additions & 0 deletions tests/v3/topic_tree_test.c
Original file line number Diff line number Diff line change
Expand Up @@ -334,6 +334,8 @@ static int s_mqtt_topic_validation_fn(struct aws_allocator *allocator, void *ctx
ASSERT_TOPIC_VALIDITY(FALSE, "sport/+/player1");
ASSERT_TOPIC_VALIDITY(FALSE, "sport+");

ASSERT_TOPIC_VALIDITY(FALSE, "\x41/\xED\xBF\xBF/\x41");

return AWS_OP_SUCCESS;
}

Expand Down Expand Up @@ -362,5 +364,7 @@ static int s_mqtt_topic_filter_validation_fn(struct aws_allocator *allocator, vo
ASSERT_TOPIC_FILTER_VALIDITY(TRUE, "sport/+/player1");
ASSERT_TOPIC_FILTER_VALIDITY(FALSE, "sport+");

ASSERT_TOPIC_FILTER_VALIDITY(FALSE, "\x41/\xED\xA0\x80/\x41");

return AWS_OP_SUCCESS;
}