Fix integer overflow check in subaddress handling (#264)
What the developer wrote
Authored by Lee *!* Clagett
Fix integer overflow check in subaddress handling (#264)
What changed, and why it matters
This commit fixes a math mistake when checking whether a user requested too many Monero subaddresses. The original code divided two numbers and compared the result to a maximum, which is the wrong way to detect overflow and could allow the multiplication to overflow. The fix reverses the division so the check works correctly. A malicious or malformed request might have been able to bypass the subaddress limit, though the practical effect depends on what happens after the check.
Apply the patch. After patching, audit any other locations that validate `major * minor` against `max_subaddresses` to ensure they all use the safe division pre-check. Consider adding unit tests that exercise boundary values such as `major = 0xFFFFFFFF`, `minor = 0xFFFFFFFF`, and values just below/above `max_subaddresses`.
Security signals we found
Integer overflow check corrected from an inverted/incorrect comparison to a canonical safe division pre-check
Occurs in subaddress limit enforcement, which is a security boundary against excessive address derivation
Same bug pattern present in two independent locations (storage and REST server)
No explicit CVE, advisory, or security disclosure supplied
Evidence from the diff
The patch corrects an integer overflow guard in two places. The original expression std::numeric_limits<std::uint32_t>::max() < major / minor is logically inverted and does not prevent major * minor from overflowing a 32-bit unsigned integer. The corrected expression std::numeric_limits<std::uint32_t>::max() / minor < major is the standard safe overflow test: if max / minor is less than major, then major * minor would exceed max. The same pattern is fixed in both src/db/storage.cpp and src/rest_server.cpp.
Changed components
src/db/storage.cppsrc/rest_server.cppInspect captured patch +2 / −2
diff --git a/src/db/storage.cpp b/src/db/storage.cpp
index 6358807..68286d2 100644
--- a/src/db/storage.cpp
+++ b/src/db/storage.cpp
@@ -2332,7 +2332,7 @@ namespace db
return success();
// Quick fail check
- if (std::numeric_limits<std::uint32_t>::max() < major / minor)
+ if (std::numeric_limits<std::uint32_t>::max() / minor < major)
return {error::max_subaddresses};
if (max_subaddresses < major * minor)
return {error::max_subaddresses};
diff --git a/src/rest_server.cpp b/src/rest_server.cpp
index 7bedd57..0a90990 100644
--- a/src/rest_server.cpp
+++ b/src/rest_server.cpp
@@ -233,7 +233,7 @@ namespace lws
if (minor)
{
const auto major = to_uint(lookahead.maj_i);
- if (std::numeric_limits<std::uint32_t>::max() < major / minor)
+ if (std::numeric_limits<std::uint32_t>::max() / minor < major)
return false;
return major * minor <= data.global->options.max_subaddresses;
}
Why this scored 59/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.