Skip to end of metadata
Go to start of metadata

Software vulnerabilities can result when a programmer fails to consider all possible data states.

Noncompliant Code Example (if Chain)

This noncompliant code example fails to test for conditions in which a is neither b nor c. This may be the correct behavior in this case, but failure to account for all the values of a can result in logic errors if a unexpectedly assumes a different value.

if (a == b) {
  /* ... */
}
else if (a == c) {
  /* ... */
}

Compliant Solution (if Chain)

This compliant solution explicitly checks for the unexpected condition and handles it appropriately:

if (a == b) {
  /* ... */
}
else if (a == c) {
  /* ... */
}
else {
  /* Handle error condition */
}

Noncompliant Code Example (switch)

Even though x is supposed to represent a bit (0 or 1) in this noncompliant code example, some previous error may have allowed x to assume a different value. Detecting and dealing with that inconsistent state sooner rather than later makes the error easier to find.

switch(x) {
  case 0: foo(); break;
  case 1: bar(); break;
}

Compliant Solution (switch)

This compliant solution provides the default label to handle all possible values of type int:

switch(x) {
  case 0: foo(); break;
  case 1: bar(); break;
  default: /* Handle error */ break;
} 

Noncompliant Code Example (Zune 30)

This noncompliant code example is adapted from C code that appeared in the Zune 30 media player, causing many players to lock up on December 30, 2008, at midnight PST. It contains incomplete logic that causes a denial of service when converting dates.

final static int ORIGIN_YEAR = 1980;
/* Number of days since January 1, 1980 */
public void convertDays(long days){  
	int year = ORIGIN_YEAR;
    /* ... */
    while (days > 365) {
    	if (IsLeapYear(year)) {
        	if (days > 366) {
            	days -= 366;
              	year += 1;
            }
        } else {
            days -= 365;
            year += 1;
        }
    }
}

The original ConvertDays() function in the real-time clock (RTC) routines for the MC13783 PMIC RTC takes the number of days since January 1, 1980, and computes the correct year and number of days since January 1 of the correct year.

The flaw in the code occurs when days has the value 366 because the loop never terminates. This bug manifested itself on the 366th day of 2008, which was the first leap year in which this code was active.

Compliant Solution (Zune 30)

This proposed rewrite is provided by "A Lesson on Infinite Loops" by Bryant Zadegan [Zadegan 2009]. The loop is guaranteed to exit, as days decreases for each iteration of the loop, unless the while condition fails, in which case the loop terminates.

final static int ORIGIN_YEAR = 1980;
/* Number of days since January 1, 1980 */
public void convertDays(long days){    
	int year = ORIGIN_YEAR;
    /* ... */
    int daysThisYear = (IsLeapYear(year) ? 366 : 365);
    while (days > daysThisYear) {
    	days -= daysThisYear;
        year += 1;
        daysThisYear = (IsLeapYear(year) ? 366 : 365);
    }
}

This compliant solution is for illustrative purposes and may differ from the solution implemented by Microsoft.

Applicability

Failing to take into account all possibilities within a logic statement can lead to a corrupted running state, potentially resulting in unintentional information disclosure or abnormal termination.

Related Guidelines

Bibliography

[Hatton 1995]§2.7.2, "Errors of Omission and Addition"
[Viega 2005]§5.2.17, "Failure to Account for Default Case in Switch"
[Zadegan 2009]A Lesson on Infinite Loops



7 Comments

  1. I think we need to -

    • mention that the code for the Zune 30 NCE has been ported to Java here
    • Instead of UINT32 days use a long primitive to hold the value
    • Compile the examples

    1. Done all three things.

  2. Concider the logical compliteness, colleagues of mine assert we should have an else clause for each if clause even for doing nothing.

    if (a == b) {
      /* ... */
    } else {
      /* Nothing to do */
    }
    They argue it is the proof we have thought of what to do (or not) if not (a==b).
    As for me, i am not agree. I think writing an else clause just for writing 'Nothing to do' is confusing and is less readable.
    The good way to do for explaining 'we have thought of what to do (or not) if not (a==b)', if necessary, is to write a simple commentary (without an else clause)
    if (a == b) { /* only condition */
      /* ... */
    }  /* Nothing to do for the other cases */

    1. First of all, this is a recommendation, not a rule. This means that it is possible to write secure code that violates this recommendation. Second, this is a style guideline, as it recommends adding trivial constructs that do not change the meaning of the code (such as empty else clauses).

      I tend to agree with you, that an if statement with no else-if statements does not need an empty else block. I would enforce an empty else block on an if statement with one or more else-if constructs. I would also enforce a 'default' case on switch statements (which are designed to handle multiple cases).

  3. Effectively, it is a style guideline.

     

    I agree with you ! It is necessary for one or more else-if statement and default on switch statements.

     

  4. We could reference MSC01-C here and vice versa.