Skip to content
Permalink
Browse files

[HPHP Tainting] Cleanup

Summary:
This is just a bit of cleanup, but deserves maybe a bit of explanation:
- I removed the set/clear mask NO_TRACE system. Even after a thorough
  IDL audit, these don't seem to have any real use. We may someday want
  the ability to delay tracing in PHPland (which is somewhat like the
  set_mask functionality), but implementing this at the TaintObserver
  level would require some kind of mask inheritance, which is
  undesireable. Similarly, the TaintTracerSwitchGuard has functionality
  like clear_mask.
- I also removed the helper macro for TAINT_FLAG_HTML_CLEAN because it
  is rather taint-like and looks more consistent without the wrapper.
- Removed comments that are at this point covered in the README.
- Also fixed a trace message.

Test Plan: ran some requests in hphpi

Reviewers: srenfro, amenghra, mwilliams

Reviewed By: amenghra

CC: ps, mwilliams, amenghra

Differential Revision: 312566
  • Loading branch information...
mxw authored and scottmac committed Aug 26, 2011
1 parent b44280b commit 9d2b8ccffcdb42acba4bd2144e4c856fe80461e5
@@ -60,21 +60,15 @@ void TaintObserver::RegisterMutated(TaintData& td, const char *s) {
td.attachTaintTrace(NEW(TaintTraceData)(TaintTracer::Trace(s, true, true)));
}

if (TaintTracer::IsTraceEnabled(TAINT_BIT_TRACE_HTML)) {
ASSERT(!(TAINT_ISSET_HTML_NO_TRACE(set_mask) &&
TAINT_ISSET_HTML_NO_TRACE(clear_mask)));

// Propagate TRACE, kill TRACE, or perform tracing as desired.
if (TAINT_ISSET_HTML_NO_TRACE(set_mask)) {
set_mask = TAINT_GET_TAINT(set_mask);
} else if (TAINT_ISSET_HTML_NO_TRACE(clear_mask)) {
t &= ~TAINT_BIT_TRACE_HTML;
} else if ((t & TAINT_BIT_HTML) && (t & TAINT_BIT_TRACE_HTML) &&
TAINT_ISSET_HTML_CLEAN(tc->m_current_taint.getRawTaint())) {
t &= ~TAINT_BIT_TRACE_HTML;
TaintTraceDataPtr ttd = TaintTracer::CreateTrace();
tc->m_current_taint.attachTaintTrace(ttd);
}
bool do_trace = (t & TAINT_BIT_HTML) &&
(t & TAINT_BIT_TRACE_HTML) &&
(tc->m_current_taint.getRawTaint() & TAINT_FLAG_HTML_CLEAN);

// Perform HTML trace as desired.
if (TaintTracer::IsTraceEnabled(TAINT_BIT_TRACE_HTML) && do_trace) {
t &= ~TAINT_BIT_TRACE_HTML;
TaintTraceDataPtr ttd = TaintTracer::CreateTrace();
tc->m_current_taint.attachTaintTrace(ttd);
}

// Propagate the taint and any trace data.
@@ -30,30 +30,7 @@
* in the stack. You can therefore not make calls such as new TaintObserver().
*
* While adding convenience, the TaintObserver system has some downsides,
* which boil down to two main gotchas:
*
* (1) SCOPE. Since TaintObservers form a stack, they are inherently
* sensitive to scope. By default, no TaintObserver is declared, so in the
* general case, even if strings are tainted, their taints will not propagate
* to other strings (but will maintain state). Meanwhile, TaintObservers never
* leave scope unless replaced or until their resident frame is destroyed.
* Thus, stray TaintObservers in places that are anomalous w.r.t. scope, such
* as the call_user_func*() family, can wreak havoc.
*
* This issue of scoping also means that there is no single bottlenecked
* location where we can handle taint semantics. Any callflow relevant to
* taint ultimately drops into a String constructor, which unquestioningly
* accepts the taint of whichever TaintObserver is in scope. Hence, though
* this system lets us do less specialized casework, the individual placement
* of TaintObserver declarations still demands attention.
*
* (2) LOCALNESS. TaintObservers are rather coarse-grained because of their
* very local understanding of taint; the only events they recognize are
* access and creation, and hence they cannot ever make a semantic distinction
* between strings whose taints matter and strings whose taints don't. This,
* however, will at worst generate some false positives or demand manual labor
* for certain taint tasks which require more global knowledge, like HTML
* taint tracing.
* which are explained further in the README.
*/

// This flag is marked in the taints of $_GET, $_POST, and relevant $_COOKIE
@@ -62,24 +39,12 @@
#define TAINT_FLAG_ORIG (0x80000000)
#define TAINT_ISSET_ORIG(bits) ((bits) & TAINT_FLAG_ORIG)

// This flag turns off tracing in the scope of a TaintObserver; it is only
// meant to be used in a TaintObserver's m_set_mask and m_clear_mask fields.
// Normally, trace data is generated when an HTML-tainted, TRACE-tainted
// string encounters an HTML-untainted string, and the TRACE bit is not
// propagated. When this flag is set in set_mask, we PROPAGATE any TRACE bits
// instead of tracing. When this flag is set in clear_mask, we IGNORE all
// TRACE bits and do not propagate. If it is set in both, something is
// horribly, horribly wrong.
#define TAINT_FLAG_HTML_NO_TRACE (0x40000000)
#define TAINT_ISSET_HTML_NO_TRACE(bits) ((bits) & TAINT_FLAG_HTML_NO_TRACE)

// This flag is set in a TaintObserver's m_current_taint when it encounters
// an HTML-untainted string. It allows us to determine when an HTML-tainted
// string first makes concact with a clean string. If both this flag and the
// string first makes contact with a clean string. If both this flag and the
// HTML bit itself are set at the time of String creation (i.e., mutation),
// then we capture trace data.
#define TAINT_FLAG_HTML_CLEAN (0x20000000)
#define TAINT_ISSET_HTML_CLEAN(bits) ((bits) & TAINT_FLAG_HTML_CLEAN)
#define TAINT_FLAG_HTML_CLEAN (0x40000000)

namespace HPHP {

@@ -118,8 +118,8 @@ std::string TaintTracer::ExtractTrace(const TaintTraceNodePtr& root) {
ss << "source strings fragments:\n";
if (sourceset.empty()) {
ss << " (none)\n";
ss << "NB: Source strings are not kept for file reads and are dropped";
ss << "when passed into the APC.\n";
ss << "NB: Source strings are not kept for file reads; they are also ";
ss << "dropped when passed into the APC.\n";
}
for (i = 0, it = sourceset.begin(); it != sourceset.end(); ++i, ++it) {
ss << " #" << i << " ";

0 comments on commit 9d2b8cc

Please sign in to comment.
You can’t perform that action at this time.