[PATCH] D144269: [Analyzer] Show "taint originated here" note of alpha.security.taint.TaintPropagation checker at the correct place

dkrupp closed this revision. dkrupp added a comment. Committed in 343bdb10940cb2387c0b9bd3caccee7bb56c937b . CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144269/new/ https://reviews.llvm.org/D144269

[PATCH] D144269: [Analyzer] Show "taint originated here" note of alpha.security.taint.TaintPropagation checker at the correct place

steakhal accepted this revision. steakhal added a comment. This revision is now accepted and ready to land. LGTM. About formatting the tests: Personally, I would have preferred to "clean as you code", but I can see your point. Leave it as-is. Land it, please. CHANGES SINCE LAST ACTION

[PATCH] D144269: [Analyzer] Show "taint originated here" note of alpha.security.taint.TaintPropagation checker at the correct place

dkrupp added a comment. @steakhal thanks for the review. I fixed all outstanding remarks. I left the test taint-diagnostic-visitor.c formatting as is to remain consistent with the rest of the file. I think we should keep it as is, or reformat the whole file. Comment at:

[PATCH] D144269: [Analyzer] Show "taint originated here" note of alpha.security.taint.TaintPropagation checker at the correct place

dkrupp updated this revision to Diff 516389. dkrupp marked an inline comment as done. dkrupp added a comment. -using llvm::ArrayRef in the reportTaintBug(..) function in the DivZero Checker CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144269/new/ https://reviews.llvm.org/D144269

[PATCH] D144269: [Analyzer] Show "taint originated here" note of alpha.security.taint.TaintPropagation checker at the correct place

dkrupp updated this revision to Diff 516380. dkrupp marked 10 inline comments as done. dkrupp added a comment. -append_range(..) used instead of std::vector.insert(...) to improve readability -minor updates based on @steakhal comments CHANGES SINCE LAST ACTION

[PATCH] D144269: [Analyzer] Show "taint originated here" note of alpha.security.taint.TaintPropagation checker at the correct place

steakhal added a comment. To conclude the review, please respond to the "Not Done" inline comments, and mark them "Done" if you think they are resolved. Thank you for your patience. Comment at: clang/lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp:108-109 + if ((stateNotZero

[PATCH] D144269: [Analyzer] Show "taint originated here" note of alpha.security.taint.TaintPropagation checker at the correct place

dkrupp added a comment. @steakhal your comments are fixed. Thanks for the review. Comment at: clang/lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp:108-109 + if ((stateNotZero && stateZero)) { +std::vector taintedSyms = getTaintedSymbols(C.getState(), *DV); +if

[PATCH] D144269: [Analyzer] Show "taint originated here" note of alpha.security.taint.TaintPropagation checker at the correct place

dkrupp updated this revision to Diff 516077. dkrupp marked 13 inline comments as done. dkrupp added a comment. -getTaintedSymbols(.) -> getTaintedSymbolsImpl() proxy function introduced for interface safety -Other minor fixes based on comments from @steakhal CHANGES SINCE LAST ACTION

[PATCH] D144269: [Analyzer] Show "taint originated here" note of alpha.security.taint.TaintPropagation checker at the correct place

steakhal requested changes to this revision. steakhal added a comment. This revision now requires changes to proceed. I didn't go through the whole revision this time, but I think the next steps are already clear for the next round. My impression was that I might not expressed my intent and

[PATCH] D144269: [Analyzer] Show "taint originated here" note of alpha.security.taint.TaintPropagation checker at the correct place

dkrupp marked an inline comment as done. dkrupp added a comment. @steakhal is there anything else to do before we merge this? Thanks. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144269/new/ https://reviews.llvm.org/D144269 ___ cfe-commits

[PATCH] D144269: [Analyzer] Show "taint originated here" note of alpha.security.taint.TaintPropagation checker at the correct place

dkrupp marked an inline comment as done. dkrupp added a comment. @steakhal thanks for your review. All your remarks have been fixed. Comment at: clang/lib/StaticAnalyzer/Checkers/Taint.cpp:147-150 bool taint::isTainted(ProgramStateRef State, const Stmt *S,

[PATCH] D144269: [Analyzer] Show "taint originated here" note of alpha.security.taint.TaintPropagation checker at the correct place

dkrupp updated this revision to Diff 514973. dkrupp marked an inline comment as done. dkrupp added a comment. - Implemented early return in getTaintedSymbols() when it is called by isTainted() for efficiency - Fixed test incompatibility on Windows CHANGES SINCE LAST ACTION

[PATCH] D144269: [Analyzer] Show "taint originated here" note of alpha.security.taint.TaintPropagation checker at the correct place

steakhal requested changes to this revision. steakhal added a comment. This revision now requires changes to proceed. Nice improvement! I only have minor nitpicks and some recommendations for the taint API. Comment at:

[PATCH] D144269: [Analyzer] Show "taint originated here" note of alpha.security.taint.TaintPropagation checker at the correct place

dkrupp added a comment. You can find the improved reports on tmux, postgres, twin, openssl here: here

[PATCH] D144269: [Analyzer] Show "taint originated here" note of alpha.security.taint.TaintPropagation checker at the correct place

dkrupp added a comment. All remarks from @steakhal has been fixed. Thanks for the review. This new version now can handle the tracking back of multiple symbols! Comment at: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:129-130 /// Given a pointer/reference

[PATCH] D144269: [Analyzer] Show "taint originated here" note of alpha.security.taint.TaintPropagation checker at the correct place

dkrupp updated this revision to Diff 513556. dkrupp marked 11 inline comments as done. dkrupp edited the summary of this revision. dkrupp added a comment. -All remarks from @steakhal was fixed. Thanks for the review! -Now we can generate diagnostics for all tainted values when they reach a sink.

[PATCH] D144269: [Analyzer] Show "taint originated here" note of alpha.security.taint.TaintPropagation checker at the correct place

steakhal added a comment. I think as we converge, now it could be time to update the summary of the patch to reflect the current implementation. (e.g. flowids etc.) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144269/new/ https://reviews.llvm.org/D144269

[PATCH] D144269: [Analyzer] Show "taint originated here" note of alpha.security.taint.TaintPropagation checker at the correct place

steakhal added a comment. Looks even better. Only minor concerns remained, mostly about style and suggestions of llvm utilities. Comment at: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:129-130 /// Given a pointer/reference argument, return the value it refers

[PATCH] D144269: [Analyzer] Show "taint originated here" note of alpha.security.taint.TaintPropagation checker at the correct place

dkrupp added a comment. All comments addressed. Thanks for the review @steakhal . Comment at: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:162 + const CallEvent& Call) { + const LocationContext* LC = Call.getCalleeStackFrame(0); +

[PATCH] D144269: [Analyzer] Show "taint originated here" note of alpha.security.taint.TaintPropagation checker at the correct place

dkrupp updated this revision to Diff 511078. dkrupp marked 21 inline comments as done. dkrupp added a comment. @steakhal thanks for your review. I tried to address all your concerns. I added an extra test case too (multipleTaintSources(..)) which highlights the limitation of the current patch:

[PATCH] D144269: [Analyzer] Show "taint originated here" note of alpha.security.taint.TaintPropagation checker at the correct place

steakhal added a comment. In D144269#4237539 , @dkrupp wrote: > This is a totally rewritten version of the patch which solely relies on the > existing "interestingness" utility to track back the taint propagation. (And > does not introduce a new

[PATCH] D144269: [Analyzer] Show "taint originated here" note of alpha.security.taint.TaintPropagation checker at the correct place

dkrupp updated this revision to Diff 510108. dkrupp added a comment. This is a totally rewritten version of the patch which solely relies on the existing "interestingness" utility to track back the taint propagation. (And does not introduce a new FlowID in the ProgramState as requested in the

[PATCH] D144269: [Analyzer] Show "taint originated here" note of alpha.security.taint.TaintPropagation checker at the correct place

dkrupp planned changes to this revision. dkrupp added a comment. @steakhal , @NoQ thanks for the reviews. I will try to implement an alternative solution based on your suggestions. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144269/new/ https://reviews.llvm.org/D144269

[PATCH] D144269: [Analyzer] Show "taint originated here" note of alpha.security.taint.TaintPropagation checker at the correct place

steakhal added a comment. If we worry about having taint-related reports without a Note message explaining where the taint was introduced, we could just assert that in a `BugReportVisitor` at the `finalizeVisitor()` callback. I think such an assertion would make a lot of sense. To achieve

[PATCH] D144269: [Analyzer] Show "taint originated here" note of alpha.security.taint.TaintPropagation checker at the correct place

steakhal added a comment. First and foremost, I want to deeply apologize about my rushed response. When I say that the `Taint originated here` note remained, I **wrongly** draw conclusions. Won't happen again. __Taint-flow IDs__: I would challenge that we need a flow ID (number) because for me

[PATCH] D144269: [Analyzer] Show "taint originated here" note of alpha.security.taint.TaintPropagation checker at the correct place

dkrupp added a comment. > TaintBugReport is brilliant and we already have a precedent for subclassing > BugReport in another checker. However I'm somewhat worried that once we start > doing more of this, we'll eventually end up with multiple inheritance > situations when the report needs

[PATCH] D144269: [Analyzer] Show "taint originated here" note of alpha.security.taint.TaintPropagation checker at the correct place

NoQ added a comment. Yeah looks like I replied without properly reading the patch. `TaintBugReport` is brilliant and we already have a precedent for subclassing `BugReport` in another checker. However I'm somewhat worried that once we start doing more of this, we'll eventually end up with

[PATCH] D144269: [Analyzer] Show "taint originated here" note of alpha.security.taint.TaintPropagation checker at the correct place

Szelethus added a comment. In D144269#4143066 , @NoQ wrote: > The challenging part with note tags is how do you figure out whether your bug > report is taint-related. The traditional solution is to check the `BugType` > but in this case an

[PATCH] D144269: [Analyzer] Show "taint originated here" note of alpha.security.taint.TaintPropagation checker at the correct place

dkrupp added a comment. @steakhal, @NoQ thanks for your reviews. Please note that I am not extending `TaintBugVisitor`. On the contrary I removed it. Instead I use NoteTag to generate the "Taint Originated here" text (see GenericTaintChecker.cpp:156). I can also add additional NoteTags for

[PATCH] D144269: [Analyzer] Show "taint originated here" note of alpha.security.taint.TaintPropagation checker at the correct place

NoQ added a comment. I completely agree with @steakhal, these should be note tags: - The "visitor way" is to reverse-engineer the exploded graph after the fact. - The "slightly more sophisticated visitor way" is have checker callbacks leave extra hints in the graph to assist reverse

[PATCH] D144269: [Analyzer] Show "taint originated here" note of alpha.security.taint.TaintPropagation checker at the correct place

steakhal added a comment. I haven't checked the implementation, but fundamentally patching the TaintBugVisitor is not how we should improve the diagnostic for taint issues. I saw that this patch is not about NoteTags, so I didn't go any further that point. What we should do instead, to add a

[PATCH] D144269: [Analyzer] Show "taint originated here" note of alpha.security.taint.TaintPropagation checker at the correct place

dkrupp updated this revision to Diff 498795. dkrupp added a comment. Added documentation to the newly introduced types: TaintData, TaintBugReport. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144269/new/ https://reviews.llvm.org/D144269 Files:

[PATCH] D144269: [Analyzer] Show "taint originated here" note of alpha.security.taint.TaintPropagation checker at the correct place

dkrupp created this revision. dkrupp added a reviewer: Szelethus. dkrupp added a project: clang. Herald added subscribers: steakhal, manas, ASDenysPetrov, martong, gamesh411, donat.nagy, mikhail.ramalho, a.sidorin, JDevlieghere, rnkovacs, szepet, baloghadamsoftware, xazax.hun. Herald added a