From d15eb9e8125d971c07534e78c3d07fbaf5920afa Mon Sep 17 00:00:00 2001 From: Bret Ambrose Date: Wed, 23 Aug 2023 11:47:22 -0700 Subject: [PATCH 1/2] Utf8 validation for topics, topic filters, username and client id --- source/client.c | 24 +++++-- source/mqtt.c | 4 ++ tests/CMakeLists.txt | 3 + .../{shared_utils.c => shared_utils_tests.c} | 0 tests/v3/connection_state_test.c | 71 ++++++++++++++++++- tests/v3/topic_tree_test.c | 4 ++ 6 files changed, 100 insertions(+), 6 deletions(-) rename tests/{shared_utils.c => shared_utils_tests.c} (100%) diff --git a/source/client.c b/source/client.c index f7df6208..2f6ed96d 100644 --- a/source/client.c +++ b/source/client.c @@ -942,6 +942,11 @@ 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, @@ -949,11 +954,6 @@ static int s_aws_mqtt_client_connection_311_set_will( (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); @@ -1007,6 +1007,11 @@ 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); @@ -1428,6 +1433,15 @@ 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 diff --git a/source/mqtt.c b/source/mqtt.c index 8edc3d23..ac8c4529 100644 --- a/source/mqtt.c +++ b/source/mqtt.c @@ -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; diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 0ee8847d..4b9ec529 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -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) diff --git a/tests/shared_utils.c b/tests/shared_utils_tests.c similarity index 100% rename from tests/shared_utils.c rename to tests/shared_utils_tests.c diff --git a/tests/v3/connection_state_test.c b/tests/v3/connection_state_test.c index dcc2cf7b..b5ca86ee 100644 --- a/tests/v3/connection_state_test.c +++ b/tests/v3/connection_state_test.c @@ -3868,4 +3868,73 @@ 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) \ No newline at end of file + &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) diff --git a/tests/v3/topic_tree_test.c b/tests/v3/topic_tree_test.c index ce512a7d..6e0bb853 100644 --- a/tests/v3/topic_tree_test.c +++ b/tests/v3/topic_tree_test.c @@ -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; } @@ -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; } From 6b8fc0300fdc0769c3768992d804e52a9da54a3d Mon Sep 17 00:00:00 2001 From: Bret Ambrose Date: Thu, 24 Aug 2023 14:51:00 -0700 Subject: [PATCH 2/2] Formatting? --- source/client.c | 6 ++++-- tests/v3/connection_state_test.c | 7 +++++-- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/source/client.c b/source/client.c index 2f6ed96d..e4385fab 100644 --- a/source/client.c +++ b/source/client.c @@ -1008,7 +1008,8 @@ static int s_aws_mqtt_client_connection_311_set_login( } 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); + 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); } @@ -1438,7 +1439,8 @@ static int s_aws_mqtt_client_connection_311_connect( } 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); + 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); } diff --git a/tests/v3/connection_state_test.c b/tests/v3/connection_state_test.c index b5ca86ee..9cfc6f13 100644 --- a/tests/v3/connection_state_test.c +++ b/tests/v3/connection_state_test.c @@ -3874,7 +3874,9 @@ static struct aws_byte_cursor s_bad_client_id_utf8 = AWS_BYTE_CUR_INIT_FROM_STRI 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) { +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; @@ -3906,7 +3908,8 @@ static int s_test_mqtt_validation_failure_invalid_will_topic_utf8_fn(struct aws_ 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)); + 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);