Protobuf modelling

[smowton]

This isn't quite done yet, but it's close enough that I'd appreciate feedback on the trickier points here.

Problems to overcome:

• Desire to taint the output of Marshal based on transitive field / array-element writes (e.g. msg.SubMessage.RepeatedMessage[2].Description = somethingNasty())
• Desire to propagate taint properly across Merge and Clone, which should copy taint from corresponding field to field
• Desire to support MarshalOptions.MarshalState, which takes its message from and returns its buffer in a struct field

Current approach:

• The read-side of MarshalOptions.MarshalState precisely models the field the taint is consumed from. The write-side simply taints the whole output, since it only has one field, and relies on the default taint rule that object taint implies field-read taint.
• Messages are tainted as a whole, which simplifies Merge and Clone operations but complicates the additional steps needed to convey taint from a field-write to the underlying SSA variable (I extend SsaWithFields to navigate the access path consisting of field-accesses and element-reads, but this might not be appropriate and we might need a separate mechanism for this). This also could produce false-positives by spreading taint to untainted fields (and of course array elements, but this is more ordinary); whether this is a practical problem depends on whether people like to use their protobuf structs as working data structures, or if they simply populate them immediately before serialisation (i.e., if the fields are in practice write-only)

Possible changes:

• Given I gave up on field-sensitive modelling of Marshal for the time being, we could in the same spirit throw away the complicated MarshalOptions.MarshalState stuff and simply propagate taint from a Message to a MarshalInput struct.
• Extending SsaWithFields as I have done might be inappropriate because its similar() method will currently conflate access paths using differing indices. In that case we need a different way to walk down an access path consisting of field-read and element-read operations to taint the bottom Message in the pile.
• If any of this is too contentious I could back off some of today's experiments and simply PR the easy partial support and follow up with either custom hackery as seen here, or with a more general mechanism to state a function model that deals in access paths rather than simply direct inputs and outputs.

[smowton]

I added further tests including ones exhibiting inaccuracy of the current implementation.

TODO:

• Clean up the test directory, which contains many redundant copies of the protoc output and some debugging queries that don't need to be in the final PR
• Add support for UnmarshalState, MergeState?
• Change note

[smowton]

I also note a search of Github suggests this is now the only user of MarshalOptions.MarshalState that exists (the API was only introduced earlier this year)

[smowton]

TODOs done apart from the change-note, which I'll write last so as not to forget to update it.

I gave up on modelling UnmarshalOptions.UnmarshalState, as that would need to propagate taint like

func testUnmarshalOptions() {
	options := proto.UnmarshalOptions{}

	untrustedSerialized := getUntrustedBytes()
	query := &query.Query{}

	state := protoiface.UnmarshalInput {
		Message: query.ProtoReflect(),
		Buf: untrustedSerialized,
		Flags: 0,
		Resolver: nil,
	}

	options.UnmarshalState(state)
	
	sinkString(query.Description) // BAD
}

That would need us to work backwards from the input struct to its field, and then from .ProtoReflect() call to the query it actually refers to. Ultimately doable but painful.

[owen-mc]

I also note a search of Github suggests this is now the only user of MarshalOptions.MarshalState that exists (the API was only introduced earlier this year)

But note that github code search is pretty unreliable.

[max-schaefer]

Partial review with a few simple suggestions; will continue later.

[max-schaefer]

Suggested change

	exists(SsaWithFieldsAndElements base | this = TFieldStep(base, _))
	exists(SsaWithFields base | this = TFieldStep(base, _))

[max-schaefer]

Suggested change

	* Additional taint-flow step modelling flow from MarshalInput.Message to MarshalOutput,
	* Additional taint-flow step modelling flow from `MarshalInput.Message` to `MarshalOutput`,

[max-schaefer]

Suggested change

	* mediated by a MarshalOptions.MarshalState call.
	* mediated by a `MarshalOptions.MarshalState` call.

[max-schaefer]

Suggested change

	* Note we can taint the whole MarshalOutput as it only has one field (Buf), and taint-
	* Note we can taint the whole `MarshalOutput` as it only has one field (`Buf`), and taint-

