Page MenuHomeVyOS Platform

Supress sprintf format warnings in ipaddrcheck at the source level
Closed, ResolvedPublic

Description

ipaddrcheck uses sprintf to assemble IP addresses with prefix lengths from components of address ranges to pass them to libcidr's functions. Such use of sprintf for arbitrary strings would be unsafe due to the risk of overflows.

In ipaddrcheck, that risk is mitigated by checking the input with a regex, which guarantees that they don't exceed certain length. We also build it with -Werror, and that use of sprintf still raises a GCC warning — so that warning needs to be suppressed. As of now, it's suppressed in CFLAGS. However, a) CFLAGS may be overridden during the build process and b) it's a bad idea to suppress that warning for all code, not just for places where it's verified to be safe.

It does, in fact, lead to build errors in Debian package build that may not occur when running make all by hand:

make[2]: Entering directory '/vyos/ipaddrcheck/tests'
make  check_ipaddrcheck
make[3]: Entering directory '/vyos/ipaddrcheck/tests'
gcc -DHAVE_CONFIG_H -I. -I../src   -Wdate-time -D_FORTIFY_SOURCE=2 -pthread  -g -O2 -ffile-prefix-map=/vyos/ipaddrcheck=. -fstack-protector-strong -Wformat -Werror=format-security -c -o check_ipaddrcheck-check_ipaddrcheck.o `test -f 'check_ipaddrcheck.c' || echo './'`check_ipaddrcheck.c
gcc -DHAVE_CONFIG_H -I. -I../src   -Wdate-time -D_FORTIFY_SOURCE=2 -pthread  -g -O2 -ffile-prefix-map=/vyos/ipaddrcheck=. -fstack-protector-strong -Wformat -Werror=format-security -c -o ../src/check_ipaddrcheck-ipaddrcheck_functions.o `test -f '../src/ipaddrcheck_functions.c' || echo './'`../src/ipaddrcheck_functions.c
../src/ipaddrcheck_functions.c: In function ‘is_ipv4_range’:
../src/ipaddrcheck_functions.c:584:48: warning: ‘%u’ directive writing between 1 and 10 bytes into a region of size between 3 and 18 [-Wformat-overflow=]
  584 |                     sprintf(left_pref_str, "%s/%u", left, prefix_length);
      |                                                ^~
../src/ipaddrcheck_functions.c:584:44: note: directive argument in the range [1, 2147483647]
  584 |                     sprintf(left_pref_str, "%s/%u", left, prefix_length);
      |                                            ^~~~~~~
In file included from /usr/include/stdio.h:906,
                 from ../src/ipaddrcheck_functions.h:26,
                 from ../src/ipaddrcheck_functions.c:26:
In function ‘sprintf’,
    inlined from ‘is_ipv4_range’ at ../src/ipaddrcheck_functions.c:584:21:
/usr/include/x86_64-linux-gnu/bits/stdio2.h:30:10: note: ‘__builtin___sprintf_chk’ output between 3 and 27 bytes into a destination of size 19
   30 |   return __builtin___sprintf_chk (__s, __USE_FORTIFY_LEVEL - 1,
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   31 |                                   __glibc_objsize (__s), __fmt,
      |                                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   32 |                                   __va_arg_pack ());
      |                                   ~~~~~~~~~~~~~~~~~
../src/ipaddrcheck_functions.c: In function ‘is_ipv6_range’:
../src/ipaddrcheck_functions.c:685:48: warning: ‘%u’ directive writing between 1 and 10 bytes into a region of size between 4 and 43 [-Wformat-overflow=]
  685 |                     sprintf(left_pref_str, "%s/%u", left, prefix_length);
      |                                                ^~
../src/ipaddrcheck_functions.c:685:44: note: directive argument in the range [1, 2147483647]
  685 |                     sprintf(left_pref_str, "%s/%u", left, prefix_length);
      |                                            ^~~~~~~
In function ‘sprintf’,
    inlined from ‘is_ipv6_range’ at ../src/ipaddrcheck_functions.c:685:21:
/usr/include/x86_64-linux-gnu/bits/stdio2.h:30:10: note: ‘__builtin___sprintf_chk’ output between 3 and 51 bytes into a destination of size 44
   30 |   return __builtin___sprintf_chk (__s, __USE_FORTIFY_LEVEL - 1,
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   31 |                                   __glibc_objsize (__s), __fmt,
      |                                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   32 |                                   __va_arg_pack ());
      |                                   ~~~~~~~~~~~~~~~~~
gcc -pthread  -g -O2 -ffile-prefix-map=/vyos/ipaddrcheck=. -fstack-protector-strong -Wformat -Werror=format-security  -Wl,-z,relro -o check_ipaddrcheck check_ipaddrcheck-check_ipaddrcheck.o ../src/check_ipaddrcheck-ipaddrcheck_functions.o -lcidr -lpcre -lcheck_pic -lrt -lm -lsubunit  
make[3]: Leaving directory '/vyos/ipaddrcheck/tests'
make  check-TESTS
make[3]: Entering directory '/vyos/ipaddrcheck/tests'
make[4]: Entering directory '/vyos/ipaddrcheck/tests'
PASS: check_ipaddrcheck
FAIL: integration_tests.sh
===========================================
   ipaddrcheck 1.1: tests/test-suite.log
===========================================

# TOTAL: 2
# PASS:  1
# SKIP:  0
# XFAIL: 0
# FAIL:  1
# XPASS: 0
# ERROR: 0

A better solution is to disable that warning in the source file for specific lines — both more robust for build purposes and safer if someone inserts an unsafe use of sprintf in the future.

Details

Version
-
Is it a breaking change?
Perfectly compatible
Issue type
Internal change (not visible to end users)

Revisions and Commits

Event Timeline

dmbaturin triaged this task as Normal priority.
Restricted Repository Identity closed this task as Resolved by committing Restricted Diffusion Commit.Feb 11 2025, 5:13 PM
Restricted Repository Identity added a commit: Restricted Diffusion Commit.