epee, common: pass unsigned char to <cctype> functions
What the developer wrote
Authored by pucedoteth
epee, common: pass unsigned char to <cctype> functions
The <cctype> functions require their argument to be representable as
unsigned char or to equal EOF. char is signed on the platforms Monero
targets, so every byte >= 0x80 reaches them negative, which is undefined
behaviour.
All six call sites take bytes that come from outside the process:
hex_to_dec_2bytes() percent-escapes in a payment URI
http_client.h the status line of an HTTP response
updates.cpp the hash field of a DNS update record
glibc and the macOS libc happen to tolerate -128..-1 because their tables
carry padding below zero, so this is quiet in practice on those, but it is
still out of contract and other libcs index without that cushion.
Cast to unsigned char at each call. The conversion is behaviour-preserving
for the ASCII these actually test: for a byte such as 0xC3, toupper()
returns it unchanged in the C locale, it narrows back to char, memchr()
misses it in the hex table, and hex_to_dec_2bytes() falls through to the
existing literal "%XY" output exactly as before.
epee::misc_utils::parse::isdigit/isalpha/isspace are separate char-taking
helpers and are already well defined, so they are untouched, as is
is_base64(), which already takes unsigned char.
What changed, and why it matters
This commit fixes a low-level programming mistake in how Monero handles bytes received from outside the program. Functions that classify characters (like checking if a byte is a space, blank, letter, or digit) were being given bytes that could be interpreted as negative numbers on the systems Monero runs on. That is officially undefined behavior, meaning the program could in theory read out of bounds, crash, or misbeave on some C library implementations. The fix casts those bytes to unsigned char before passing them to the classification functions. The commit message says the bug is quiet in practice on glibc and macOS, but could be a real problem on other C libraries.
Apply the patch. It is a small, safe correctness fix that removes undefined behavior on untrusted input bytes. No immediate incident response is indicated because the commit message states the issue is quiet on the commonly used glibc and macOS libcs, but leaving the UB in place creates latent risk on other platforms or future libc changes.
Security signals we found
Undefined behavior in <cctype> functions triggered by attacker-controlled bytes
Potential out-of-bounds table read in libc character-classification tables on non-glibc/macOS libcs
Network/input-derived bytes passed directly to isblank, isspace, isalnum, toupper
Fix is defensive/correctness rather than a confirmed exploitable vulnerability
Evidence from the diff
The patch addresses undefined behavior (UB) from passing signed char values (bytes >= 0x80) to
Changed components
contrib/epee/include/net/http_client.hcontrib/epee/src/abstract_http_client.cppsrc/common/updates.cppInspect captured patch +6 / −6
diff --git a/contrib/epee/include/net/http_client.h b/contrib/epee/include/net/http_client.h
index 6396bbe..6e49f5f 100644
--- a/contrib/epee/include/net/http_client.h
+++ b/contrib/epee/include/net/http_client.h
@@ -662,14 +662,14 @@ namespace net_utils
ptr = end + 1;
CHECK_AND_ASSERT_MES(epee::misc_utils::parse::isdigit(*ptr), false, "Invalid first response line: " + m_header_cache + ", ptr: " << ptr);
ul = strtoul(ptr, &end, 10);
- CHECK_AND_ASSERT_MES(ul <= INT_MAX && isblank(*end), false, "Invalid first response line: " + m_header_cache + ", ptr: " << ptr);
+ CHECK_AND_ASSERT_MES(ul <= INT_MAX && isblank(static_cast<unsigned char>(*end)), false, "Invalid first response line: " + m_header_cache + ", ptr: " << ptr);
m_response_info.m_http_ver_lo = ul;
ptr = end + 1;
- while (isblank(*ptr))
+ while (isblank(static_cast<unsigned char>(*ptr)))
++ptr;
CHECK_AND_ASSERT_MES(epee::misc_utils::parse::isdigit(*ptr), false, "Invalid first response line: " + m_header_cache);
ul = strtoul(ptr, &end, 10);
- CHECK_AND_ASSERT_MES(ul >= 100 && ul <= 999 && isspace(*end), false, "Invalid first response line: " + m_header_cache);
+ CHECK_AND_ASSERT_MES(ul >= 100 && ul <= 999 && isspace(static_cast<unsigned char>(*end)), false, "Invalid first response line: " + m_header_cache);
m_response_info.m_response_code = ul;
ptr = end;
// ignore the optional text, till the end
diff --git a/contrib/epee/src/abstract_http_client.cpp b/contrib/epee/src/abstract_http_client.cpp
index bd9cb0d..dd3763a 100644
--- a/contrib/epee/src/abstract_http_client.cpp
+++ b/contrib/epee/src/abstract_http_client.cpp
@@ -65,8 +65,8 @@ namespace net_utils
std::string hex_to_dec_2bytes(const char *s)
{
const char *hex = get_hex_vals();
- int i0 = get_index(hex, toupper(s[0]));
- int i1 = get_index(hex, toupper(s[1]));
+ int i0 = get_index(hex, toupper(static_cast<unsigned char>(s[0])));
+ int i1 = get_index(hex, toupper(static_cast<unsigned char>(s[1])));
if (i0 < 0 || i1 < 0)
return std::string("%") + std::string(1, s[0]) + std::string(1, s[1]);
return std::string(1, i0 * 16 | i1);
diff --git a/src/common/updates.cpp b/src/common/updates.cpp
index a8202b0..6c445ca 100644
--- a/src/common/updates.cpp
+++ b/src/common/updates.cpp
@@ -73,7 +73,7 @@ namespace tools
bool alnum = true;
for (auto c: fields[3])
- if (!isalnum(c))
+ if (!isalnum(static_cast<unsigned char>(c)))
alnum = false;
if (fields[3].size() != 64 && !alnum)
{
Why this scored 51/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.