[sauyon]

In case you didn't know, you can click the + for comments and drag to make a multi-line comment or suggestion.

[max-schaefer]

Suggested change

	passedMarshalInput.asExpr().getGlobalValueNumber() =
	marshalInput.asExpr().getGlobalValueNumber() and
	globalValueNumber(passedMarshalInput) = globalValueNumber(marshalInput) and

Alternatively, you can replace the single use of passedMarshalInput with globalValueNumber(marshalInput).getANode().

[max-schaefer]

Suggested change

	succ.(DataFlow::PostUpdateNode).getPreUpdateNode().asInstruction() =
	accessPath.getBaseVariable().getAUse()
	succ = DataFlow::ssaNode(accessPath.getBaseVariable())

[max-schaefer]

Desire to propagate taint properly across Merge and Clone, which should copy taint from corresponding field to field

I seem to vaguely remember that we don't do field-sensitive propagation through taint, only through plain data flow. In other words, if we know that there is data flow between x and y, then we will conclude that the same is true for x.f and y.f. If, on the other hand, all we know is that taint is propagated from x to y, we don't do the same. Have you tried promoting your models of Merge and Clone from TaintTracking::FunctionModels to DataFlow::FunctionModels to see if that helps?

[smowton]

Distcompare result was unremarkable

[sauyon]

My rather belated review. (Sorry!)

[sauyon]

Seems like this comment was out of date, can you fix it?

[sauyon]

Does this not also include maps?

[smowton]

No, I forgot they were a first-class type. Looks like the Go implementation does indeed produce maps too -- I'll add this.

[sauyon]

Would it make sense to use GVNs here?

[smowton]

You mean as an alternative to IR::Instruction as the representation of the index? That does sound better, I was just worried that because GVN analysis is based on the SSA representation this would invert the pass order.

[sauyon]

I think this should also deal with slices and maps, as well as maybe strings.

[smowton]

Slices and maps agreed -- I'll do both.

[sauyon]

In case you didn't know, you can click the + for comments and drag to make a multi-line comment or suggestion.

[sauyon]

Would it make sense to just make any write to MarshalInput.Message taint the entire struct? I feel like that would reduce the complexity of this quite a bit.

[smowton]

Yeah, this is probably overkill.

[max-schaefer]

Remind me, have we checked that this is sensible in practice, i.e., if one field is tainted then so are the rest?

[smowton]

For this case the fields are an incoming byte buffer, the message to be serialized into it, and some flags, so I don't think there's a strong chance of a false positive.

[sauyon]

Could you make the tests use something like the comment tests from here and it's corresponding ql? I think it's something we should move to.

I'm also happy to do it myself if you'd like.

[smowton]

Sure, sounds like a good step, I'll have a go

[sauyon]

Though I think SsaWithFieldsAndElements might be useful elsewhere, I think it might also be useful to have a superclass for instructions / nodes / exprs that have a "base" so you can just use .getBase()+ here.

[smowton]

Pushed fixes for the simple review comments, will get to Sauyon's improvements this afternoon.

[smowton]

@max-schaefer @sauyon I've pushed the two big changes: support for maps, and using getBase() instead of SsaWithFieldsAndElements, as the top two commits. If you like the getBase solution better I'll drop the SsaWithFieldsAndElements commit.

[max-schaefer]

If you like the getBase solution better I'll drop the SsaWithFieldsAndElements commit.

Yes, I much prefer it. I hadn't realised (or perhaps had forgotten) that we weren't matching up the series of field and element accesses between write and read anyway, so there really isn't much point to capturing it in a special data structure.

[max-schaefer]

A few more suggestions. IIUC, moving away from SsaWithFieldsAndElements should simplify this considerably, which I look forward to.

[max-schaefer]

👍 to introducing this.

Inevitable naming bikeshed: I don't think "aggregate" is a common term in Go (it's not mentioned in the language spec), and of course in QL aggregates are something completely different. How about using "component" as an umbrella term for "element" and "field", and then renaming this class to ComponentReadNode?

