Page MenuHomePhabricator

[analyzer] CERT STR rule checkers: STR50-CPP
Needs RevisionPublic

Authored by Charusso on May 24 2020, 9:45 PM.

Details

Summary

This patch introduces a new experimental checker:
alpha.security.cert.str.50cpp

This checker is implemented based on the following rule:
https://wiki.sei.cmu.edu/confluence/x/i3w-BQ

It warns on misusing std::cin and std::basic_istream<T>::read() to
create a std::string which may not null-terminated.

Diff Detail

Event Timeline

Charusso created this revision.May 24 2020, 9:45 PM
baloghadamsoftware requested changes to this revision.Jun 26 2020, 6:09 AM

Why limiting the checker to std::cin?

Anyway, this checker is far from ready yet (the essence is missing), see my comment inside.

clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp
658

Hmm, let us imagine that I write the following code:

TypeT cin;
AnyClassWithInstanceGTGTOperatorTakingTypeT >> cin;

Your code will probably crash when trying to retrieve arg 1 because you neither check for std::cin nor for InstanceCall.

669

Do we need such heavyweight thing here? In my experience, AST matchers are expensive to create.

673

Here should be the check for the size of the buffer and the comparison to cin.width(). It seems that it is not ready yet. This should be the most essential part of this checker.

676

std::cin does not write. It is just a stream from where we read. operator>> writes. Please rephrase the error message.

clang/test/Analysis/cert/str50-cpp.cpp
24

We should get warning for bufTwo and no warning for bufOne. operator>> reads cin.width()-1 characters to the buffer plus a null-terminator, which is exactly 12 bytes. So no overflow happens. However, it also resets the width, so the next read is unlimited. Please check STR50-CPP. It describes exactly the same as above.

This revision now requires changes to proceed.Jun 26 2020, 6:09 AM
whisperity added inline comments.Jun 26 2020, 6:25 AM
clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp
671

IIRC the matches are of SmallVector which should have an empty() method, prefer it to size() <op> 0.