Page MenuHomePhabricator

[analyzer] sprintf is a taint propagator not a source
ClosedPublic

Authored by steakhal on Oct 26 2021, 10:06 AM.

Details

Summary

Due to a typo, sprintf() was recognized as a taint source instead of a
taint propagator. It was because an empty taint source list - which is
the first parameter of the TaintPropagationRule - encoded the
unconditional taint sources.
This typo effectively turned the sprintf() into an unconditional taint
source.

This patch fixes that typo and demonstrated the correct behavior with
tests.

Diff Detail

Event Timeline

steakhal created this revision.Oct 26 2021, 10:06 AM
steakhal requested review of this revision.Oct 26 2021, 10:06 AM

Probably it is a bit more correct but it can be better to remove the sprintf like functions from taint propagation. These functions return the number of bytes written to buf. This is in some extent dependent on the input parameters but maybe more no than yes. If there are fixed characters in the format string the return value is never zero only if error occurs. (But a more taint-likely case is if the %s format is used.)

martong accepted this revision.Oct 27 2021, 3:09 AM

Probably it is a bit more correct but it can be better to remove the sprintf like functions from taint propagation. These functions return the number of bytes written to buf. This is in some extent dependent on the input parameters but maybe more no than yes. If there are fixed characters in the format string the return value is never zero only if error occurs. (But a more taint-likely case is if the %s format is used.)

Yeah, it is worth thinking about it, but there are no decisive arguments beside each option. So, we could just keep as is.

clang/test/Analysis/taint-generic.c
345

C'mon, no covid please.
I know it's everywhere, but people die in this sh*t and it might offend any reader of this code who has been affected in anyway.

This revision is now accepted and ready to land.Oct 27 2021, 3:09 AM
steakhal updated this revision to Diff 382600.Oct 27 2021, 4:06 AM
steakhal marked an inline comment as done.

Probably it is a bit more correct but it can be better to remove the sprintf like functions from taint propagation. These functions return the number of bytes written to buf. This is in some extent dependent on the input parameters but maybe more no than yes. If there are fixed characters in the format string the return value is never zero only if error occurs. (But a more taint-likely case is if the %s format is used.)

Yeah, it is worth thinking about it, but there are no decisive arguments beside each option. So, we could just keep as is.

Okay, I see now. I think the "%d" format string is probably more reasonable. In that case, not sprintf() can actually return 0.

clang/test/Analysis/taint-generic.c
345

Okay.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 28 2021, 2:03 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript