Skip to end of metadata
Go to start of metadata

Redundant testing by caller and by callee as a style of defensive programming is largely discredited in the C and C++ communities, the main problem being performance. The usual discipline in C and C++ is to require validation on only one side of each interface.

Requiring the caller to validate arguments can result in faster code because the caller may understand certain invariants that prevent invalid values from being passed. Requiring the callee to validate arguments allows the validation code to be encapsulated in one location, reducing the size of the code and making it more likely that these checks are performed in a consistent and correct fashion.

For safety and security reasons, this standard recommends that the called function validate its parameters. Validity checks allow the function to survive at least some forms of improper usage, enabling an application using the function to likewise survive. Validity checks can also simplify the task of determining the condition that caused the invalid parameter.

Noncompliant Code Example

In this noncompliant code example, setfile() and usefile() do not validate their parameters. It is possible that an invalid file pointer can be used by the library, corrupting the library's internal state and exposing a vulnerability.

/* Sets some internal state in the library */
extern int setfile(FILE *file);

/* Performs some action using the file passed earlier */
extern int usefile();

static FILE *myFile;

void setfile(FILE *file) {
    myFile = file;
}

void usefile(void) {
    /* Perform some action here */
}

The vulnerability can be more severe if the internal state references sensitive or system-critical data.

Compliant Solution

Validating the function parameters and verifying the internal state leads to consistency of program execution and may eliminate potential vulnerabilities. In addition, implementing commit or rollback semantics (leaving program state unchanged on error) is a desirable practice for error safety.

/* Sets some internal state in the library */
extern errno_t setfile(FILE *file);

/* Performs some action using the file passed earlier */
extern errno_t usefile(void);

static FILE *myFile;

errno_t setfile(FILE *file) {
 if (file && !ferror(file) && !feof(file)) {
    myFile = file;
    return 0;
  }

  /* Error safety: leave myFile unchanged */
  return -1;
}

errno_t usefile(void) {
  if (!myFile) return -1;

    /*
     * Perform other checks if needed; return 
     * error condition.
     */

    /* Perform some action here */
    return 0;
}

Risk Assessment

Failing to validate the parameters in library functions may result in an access violation or a data integrity violation. Such a scenario indicates a flaw in how the library is used by the calling code. However, the library itself may still be the vector by which the calling code's vulnerability is exploited.

Recommendation

Severity

Likelihood

Remediation Cost

Priority

Level

API00-C

Medium

Unlikely

High

P2

L3

Automated Detection

Tool

Version

Checker

Description

CodeSonar4.4LANG.STRUCT.UPDUnchecked parameter dereference
Parasoft C/C++test9.5

CODSTA-86

 

Polyspace Bug FinderR2016a

Invalid use of standard library memory routine

Invalid use of standard library routine

Invalid use of standard library string routine

Standard function call with incorrect arguments

Tainted Data Defects

Standard library memory function called with invalid arguments

Wrong arguments to standard library function

Standard library string function called with invalid arguments

Argument to a standard function does not meet requirements for use in the function

Defects related to code elements from an unsecure source

Related Vulnerabilities

Search for vulnerabilities resulting from the violation of this rule on the CERT website.

Related Guidelines

Bibliography

 


8 Comments

  1. I think you should use "extern functions" instead of "callable from outside functions" to be consistent with standard terminology.

  2. I think setfile() should not always return 0; it should either be a void function or it should return an error indication (non-zero status) when it rejects the file.  Functions that always return zero are a hangover from the pre-void days of C.

  3. There are a couple of enforceable subrules trying to grow out of this rule. This rule is unenforceable because

    • how is ROSE to identify a library function?
    • What constitutes a valid parameter?
  4. A suggestion regarding the error safety of the compliant solution (not related to this rule itself). A good design rule to strive for in all software is transaction (commit/rollback) semantics: a function either succeeds to achieve its specified effects, or it fails and has no observable effect on program state. In addition, unless INVALID_ARG is defined in one of the secure C library extensions, using a well-known errno value such as EINVAL might be more informative (although lumping the three distinct error conditions under the same error value would likely be problematic in practice). With this in mind, setfile() would be better implemented as follows:

    errno_t setfile(FILE *file) {
     if (file && !ferror(file) && !feof(file)) {
        myFile = file;
        return 0;
      }
    
      /* no effects on error */
      return EINVAL;
    }
    
  5. Should the definition of setfile() in the NCE have the "const" qualifier?

    1. Probably not, I've removed it.

  6. In the "compliant" version of usefile myFile may be evaluated without prior initalization.

    Change the example code from

    static FILE *myFile;
    to
    static FILE *myFile = NULL;
    1. Static variables are always zero-initialized, so the assignment is not strictly required.  Specifically, 6.7.9p10:

      10 If an object that has automatic storage duration is not initialized explicitly, its value is indeterminate. If an object that has static or thread storage duration is not initialized explicitly, then:

      — if it has pointer type, it is initialized to a null pointer;

      — if it has arithmetic type, it is initialized to (positive or unsigned) zero;

      — if it is an aggregate, every member is initialized (recursively) according to these rules, and any padding is initialized to zero bits;

      — if it is a union, the first named member is initialized (recursively) according to these rules, and any padding is initialized to zero bits;