Fix subnet mask computation in IpAddress::isWithinSubet - #800
Open
MarkRose wants to merge 1 commit into
Open
Conversation
IpAddress::isWithinSubet() computed the netmask from the CIDR prefix length using `atoi()` and `pow(2.0, 32 - prefix)`, with no validation that the prefix was a valid number in the 0-32 range: - A non-numeric prefix (e.g. "10.0.0.0/x") makes atoi() return 0, so pow(2.0, 32) overflows a uint32_t and the resulting mask collapses to 0. - An out-of-range prefix such as /33 or a negative value produces a negative exponent, which similarly yields a mask of 0 once cast to uint32_t (undefined behaviour on the out-of-range double-to-integer conversion). A mask of 0 makes isWithinSubet() match every address, silently turning an ALLOW_IP/subnet check into an "allow all" rule when the configured prefix is malformed. Replaced the computation with a small helper, parseNetmaskPrefix(), that parses the prefix with strtol(), rejects anything outside 0-32 or with trailing garbage, and builds the mask with a plain integer shift instead of floating point exponentiation. isWithinSubet() now returns false (no match) for an invalid prefix instead of failing open. Co-Authored-By: Claude Opus 4.8 <[email protected]>
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.
IpAddress::isWithinSubet() computed the netmask from the CIDR prefix
length using
atoi()andpow(2.0, 32 - prefix), with no validationthat the prefix was a valid number in the 0-32 range:
so pow(2.0, 32) overflows a uint32_t and the resulting mask
collapses to 0.
negative exponent, which similarly yields a mask of 0 once cast to
uint32_t (undefined behaviour on the out-of-range double-to-integer
conversion).
A mask of 0 makes isWithinSubet() match every address, silently
turning an ALLOW_IP/subnet check into an "allow all" rule when the
configured prefix is malformed.
Replaced the computation with a small helper, parseNetmaskPrefix(),
that parses the prefix with strtol(), rejects anything outside 0-32
or with trailing garbage, and builds the mask with a plain integer
shift instead of floating point exponentiation. isWithinSubet() now
returns false (no match) for an invalid prefix instead of failing
open.
Co-Authored-By: Claude Opus 4.8 [email protected]
This PR also adds a unit test (
IpAddressTest.cpp). It is auto-discovered and executed by the CTest suite proposed in #762 once that is merged; without that suite present the test file is inert and does not affect the build.