Skip to content

T8374: convert addresses to the host byte order before comparison in IPv4 range checks#21

Merged
jestabro merged 1 commit intovyos:currentfrom
dmbaturin:T8374-fix-range-check
Mar 11, 2026
Merged

T8374: convert addresses to the host byte order before comparison in IPv4 range checks#21
jestabro merged 1 commit intovyos:currentfrom
dmbaturin:T8374-fix-range-check

Conversation

@dmbaturin
Copy link
Member

@dmbaturin dmbaturin commented Mar 11, 2026

The original logic didn't account for the fact that in_addr->s_addr is stored in the network by order (big endian) and shouldn't be compared directly without byte order conversion on little-endian architectures.

That made certain checks like ipaddrcheck --verbose --is-ipv4-range 192.0.2.0-1.8.2.1 succeed, but the issue remained hidden because the comparison logic issue didn't affect more common bad ranges like 192.0.2.200-192.0.2.100.

In my defense, the POSIX docs for netinet/in.h doesn't say anything about the order and at the time I didn't realize the conversion happens much earlier than I thought.

Note that the IPv6 comparison logic is byte-by-byte from the start of the array, so it wasn't susceptible to that issue:

vyos_bld@75dc4f49e7e6:/vyos$ git diff
diff --git a/src/ipaddrcheck_functions.c b/src/ipaddrcheck_functions.c
index 09830a1..fa8351c 100644
--- a/src/ipaddrcheck_functions.c
+++ b/src/ipaddrcheck_functions.c
@@ -502,6 +502,7 @@ int compare_ipv6(struct in6_addr *left, struct in6_addr *right)
     int i = 0;
     for( i = 0; i < 16; i++ )
     {
+        printf("Comparing bytes %x and %x\n", left->s6_addr[i], right->s6_addr[i]);
         if (left->s6_addr[i] < right->s6_addr[i])
             return -1;
         else if (left->s6_addr[i] > right->s6_addr[i])

vyos_bld@75dc4f49e7e6:/vyos$ ./src/ipaddrcheck --is-ipv6-range aaaa:bbbb::1-aaaa:bbbb::10
Comparing bytes aa and aa
Comparing bytes aa and aa
Comparing bytes bb and bb
Comparing bytes bb and bb
Comparing bytes 0 and 0
Comparing bytes 0 and 0
Comparing bytes 0 and 0
Comparing bytes 0 and 0
Comparing bytes 0 and 0
Comparing bytes 0 and 0
Comparing bytes 0 and 0
Comparing bytes 0 and 0
Comparing bytes 0 and 0
Comparing bytes 0 and 0
Comparing bytes 0 and 0
Comparing bytes 1 and 10

Change Summary

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes)
  • Other (please describe):

Related Task(s)

Related PR(s)

Proposed changes

How to test

Checklist:

  • I have read the CONTRIBUTING document
  • I have linked this PR to one or more Phabricator Task(s)
  • My commit headlines contain a valid Task id
  • My change requires a change to the documentation
  • I have updated the documentation accordingly

when checking that the lower boundary of an IPv4 range
is actually lower than the upper bound
Copy link
Contributor

@jestabro jestabro left a comment

Choose a reason for hiding this comment

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

Fix obvious (in hindsight) bug; test added to confirm.

@jestabro jestabro merged commit b00cba1 into vyos:current Mar 11, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants