Skip to content

[modem]: Harden the lib after security audit #1014

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
david-cermak wants to merge 3 commits into
base: master
Choose a base branch
from
Open

[modem]: Harden the lib after security audit #1014

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e3759f4
fix(modem): Harden AT library against command injection
david-cermak Feb 23, 2026
c937881
fix(modem): Fix URC handling: unbouded size, stale last urc
david-cermak Feb 23, 2026
0df8c0b
fix(modem): Fix netif destructor UAF
david-cermak Feb 23, 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
46 changes: 26 additions & 20 deletions components/esp_modem/src/esp_modem_command_library.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* SPDX-FileCopyrightText: 2021-2025 Espressif Systems (Shanghai) CO LTD
* SPDX-FileCopyrightText: 2021-2026 Espressif Systems (Shanghai) CO LTD
*
* SPDX-License-Identifier: Apache-2.0
*/
Expand Down Expand Up @@ -89,10 +89,9 @@ template <typename T> command_result generic_get_string(CommandableIf *t, const
std::string_view response((char *)data, len);
while ((pos = response.find('\n')) != std::string::npos) {
std::string_view token = response.substr(0, pos);
for (auto it = token.end() - 1; it > token.begin(); it--) // strip trailing CR or LF
if (*it == '\r' || *it == '\n') {
token.remove_suffix(1);
}
while (!token.empty() && (token.back() == '\r' || token.back() == '\n')) {
token.remove_suffix(1);
}
ESP_LOGV(TAG, "Token: {%.*s}\n", static_cast<int>(token.size()), token.data());

if (token.find("OK") != std::string::npos) {
Expand Down Expand Up @@ -187,7 +186,7 @@ command_result get_battery_status(CommandableIf *t, int &voltage, int &bcs, int
out = out.substr(pattern.size());
int pos, value, property = 0;
while ((pos = out.find(',')) != std::string::npos) {
if (std::from_chars(out.data(), out.data() + pos, value).ec == std::errc::invalid_argument) {
if (std::from_chars(out.data(), out.data() + pos, value).ec != std::errc{}) {
return command_result::FAIL;
}
switch (property++) {
Expand All @@ -200,7 +199,7 @@ command_result get_battery_status(CommandableIf *t, int &voltage, int &bcs, int
}
out = out.substr(pos + 1);
}
if (std::from_chars(out.data(), out.data() + out.size(), voltage).ec == std::errc::invalid_argument) {
if (std::from_chars(out.data(), out.data() + out.size(), voltage).ec != std::errc{}) {
return command_result::FAIL;
}
return command_result::OK;
Expand All @@ -224,10 +223,13 @@ command_result get_battery_status_sim7xxx(CommandableIf *t, int &voltage, int &b
}

int volt, fraction;
if (std::from_chars(out.data() + num_pos, out.data() + dot_pos, volt).ec == std::errc::invalid_argument) {
if (std::from_chars(out.data() + num_pos, out.data() + dot_pos, volt).ec != std::errc{}) {
return command_result::FAIL;
}
if (dot_pos + 2 > out.size()) {
return command_result::FAIL;
}
if (std::from_chars(out.data() + dot_pos + 1, out.data() + out.size() - 1, fraction).ec == std::errc::invalid_argument) {
if (std::from_chars(out.data() + dot_pos + 1, out.data() + out.size() - 1, fraction).ec != std::errc{}) {
return command_result::FAIL;
}
bcl = bcs = -1; // not available for these models
Expand Down Expand Up @@ -256,14 +258,14 @@ command_result get_operator_name(CommandableIf *t, std::string &operator_name, i
if (property++ == 2) { // operator name is after second comma (as a 3rd property of COPS string)
operator_name = out.substr(++pos);
auto additional_comma = operator_name.find(','); // check for the optional ACT
if (additional_comma != std::string::npos && std::from_chars(operator_name.data() + additional_comma + 1, operator_name.data() + operator_name.length(), act).ec != std::errc::invalid_argument) {
if (additional_comma != std::string::npos && std::from_chars(operator_name.data() + additional_comma + 1, operator_name.data() + operator_name.length(), act).ec == std::errc{}) {
operator_name = operator_name.substr(0, additional_comma);
}
// and strip quotes if present
auto quote1 = operator_name.find('"');
auto quote2 = operator_name.rfind('"');
if (quote1 != std::string::npos && quote2 != std::string::npos) {
operator_name = operator_name.substr(quote1 + 1, quote2 - 1);
if (quote1 != std::string::npos && quote2 != std::string::npos && quote2 > quote1) {
operator_name = operator_name.substr(quote1 + 1, quote2 - quote1 - 1);
}
return command_result::OK;
}
Expand Down Expand Up @@ -447,10 +449,10 @@ command_result get_signal_quality(CommandableIf *t, int &rssi, int &ber)
return command_result::FAIL;
}

if (std::from_chars(out.data() + rssi_pos, out.data() + ber_pos, rssi).ec == std::errc::invalid_argument) {
if (std::from_chars(out.data() + rssi_pos, out.data() + ber_pos, rssi).ec != std::errc{}) {
return command_result::FAIL;
}
if (std::from_chars(out.data() + ber_pos + 1, out.data() + out.size(), ber).ec == std::errc::invalid_argument) {
if (std::from_chars(out.data() + ber_pos + 1, out.data() + out.size(), ber).ec != std::errc{}) {
return command_result::FAIL;
}
return command_result::OK;
Expand Down Expand Up @@ -482,7 +484,7 @@ command_result get_network_attachment_state(CommandableIf *t, int &state)
return command_result::FAIL;
}

if (std::from_chars(out.data() + pos, out.data() + out.size(), state).ec == std::errc::invalid_argument) {
if (std::from_chars(out.data() + pos, out.data() + out.size(), state).ec != std::errc{}) {
return command_result::FAIL;
}

Expand All @@ -509,7 +511,7 @@ command_result get_radio_state(CommandableIf *t, int &state)
return command_result::FAIL;
}

if (std::from_chars(out.data() + pos, out.data() + out.size(), state).ec == std::errc::invalid_argument) {
if (std::from_chars(out.data() + pos, out.data() + out.size(), state).ec != std::errc{}) {
return command_result::FAIL;
}

Expand Down Expand Up @@ -570,12 +572,16 @@ command_result get_network_system_mode(CommandableIf *t, int &mode)
}

constexpr std::string_view pattern = "+CNSMOD: ";
int mode_pos = out.find(",") + 1; // Skip "<n>,"
if (out.find(pattern) == std::string::npos) {
return command_result::FAIL;
}
auto comma = out.find(',');
if (comma == std::string::npos) {
return command_result::FAIL;
}
size_t mode_pos = comma + 1;

if (std::from_chars(out.data() + mode_pos, out.data() + out.size(), mode).ec == std::errc::invalid_argument) {
if (std::from_chars(out.data() + mode_pos, out.data() + out.size(), mode).ec != std::errc{}) {
return command_result::FAIL;
}

Expand All @@ -602,7 +608,7 @@ command_result get_gnss_power_mode(CommandableIf *t, int &mode)
return command_result::FAIL;
}

if (std::from_chars(out.data() + pos, out.data() + out.size(), mode).ec == std::errc::invalid_argument) {
if (std::from_chars(out.data() + pos, out.data() + out.size(), mode).ec != std::errc{}) {
return command_result::FAIL;
}

Expand Down Expand Up @@ -668,7 +674,7 @@ command_result get_network_registration_state(CommandableIf *t, int &state)
}

// Extract state value (skip the comma)
if (std::from_chars(out.data() + state_pos_start + 1, out.data() + state_pos_end, state).ec == std::errc::invalid_argument) {
if (std::from_chars(out.data() + state_pos_start + 1, out.data() + state_pos_end, state).ec != std::errc{}) {
return command_result::FAIL;
}

Expand Down
8 changes: 7 additions & 1 deletion components/esp_modem/src/esp_modem_dte.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* SPDX-FileCopyrightText: 2021-2025 Espressif Systems (Shanghai) CO LTD
* SPDX-FileCopyrightText: 2021-2026 Espressif Systems (Shanghai) CO LTD
*
* SPDX-License-Identifier: Apache-2.0
*/
Expand Down Expand Up @@ -166,6 +166,7 @@ command_result DTE::command(const std::string &command, got_line_cb got_line, ui
#ifdef CONFIG_ESP_MODEM_URC_HANDLER
// Track command end
buffer_state.command_waiting = false;
buffer_state.last_urc_processed = 0;

@cursor cursor Bot Feb 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

URC state reset races with UART task

High Severity

DTE::command() now writes buffer_state.last_urc_processed = 0 without taking command_cb.line_lock, while the terminal read callback reads/writes buffer_state.* under line_lock. This introduces a cross-thread data race on buffer_state.last_urc_processed (and related fields), which is undefined behavior and can corrupt URC parsing state.

Additional Locations (1)

Fix in Cursor Fix in Web

#endif
buffer.consumed = 0;
#ifdef CONFIG_ESP_MODEM_USE_INFLATABLE_BUFFER_IF_NEEDED
Expand Down Expand Up @@ -398,6 +399,11 @@ bool DTE::command_cb::process_line(uint8_t *data, size_t consumed, size_t len, D

case UrcConsumeResult::CONSUME_PARTIAL:
// Consume only specified amount
if (consume_info.consume_size > consumed + len) {
ESP_LOGW("esp_modem_dte", "URC consume_size %zu exceeds buffer %zu, treating as CONSUME_NONE",
(size_t)consume_info.consume_size, (size_t)(consumed + len));
break;
}
dte->buffer_state.last_urc_processed += consume_info.consume_size;
// Adjust data pointers for command processing
data += consume_info.consume_size;
Expand Down
1 change: 1 addition & 0 deletions components/esp_modem/src/esp_modem_netif.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ void Netif::pause()

Netif::~Netif()
{
ppp_dte->set_read_cb(nullptr);
if (signal.is_any(PPP_STARTED)) {
esp_netif_action_stop(driver.base.netif, nullptr, 0, nullptr);
signal.clear(PPP_STARTED);
Expand Down
Loading