Also, I would strongly advise not making this abstract but instead going the slightly more convoluted route of introducing corresponding IR instructions and AST node types and overriding the type of insn. This may seem like overkill, but you'll thank me eventually.

On a similar note, instead of making getBase() abstract (meaning that every subclass has to override it), implement it as none().

[max-schaefer]

It seems like this is currently only used in Protobuf.qll, so perhaps just move it there?

[max-schaefer]

Would it make sense to pull out the very long name of this package into a helper predicate to avoid repetition and line wrapping?

[max-schaefer]

Remind me, have we checked that this is sensible in practice, i.e., if one field is tainted then so are the rest?

[max-schaefer]

Does this do the right thing? Shouldn't succ rather be the post-update node whose pre-update is the base of the write?

[smowton]

Adding the post-update node stuff loses you cases like x[0].y = value, because x does not have a post-update node. What do you recommend here?

[max-schaefer]

Oh, hm, I see. But what's the use of having a step from value to x in this example? There isn't any outgoing flow from x, is there? So wouldn't the flow get stuck?

[smowton]

Aha, looks like I had a pair of nearly-cancelling bugs -- the default taint rule for derefs was conveying the taint from the operand of an implicit deref (here x) back out to the deref (implicit *x) and then to the post-update node of x -- which did exist because of the implicit deref operation.

Only for complex cases I was missing the post-update node's existence because I was failing to account for implicit derefs happening during x[0].y.

I've now fixed that omission and introduced a cast to PostUpdateNode as expected. This means that this case works, due to an implicit deref of x at the top level:

x := &SomeType{}
x.y[z].w[v].a = getTaint()

But this case doesn't, as x doesn't have a post-update node:

x := SomeType{}
x.y[z].w[v].a = getTaint()

[max-schaefer]

Looks like we might need a type that allows us to uniformly treat element/field writes as well as reads?

[smowton]

Combined these by introducing writesComponent

[smowton]

@max-schaefer most suggestions implemented except as noted

[smowton]

@max-schaefer pushed a fix to the post-update node problems, and an extra probably controversial commit that bypasses PostUpdateNode so we can update objects with longer access paths.

[max-schaefer]

Oh, sorry, I didn't mean to suggest that you should do a dbscheme update. That's overkill, and also not the right level of abstraction since a @selectorexpr can be any number of things, not just a field access.

I meant to suggest that you should do this at the QL level:

class ComponentAccess extends Expr {
  ComponentAccess() {
    this instanceof @indexexpr or
     // slightly more complicated reasoning about selectors omitted
  }
}

[smowton]

Reverted the DB and Expr parts of this

[max-schaefer]

I would support extending PostUpdateNodes to do this always. For example, in an assignment

x.y.z = 0

I think it makes sense to consider that both x and x.y have been changed, and hence should have post-update nodes.

[max-schaefer]

Note that this is a somewhat subtle change and would need careful benchmarking.

[smowton]

This will certainly have unforeseen consequences. Let's do it, but as its own subsequent PR?

[max-schaefer]

How about trying it out in a separate PR first? This approach is so ugly that I only want to accept it into main once alternatives have been tried and found wanting.

[smowton]

Dropped this commit for now

[smowton]

@max-schaefer dropped the post-update node changes to pursue in a future PR, and reverted the DB and Expr.qll changes.

[max-schaefer]

OK, this is looking quite reasonable now. I think it would be worth doing a dist-compare, though, just to make sure.

[max-schaefer]

Perhaps pull node.getBase() out into a variable. While it may not affect performance much in this case, it's generally best to keep if conditions as simple as possible.

[smowton]

Rewrote to not use if in that case

[max-schaefer]

Perhaps would could make this predicate private?

[max-schaefer]

Perhaps it would be easier to just declare marshalInput as a PostUpdateNode? Then you don't need the cast.

[smowton]

@max-schaefer changes applied, running a distcompare now

[smowton]

Distcompare results are unremarkable; think this is ready to merge.