RB: simplify html_safe modeling

[erik-krogh]

CVE-2020-16254: Recognizes the sink.

The idea is that a .html_safe call is an XSS sink in any context.

An evaluation looks OK.
Performance is neutral, and there are some new results.
But could someone with more Ruby experience confirm that the new results look OK?

[hmac]

Some of the results for rails look a bit odd to me. I'm struggling to figure out where the sources are coming from in some cases. I would expect that, outside of test code, rails would not have any ORM sources, as it doesn't define any concrete ActiveRecord subclasses.

[alexrford]

I haven't looked into the rails/rails results, but a lot of the spurious results for rb/stored-xss on other projects here seem to be due to the ActiveRecordModelInstantiation#methodCallMayAccessField(string) predicate not checking if an attr_accessor/reader/writer is explicitly defined on the ActiveRecord model class. e.g. in the case of

class User < ApplicationRecord
  attr_reader :some_attribute, :some_other_attribute
...
end

I think we can usually assume that a call to user.some_attribute does not access a database field, but we currently assume the opposite.

[erik-krogh]

The spurious results (at least some of them) seemed to be from this additional-flow-step.
That caused us to find a lot of sources for rb/stored-xss.

The use of the methodCallMayAccessField predicate was also weird with that additional-flow-step, because the type of the ORM object could change when reading the nested fields.

[erik-krogh]

A new evaluation looks good.
Performance is great!
A few lost results from nested objects, but that seems OK to me.
We can try to get those back if (when?) we improve OrmTracking.

[hvitved]

We can try to get those back if (when?) we improve OrmTracking.

One possibility is to make a subsequent DataFlow::Configuration, which takes as sources any OrmFieldAsSource, sinks any call, and which does allow the additional node2.(DataFlow2::CallNode).getReceiver() = node1 flow step.

[alexrford]

I think that the html_safe related changes here make sense.

The changes to flow through OrmTracking also seem worthwhile for the large performance improvement. That being said, there is potential to lose legitimate results here. The results that we lose in DCA are (probably) FPs, in that they seem to access sanitized versions of fields. It's probably infeasible to actually verify that these fields are properly sanitized in practice - I guess we would have to check that there don't exist any writes to this field (anywhere in the program) where unsanitized data can flow to.

On a separate note, I'm working on a change to avoid considering attribute accesses as potential ActiveRecord field accesses which should reduce the FP rate for rb/stored-xss a bit. It looks like most of these FPs from an earlier evaluation have already gone as a result of the changes to OrmTracking in this PR.