Skip to content

[lts_03_2025] Use safe_add_size_t when sizing DPS transport response buffers (backport of #2738) #2746

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

Open
Ewerton Scaboro da Silva (ewertons) wants to merge 2 commits into
base: lts_03_2025
Choose a base branch
from
Open

[lts_03_2025] Use safe_add_size_t when sizing DPS transport response buffers (backport of #2738) #2746

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fb3c7c6
Use safe_add_size_t when sizing DPS transport response buffers (#2738)
ewertons Jul 23, 2026
a61e236
Adapt backported tests to the umock-c pinned by lts_03_2025
ewertons Aug 24, 2026
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
12 changes: 10 additions & 2 deletions provisioning_client/src/prov_transport_amqp_common.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include "azure_c_shared_utility/http_proxy_io.h"
#include "azure_c_shared_utility/strings.h"
#include "azure_c_shared_utility/azure_base64.h"
#include "azure_c_shared_utility/safe_math.h"

#include "azure_uamqp_c/message_sender.h"
#include "azure_uamqp_c/message_receiver.h"
Expand Down Expand Up @@ -455,6 +456,7 @@ static AMQP_VALUE on_message_recv_callback(const void* user_ctx, MESSAGE_HANDLE
else
{
BINARY_DATA binary_data;
size_t alloc_size;
if (amqp_info->payload_data != NULL)
{
free(amqp_info->payload_data);
Expand All @@ -467,7 +469,13 @@ static AMQP_VALUE on_message_recv_callback(const void* user_ctx, MESSAGE_HANDLE
amqp_info->transport_state = TRANSPORT_CLIENT_STATE_ERROR;
amqp_info->amqp_state = AMQP_STATE_ERROR;
}
else if ((amqp_info->payload_data = malloc(binary_data.length + 1)) == NULL)
else if ((alloc_size = safe_add_size_t(binary_data.length, 1)) == SIZE_MAX)
{
LogError("failure invalid payload length");
amqp_info->transport_state = TRANSPORT_CLIENT_STATE_ERROR;
amqp_info->amqp_state = AMQP_STATE_ERROR;
}
else if ((amqp_info->payload_data = malloc(alloc_size)) == NULL)
{
LogError("failure allocating payload data");
amqp_info->transport_state = TRANSPORT_CLIENT_STATE_ERROR;
Expand All @@ -479,7 +487,7 @@ static AMQP_VALUE on_message_recv_callback(const void* user_ctx, MESSAGE_HANDLE
// be set to the default value
(void)get_retry_after_property(amqp_info, message);

memset(amqp_info->payload_data, 0, binary_data.length + 1);
memset(amqp_info->payload_data, 0, alloc_size);
memcpy(amqp_info->payload_data, binary_data.bytes, binary_data.length);
if (amqp_info->transport_state == TRANSPORT_CLIENT_STATE_REG_SENT)
{
Expand Down
15 changes: 11 additions & 4 deletions provisioning_client/src/prov_transport_http_client.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include "azure_c_shared_utility/http_proxy_io.h"
#include "azure_c_shared_utility/urlencode.h"
#include "azure_c_shared_utility/azure_base64.h"
#include "azure_c_shared_utility/safe_math.h"

#include "azure_prov_client/prov_transport_http_client.h"
#include "azure_prov_client/internal/prov_transport_private.h"
Expand Down Expand Up @@ -138,19 +139,25 @@ static void on_http_reply_recv(void* callback_ctx, HTTP_CALLBACK_REASON request_
{
if (content != NULL && content_len > 0)
{
size_t alloc_size = safe_add_size_t(content_len, 1);
if (http_info->payload_data != NULL)
{
free(http_info->payload_data);
http_info->payload_data = NULL;
}
http_info->payload_data = malloc(content_len + 1);
if (http_info->payload_data == NULL)
if (alloc_size == SIZE_MAX)
{
LogError("Failure sending http request");
LogError("Failure invalid content length specified");

@ewertons Ewerton Scaboro da Silva (ewertons) Aug 25, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed on the substance; not changing it in this PR.

You are right that the value is not caller-specified. content_len is the length reported by the receive callback — in azure-uhttp-c it is BUFFER_length(recv_msg.msg_body), i.e. bytes actually received and buffered, not the Content-Length header. That distinction is exactly why this is hardening rather than a remotely reachable overflow, so the wording is worth correcting.

The reason not to do it here: this string is byte-identical to main, and "the three provisioning_client/src files are byte-identical to main at the allocation sites" is the main safety argument for this backport. Changing it would make the branches diverge and leave the same wording wrong on main.

Better fixed on main and allowed to flow down. Happy to open that PR.

http_info->transport_state = TRANSPORT_CLIENT_STATE_ERROR;
}
else if ((http_info->payload_data = malloc(alloc_size)) == NULL)
{
LogError("Failure allocating payload data");
http_info->transport_state = TRANSPORT_CLIENT_STATE_ERROR;
}
else
{
memset(http_info->payload_data, 0, content_len + 1);
memset(http_info->payload_data, 0, alloc_size);
memcpy(http_info->payload_data, content, content_len);
if (http_info->transport_state == TRANSPORT_CLIENT_STATE_REG_SENT)
{
Expand Down
13 changes: 10 additions & 3 deletions provisioning_client/src/prov_transport_mqtt_common.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
#include "azure_c_shared_utility/shared_util_options.h"
#include "azure_c_shared_utility/http_proxy_io.h"
#include "azure_c_shared_utility/urlencode.h"
#include "azure_c_shared_utility/http_proxy_io.h"
#include "azure_c_shared_utility/safe_math.h"

#include "azure_prov_client/internal/prov_transport_mqtt_common.h"
#include "azure_umqtt_c/mqtt_client.h"
Expand Down Expand Up @@ -344,21 +344,28 @@ static MQTT_CLIENT_ACK_OPTION mqtt_notification_callback(MQTT_MESSAGE_HANDLE han
const APP_PAYLOAD* payload = mqttmessage_getApplicationMsg(handle);
if (payload != NULL)
{
size_t alloc_size = safe_add_size_t(payload->length, 1);
if (mqtt_info->payload_data != NULL)
{
free(mqtt_info->payload_data);
mqtt_info->payload_data = NULL;
}

if ((mqtt_info->payload_data = malloc(payload->length + 1)) == NULL)
if (alloc_size == SIZE_MAX)
{
LogError("failure invalid payload length");
mqtt_info->transport_state = TRANSPORT_CLIENT_STATE_ERROR;
mqtt_info->mqtt_state = MQTT_STATE_ERROR;
}
else if ((mqtt_info->payload_data = malloc(alloc_size)) == NULL)
{
LogError("failure allocating payload data");
mqtt_info->transport_state = TRANSPORT_CLIENT_STATE_ERROR;
mqtt_info->mqtt_state = MQTT_STATE_ERROR;
}
else
{
memset(mqtt_info->payload_data, 0, payload->length + 1);
memset(mqtt_info->payload_data, 0, alloc_size);
memcpy(mqtt_info->payload_data, payload->message, payload->length);
if (mqtt_info->transport_state == TRANSPORT_CLIENT_STATE_REG_SENT)
{
Expand Down
41 changes: 40 additions & 1 deletion provisioning_client/tests/prov_transport_amqp_common_ut/prov_transport_amqp_common_ut.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,7 @@ static ON_MESSAGE_RECEIVED g_on_msg_recv;
static void* msg_recv_callback_context;
static TPM_CHALLENGE_CALLBACK g_challenge_cb;
static void* g_challenge_context;
static bool g_use_invalid_msg_length;
static bool g_use_x509;

static PROV_DEVICE_TRANSPORT_STATUS g_target_transport_status;
Expand Down Expand Up @@ -210,7 +211,7 @@ static int my_message_get_body_amqp_data_in_place(MESSAGE_HANDLE message, size_t
{
(void)message;
(void)index;
amqp_data->length = strlen(TEST_JSON_REPLY);
amqp_data->length = g_use_invalid_msg_length ? (size_t)-1 : strlen(TEST_JSON_REPLY);
amqp_data->bytes = (unsigned char*)TEST_JSON_REPLY;
return 0;
}
Expand Down Expand Up @@ -701,6 +702,7 @@ BEGIN_TEST_SUITE(prov_transport_amqp_common_ut)
g_challenge_cb = NULL;
g_challenge_context = NULL;
g_use_x509 = false;
g_use_invalid_msg_length = false;
}

TEST_FUNCTION_CLEANUP(method_cleanup)
Expand Down Expand Up @@ -1758,6 +1760,43 @@ BEGIN_TEST_SUITE(prov_transport_amqp_common_ut)
prov_transport_common_amqp_destroy(handle);
}

TEST_FUNCTION(prov_transport_common_amqp_dowork_register_recv_invalid_length_fail)
{
AMQP_VALUE result;
PROV_DEVICE_TRANSPORT_HANDLE handle = prov_transport_common_amqp_create(TEST_URI_VALUE, TRANSPORT_HSM_TYPE_TPM, TEST_SCOPE_ID_VALUE, TEST_DPS_API_VALUE, on_transport_io, on_transport_error, NULL);
(void)prov_transport_common_amqp_open(handle, TEST_REGISTRATION_ID_VALUE, TEST_BUFFER_VALUE, TEST_BUFFER_VALUE, on_transport_register_data_cb, NULL, on_transport_status_cb, NULL, on_transport_challenge_callback, NULL);
(void)prov_transport_common_amqp_register_device(handle, on_transport_json_parse, on_transport_create_json_payload, NULL);
prov_transport_common_amqp_dowork(handle);
g_msg_sndr_state_changed(g_msg_sndr_state_changed_ctx, MESSAGE_SENDER_STATE_OPEN, MESSAGE_SENDER_STATE_OPENING);
g_msg_rcvr_state_changed(g_msg_rcvr_state_changed_ctx, MESSAGE_RECEIVER_STATE_OPEN, MESSAGE_RECEIVER_STATE_OPENING);
prov_transport_common_amqp_dowork(handle);
umock_c_reset_all_calls();

// A wire-provided body length of SIZE_MAX would wrap (length + 1) to 0; the transport
// must reject it instead of allocating a zero-length buffer and copying past it.
g_use_invalid_msg_length = true;

//arrange
STRICT_EXPECTED_CALL(message_get_body_type(IGNORED_PTR_ARG, IGNORED_PTR_ARG));
STRICT_EXPECTED_CALL(message_get_body_amqp_data_in_place(IGNORED_PTR_ARG, 0, IGNORED_PTR_ARG));
STRICT_EXPECTED_CALL(messaging_delivery_accepted());
STRICT_EXPECTED_CALL(connection_dowork(IGNORED_PTR_ARG));
STRICT_EXPECTED_CALL(on_transport_register_data_cb(PROV_DEVICE_TRANSPORT_RESULT_ERROR, NULL, NULL, NULL, IGNORED_PTR_ARG));

//act
result = g_on_msg_recv(msg_recv_callback_context, TEST_MESSAGE_HANDLE);
prov_transport_common_amqp_dowork(handle);

//assert
ASSERT_IS_NOT_NULL(result);
ASSERT_ARE_EQUAL(char_ptr, umock_c_get_expected_calls(), umock_c_get_actual_calls());

//cleanup
g_use_invalid_msg_length = false;
prov_transport_common_amqp_close(handle);
prov_transport_common_amqp_destroy(handle);
}

TEST_FUNCTION(prov_transport_common_amqp_dowork_register_recv_no_retry_after_property_succeed)
{
AMQP_VALUE result;
Expand Down
25 changes: 25 additions & 0 deletions provisioning_client/tests/prov_transport_http_client_ut/prov_transport_http_client_ut.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1090,6 +1090,31 @@ BEGIN_TEST_SUITE(prov_transport_http_client_ut)
prov_dev_http_transport_destroy(handle);
}

TEST_FUNCTION(prov_transport_http_reply_recv_invalid_content_len_fail)
{
//arrange
PROV_DEVICE_TRANSPORT_HANDLE handle = prov_dev_http_transport_create(TEST_URI_VALUE, TRANSPORT_HSM_TYPE_TPM, TEST_SCOPE_ID_VALUE, TEST_DPS_API_VALUE, on_transport_error, NULL);
(void)prov_dev_http_transport_open(handle, TEST_REGISTRATION_ID_VALUE, TEST_BUFFER_VALUE, TEST_BUFFER_VALUE, on_transport_register_data_cb, NULL, on_transport_status_cb, NULL, on_transport_challenge_callback, NULL);
(void)prov_dev_http_transport_register_device(handle, on_transport_json_parse, on_transport_create_json_payload, NULL);
g_on_http_open(g_http_open_ctx, HTTP_CALLBACK_REASON_OK);
prov_dev_http_transport_dowork(handle);
umock_c_reset_all_calls();

// A Content-Length of SIZE_MAX would wrap (content_len + 1) to 0; the transport must
// reject it instead of allocating a zero-length buffer and copying gigabytes past it.
Comment on lines +1103 to +1104

@ewertons Ewerton Scaboro da Silva (ewertons) Aug 25, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Same conclusion as the log-message thread, and the comment is genuinely misleading.

The test drives g_on_http_reply_recv(...) directly with content_len = (size_t)-1, so it exercises the callback parameter, not header parsing. Referring to "a Content-Length of SIZE_MAX" implies a malicious header reaches this code path unchanged, which is not what happens — uhttp errors out when the received body length does not match, so content_len is bytes actually buffered.

Not changing it here only to keep this backport a clean cherry-pick of main; the comment is identical on main. Worth correcting there.

For the record, the test itself is sound — I verified it fails against the unfixed source: reverting the three src files to malloc(len + 1) makes all three suites abort with heap corruption, and restoring the fix returns 196/196 green.

STRICT_EXPECTED_CALL(HTTPHeaders_FindHeaderValue(IGNORED_PTR_ARG, IGNORED_PTR_ARG)).SetReturn(NULL);

//act
g_on_http_reply_recv(g_http_execute_ctx, HTTP_CALLBACK_REASON_OK, (const unsigned char*)TEST_JSON_CONTENT, (size_t)-1, TEST_SUCCESS_STATUS_CODE, TEST_HTTP_HANDLE_VALUE);

//assert
ASSERT_ARE_EQUAL(char_ptr, umock_c_get_expected_calls(), umock_c_get_actual_calls());

//cleanup
(void)prov_dev_http_transport_close(handle);
prov_dev_http_transport_destroy(handle);
}

TEST_FUNCTION(prov_transport_http_reply_recv_transient_error_succeed)
{
//arrange
Expand Down
47 changes: 46 additions & 1 deletion provisioning_client/tests/prov_transport_mqtt_common_ut/prov_transport_mqtt_common_ut.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ static ON_MQTT_ERROR_CALLBACK g_on_error_cb;
static void* g_on_error_ctx;
static bool g_use_x509;
static APP_PAYLOAD g_app_msg;
static bool g_use_invalid_msg_length;


static PROV_DEVICE_TRANSPORT_STATUS g_target_transport_status;
Expand Down Expand Up @@ -197,7 +198,7 @@ static void my_mqttmessage_destroy(MQTT_MESSAGE_HANDLE handle)
static const APP_PAYLOAD* my_mqttmessage_getApplicationMsg(MQTT_MESSAGE_HANDLE handle)
{
(void)handle;
g_app_msg.length = strlen(TEST_JSON_REPLY);
g_app_msg.length = g_use_invalid_msg_length ? (size_t)-1 : strlen(TEST_JSON_REPLY);
g_app_msg.message = (unsigned char*)TEST_JSON_REPLY;
return &g_app_msg;
}
Expand Down Expand Up @@ -399,6 +400,7 @@ BEGIN_TEST_SUITE(prov_transport_mqtt_common_ut)
g_operation_cb = NULL;
g_on_error_cb = NULL;
g_use_x509 = false;
g_use_invalid_msg_length = false;
}

TEST_FUNCTION_CLEANUP(method_cleanup)
Expand Down Expand Up @@ -1329,6 +1331,49 @@ BEGIN_TEST_SUITE(prov_transport_mqtt_common_ut)
prov_transport_common_mqtt_destroy(handle);
}

TEST_FUNCTION(prov_transport_common_mqtt_dowork_register_recv_invalid_length_fail)
{
CONNECT_ACK connack = { true, CONNECTION_ACCEPTED };
QOS_VALUE QosValue[] = { DELIVER_AT_LEAST_ONCE };
SUBSCRIBE_ACK suback;
suback.packetId = 1234;
suback.qosCount = 1;
suback.qosReturn = QosValue;

PROV_DEVICE_TRANSPORT_HANDLE handle = prov_transport_common_mqtt_create(TEST_URI_VALUE, TRANSPORT_HSM_TYPE_X509, TEST_SCOPE_ID_VALUE, TEST_DPS_API_VALUE, on_mqtt_transport_io, on_transport_error, NULL);
(void)prov_transport_common_mqtt_x509_cert(handle, TEST_X509_CERT_VALUE, TEST_PRIVATE_KEY_VALUE);
(void)prov_transport_common_mqtt_open(handle, TEST_REGISTRATION_ID_VALUE, NULL, NULL, on_transport_register_data_cb, NULL, on_transport_status_cb, NULL, on_transport_challenge_callback, NULL);
(void)prov_transport_common_mqtt_register_device(handle, on_transport_json_parse, on_transport_create_json_payload, NULL);
prov_transport_common_mqtt_dowork(handle);
g_operation_cb(TEST_MQTT_CLIENT_HANDLE, MQTT_CLIENT_ON_CONNACK, &connack, g_msg_recv_callback_context);
prov_transport_common_mqtt_dowork(handle);
g_operation_cb(TEST_MQTT_CLIENT_HANDLE, MQTT_CLIENT_ON_SUBSCRIBE_ACK, &suback, g_msg_recv_callback_context);
prov_transport_common_mqtt_dowork(handle);
umock_c_reset_all_calls();

// A wire-provided payload length of SIZE_MAX would wrap (length + 1) to 0; the
// transport must reject it instead of allocating a zero-length buffer and copying past it.
g_use_invalid_msg_length = true;

//arrange
STRICT_EXPECTED_CALL(mqttmessage_getTopicName(IGNORED_PTR_ARG));
STRICT_EXPECTED_CALL(mqttmessage_getApplicationMsg(IGNORED_PTR_ARG));
STRICT_EXPECTED_CALL(mqtt_client_dowork(IGNORED_PTR_ARG));
STRICT_EXPECTED_CALL(on_transport_register_data_cb(PROV_DEVICE_TRANSPORT_RESULT_ERROR, NULL, NULL, NULL, IGNORED_PTR_ARG));

//act
g_on_msg_recv(TEST_MQTT_MESSAGE, g_msg_recv_callback_context);
prov_transport_common_mqtt_dowork(handle);

//assert
ASSERT_ARE_EQUAL(char_ptr, umock_c_get_expected_calls(), umock_c_get_actual_calls());

//cleanup
g_use_invalid_msg_length = false;
prov_transport_common_mqtt_close(handle);
prov_transport_common_mqtt_destroy(handle);
}

TEST_FUNCTION(prov_transport_common_mqtt_dowork_register_recv_transient_error_succeed)
{
CONNECT_ACK connack = { true, CONNECTION_ACCEPTED };
Expand Down
Loading