Page MenuHomePhabricator

[analyzer] Fix sensitive argument logic in GenericTaintChecker
AbandonedPublic

Authored by gamesh411 on Nov 29 2021, 6:18 AM.

Details

Summary

The semantics of taint sinks is that if ANY of the arguments is tainted, a
warning should be emmitted. Before this change, if there were multiple
arguments that are sensitive, and if the first arg is not tainted, but any of
the noninitial are tainted, a warning is not emitted. After this change the
correct semantics is reflected in code.

Diff Detail

Event Timeline

gamesh411 created this revision.Nov 29 2021, 6:18 AM
gamesh411 requested review of this revision.Nov 29 2021, 6:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 29 2021, 6:18 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
steakhal requested changes to this revision.Nov 29 2021, 10:50 AM

E.g. execl() and execlp functions are actually variadic. You should also account for them.
I would rather map directly to a full-fledged Propagation rule instead of to a sensitive arg index list. That way you could express this property.
Demonstrate this in a test.

Actually, the CallDescriptionFlags::CDF_MaybeBuiltin CallDescriptions will use the CCtx::isCLibraryFunction() for matching the call.
By using that the code would get significantly shorter and more readable as well.

clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
951–957

These algorithms are not supposed to have side-effects. Even though we technically could have side-effects, I would still apply it separately.
That being said, this seems to be a copy-paste version of the previous hunk, like 3rd time.

This revision now requires changes to proceed.Nov 29 2021, 10:50 AM

The semantics of taint sinks is that if ANY of the arguments is tainted, a
warning should be emmitted. Before this change, if there were multiple
arguments that are sensitive, and if the first arg is not tainted, but any of
the noninitial are tainted, a warning is not emitted. After this change the
correct semantics is reflected in code.

It worked well with custom sinks, didn't it? So, as I see, this patch is about system calls and subscript indices. Could you please update the description to have this information?

clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
951–957

That being said, this seems to be a copy-paste version of the previous hunk, like 3rd time.

+1, to put it into a named function

gamesh411 abandoned this revision.Mon, Jan 17, 1:13 AM

This is superseded by D116025.