Scanning FreeRADIUS with PVS-Studio

The people at PVS-Studio were kind enough to give us a temporary license so that we could scan FreeRADIUS. We scanned the v3.0.x branch of the server, as of commit a7df94. Our commentary follows.

Commentary and Analysis

We will give our comments here, not necessarily in the order that they are in the file. We do not comment on all of the messages produced by PVS-Studio. Instead, we discuss a representative sample.

scripts/jlibtool.c 441 warn V547 Expression '!out' is always false.
scripts/jlibtool.c 1334 warn V547 Expression 'path' is always true.
scripts/jlibtool.c 1636 warn V522 There might be dereferencing of a potential null pointer 'tmp'.
scripts/jlibtool.c 2244 warn V519 The 'l' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 2241, 2244.

These errors are in a tool used as part of the build process. They should be fixed, but they don't affect the running server.

src/lib/cbuff.c 107 err V571 Recurring check. The 'if (cbuff->lock)' condition was already verified in line 107.
src/lib/cbuff.c 124 err V571 Recurring check. The 'if (cbuff->lock)' condition was already verified in line 124.
src/lib/cbuff.c 139 err V571 Recurring check. The 'if (cbuff->lock)' condition was already verified in line 139.
src/lib/cbuff.c 150 err V571 Recurring check. The 'if (cbuff->lock)' condition was already verified in line 150.

These are duplicate checks, which are unnecessary. Fixed by commit 51839ad.

src/lib/misc.c 1931 err V535 The variable 'i' is being used for this loop and for the outer loop. Check lines: 1922, 1931.

This is a bug which would cause incorrect parsing of date strings. Fixed by commit 384a9d3.

src/lib/missing.c 324 err V595 The 'ctx' pointer was utilized before it was verified against nullptr. Check lines: 324, 325.

This is failure to handle an "out of memory" condition. Minor, but it should be fixed. Fixed by commit 9a613ab.

src/lib/tcp.c 114 warn V512 A call of the 'memcpy' function will lead to underflow of the buffer 'packet->vector'.
src/modules/rlm_eap/libeap/comp128.c 252 warn V512 A call of the 'memcpy' function will lead to underflow of the buffer 'x'.
src/modules/rlm_eap/libeap/comp128.c 268 warn V512 A call of the 'memcpy' function will lead to underflow of the buffer 'x'.
src/modules/rlm_eap/libeap/eapsimlib.c 262 warn V512 A call of the 'memcpy' function will lead to underflow of the buffer 'sha1digest'.
src/modules/rlm_eap/types/rlm_eap_mschapv2/rlm_eap_mschapv2.c 156 err V575 The 'memcpy' function doesn't copy the whole string. Use 'strcpy / strcpy_s' function to preserve terminal null.
src/modules/rlm_eap/types/rlm_eap_sim/rlm_eap_sim.c 100 err V512 A call of the 'memcpy' function will lead to underflow of the buffer 'ess->keys.versionlist'.
src/modules/rlm_eap/libeap/eapsimlib.c 429 warn V512 A call of the 'memcmp' function will lead to underflow of the buffer 'calcmac'.
src/modules/rlm_otp/otp_pw_valid.c 151 warn V512 A call of the 'memcpy' function will lead to underflow of the buffer 'otp_request.pwe.u.chap.response'.
src/modules/rlm_otp/otp_pw_valid.c 171 warn V512 A call of the 'memcpy' function will lead to underflow of the buffer 'otp_request.pwe.u.chap.challenge'.
src/modules/rlm_mschap/rlm_mschap.c 1292 warn V512 A call of the 'memcpy' function will lead to underflow of the buffer 'digest'.
src/modules/rlm_mschap/rlm_mschap.c 2006 err V512 A call of the 'memset' function will lead to underflow of the buffer 'mppe_sendkey'.
src/modules/rlm_mschap/rlm_mschap.c 2008 warn V512 A call of the 'memcpy' function will lead to underflow of the buffer 'mppe_sendkey'.
src/modules/rlm_mschap/mschap.c 83 warn V512 A call of the 'memcpy' function will lead to underflow of the buffer 'hash'.

The pattern is that we're encoding or decoding packet data, by gradually filling a buffer. While this may be unusual in some programs, it's common in network servers. These messages are warnings, so they are not false positives.

src/main/exec.c 172 warn V507 Pointer to local array 'buffer' is stored outside the scope of this array. Such a pointer will become invalid.

The code has an on-stack array buffer which is used in a restricted context. It uses a variable p to index into buffer. The issue seems to be that p is declared with a larger scope than buffer, even tho it is not used outside of the scope of buffer.

The fix is to narrow the scope of the variable p. This should have no impact (or change) to the code produced by the compiler, but it does make the warning go away.

src/modules/rlm_eap/types/rlm_eap_pwd/rlm_eap_pwd.c 389 err V568 It's odd that 'sizeof()' operator evaluates the size of a pointer to a class, but not the size of the 'packet' class object.
src/modules/rlm_eap/types/rlm_eap_pwd/rlm_eap_pwd.c 390 err V568 It's odd that 'sizeof()' operator evaluates the size of a pointer to a class, but not the size of the 'packet' class object.

These are serious issues, and may be externally exploitable. They have been fixed in commit d7a006e.

src/lib/radius.c 3299 warn V575 The potential null pointer is passed into 'memcpy' function. Inspect the first argument.

That's a false positive. The code does:

  head = tail = malloc(fraglen);
	if (!head) return -1;
	...
	memcpy(tail, ..., ...)

If head is NULL, then the function returns, and the memcpy() function isn't called. Since the code sets head = tail, we can't have tail == NULL being passed to the memcpy() function.

We would suggest an update to PVS-Studio which omits the message for this situation.

src/main/radclient.c 1628 err V576 Incorrect format. Consider checking the second actual argument of the 'printf' function. The memsize type argument is expected.
src/main/radclient.c 1628 err V576 Incorrect format. Consider checking the third actual argument of the 'printf' function. The memsize type argument is expected.
src/main/radclient.c 1628 err V576 Incorrect format. Consider checking the fourth actual argument of the 'printf' function. The memsize type argument is expected.
src/main/radclient.c 1628 err V576 Incorrect format. Consider checking the fifth actual argument of the 'printf' function. The memsize type argument is expected.
src/main/radclient.c 1628 err V576 Incorrect format. Consider checking the sixth actual argument of the 'printf' function. The memsize type argument is expected.

The code is printing a uint64_t, via the PRIu64 macro that was defined in C99. No other compiler or static analysis tool complains about these arguments to printf. We believe that these messages are due to build issues on different platforms.

src/lib/udpfromto.c 227 warn V641 The size of the '& si' buffer is not a multiple of the element size of the type 'struct sockaddr_in6'.

The code is using a common network pattern where a struct sockaddr_storage is defined, and then a struct sockaddr_in6* pointer is assigned to point to struct sockaddr_storage structure. There should be no issues with this kind of pattern.

The rest of the issues found by PVS-Studio follow similar patterns, so we will not discuss them here. We have, however. fixed all of the issues so that we have a "clean" build with PVS-Studio.

Conclusions

We have found that PVS-Studio is useful, and finds errors that other statis analysis tools do not find. The false positive rate is manageable, with most messages being warnings instead of errors. We believe that it is good security practice to warn about unusual code patterns, so we have few issues with the messages being produced.

We plan to continue using PVS-Studio, in conjunction with the other static analysis tools we already use. Again, thanks to the PVS-Studio team for providing us with license and a copy of their software.