CPP: Add query for detecteing incorrect error checking for scanf

[alexet]

This implements a new query for checking for a specific case of incorrect scanf use.

Specifically it looks for use of *scanf where the return value is checked against zero but not against -1. The scanf functions return -1 when reaching end of input before any of the placeholders. Only rejecting zero means that -1 is treated as a success but the results aren't set.

Known false positives:

• The input isn't attacker controlled E.g. Data from /proc or /sys or parsing /etc/passwd or simply data that is shipped with the program. Failure shouldn't be possible in these cases. However in these cases the user is still checking for an impossible error condition and ignoring the other.
• The input is checked beforehand (e.g. by regex, yacc, etc). Again here we have incomplete defensive programming where zero is handled but not -1.
• The input is somehow already checked far enough that failure with eof is not possible. E.g. jdk checks not empty then matches an unsigned integer). It is not possible to hit EOF before the actual parser is run. It's worth noting that was fixed in newer versions anyway.
• The outputs are set before calling sscanf. This means that there isn't use of an undetermined value but we do think it is likely to be set to something meaningful. I haven't seen any cases of this where it isn't a bug anyway.
• The outputs aren't read from sscanf. I haven't seen this.

The general FP rate is a bit high considering securty bugs but I think the fact that many of the security FPs are correctness issues anyway makes it good enough.

The results from this query are excluded from missing-scanf-check to avoid double reporting. The total number may go down as this query reports on the sscanf but that query reports on the use of the output.

[github-actions]

QHelp previews:

cpp/ql/src/Critical/IncorrectCheckScanf.qhelp

Incorrect return-value check for a 'scanf'-like function

This query finds calls of scanf-like functions with improper return-value checking. Specifically, it flags uses of scanf where the return value is only checked against zero.

Functions in the scanf family return either EOF (a negative value) in case of IO failure, or the number of items successfully read from the input. Consequently, a simple check that the return value is nonzero is not enough.

Recommendation

Ensure that all uses of scanf check the return value against the expected number of arguments rather than just against zero.

Example

The following examples show different ways of guarding a scanf output. In the BAD examples, the results are only checked against zero. In the GOOD examples, the results are checked against the expected number of matches instead.

{
  int i, j;

  // BAD: The result is only checked against zero
  if (scanf("%d %d", &i, &j)) { 
      use(i);
      use(j);
  }

  // BAD: The result is only checked against zero
  if (scanf("%d %d", &i, &j) == 0) { 
    i = 0;
    j = 0;
  }
  use(i);
  use(j);

  if (scanf("%d %d", &i, &j) == 2) { 
      // GOOD: the result is checked against 2
  }

  // GOOD: the result is compared directly
  int r = scanf("%d %d", &i, &j);
  if (r < 2) {
    return;
  }
  if (r == 1) { 
    j = 0;
  }
}

References

• SEI CERT C++ Coding Standard: ERR62-CPP. Detect errors when converting a string to a number.
• SEI CERT C Coding Standard: ERR33-C. Detect and handle standard library errors.
• cppreference.com: scanf, fscanf, sscanf, scanf_s, fscanf_s, sscanf_s.
• Common Weakness Enumeration: CWE-253.

[MathiasVP]

This looks really good! I only have a couple of small nits, but otherwise I think this looks good to go 🚀

[MathiasVP]

This LGTM! Let's wait for docs before merging this, though. I'll add the label now.

[subatoi]

Looks great, only really minor comments/suggestions, thank you!

[subatoi]

All looks great, thank you!