fix(modem): Harden AT library against command injection#1020
Open
david-cermak wants to merge 1 commit intoespressif:masterfrom
Open
fix(modem): Harden AT library against command injection#1020david-cermak wants to merge 1 commit intoespressif:masterfrom
david-cermak wants to merge 1 commit intoespressif:masterfrom
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
AT-001: Undefined Behavior on Empty Token in generic_get_string [FIXED]
Severity: Medium | esp_modem_command_library.cpp:92
A malicious modem sending \n\n as a response creates an empty string_view token. The original code computed token.end() - 1 on an empty view — undefined behavior. A conforming compiler may optimize away the loop guard under UB assumptions.
Fix applied: Replaced the reverse-iterator loop with while (!token.empty() && (token.back() == '\r' || token.back() == '\n')).
AT-003: Wrong substr Length in Quote Stripping [FIXED]
Severity: Medium | esp_modem_command_library.cpp:266
A malicious modem sending +COPS: 1,0,X"Attacker",7 causes get_operator_name to extract extra characters from the response. operator_name.substr(quote1 + 1, quote2 - 1) uses quote2 - 1 as count, which is only correct when quote1 == 0. When the modem places characters before the first quote, the extracted name includes trailing data.
Fix applied: Changed to substr(quote1 + 1, quote2 - quote1 - 1) with a quote2 > quote1 guard.
AT-004: Missing Comma in get_network_system_mode Returns Wrong Value [FIXED]
Severity: Medium | esp_modem_command_library.cpp:573-578
A malicious modem sending +CNSMOD: 7 (no comma) causes int mode_pos = out.find(",") + 1 to yield mode_pos = 0 (since npos + 1 = 0). from_chars then parses from position 0, and if the modem crafts the line to start with a digit (e.g., by including it in a previous URC), an attacker-controlled value is returned.
Fix applied: Added explicit comma-not-found check; return FAIL if no comma.
AT-005: from_chars Out-of-Range Not Checked [FIXED]
Severity: Medium | Multiple locations
A malicious modem sending very large numeric values (e.g., +CSQ: 99999999999999999,3) triggers std::errc::result_out_of_range, but only errc::invalid_argument was checked. The output parameter is left unmodified per the C++17 spec — returning whatever stale/uninitialized value was in the caller's variable as if parsing succeeded.
Fix applied: Changed all from_chars error checks from == std::errc::invalid_argument to != std::errc{} to catch all error codes.
AT-006: Inverted from_chars Range on Truncated Battery Response [FIXED]
Severity: Medium | esp_modem_command_library.cpp:230
A malicious modem sending +CBC: 3. (truncated after the dot) causes from_chars(out.data() + dot_pos + 1, out.data() + out.size() - 1, ...) to receive an inverted range (begin > end) — undefined behavior.
Fix applied: Added range validation (dot_pos + 2 > out.size() → return FAIL) before the from_chars call.