Data flow for Go json/encoding Encoder

[jason-invision]

I am trying to model the json/encoding Encoder data flow.

For example,

        if err := json.NewEncoder(body).Encode(data); err != nil {
		return nil, err
	}

	req, err := http.NewRequest("POST", url, body)
	if err != nil {
		return nil, err

The data flow should go from data to body.

I created the following FunctionModel's

private class JsonNewEncoderFunction extends TaintTracking::FunctionModel {
  JsonNewEncoderFunction() { this.hasQualifiedName("encoding/json", "NewEncoder") }

  override predicate hasTaintFlow(FunctionInput inp, FunctionOutput outp) {
    inp.isResult() and outp.isParameter(0)
  }

}

private class JsonEncoderDecodeFunction extends TaintTracking::FunctionModel, Method {
  JsonEncoderDecodeFunction() { this.(Method).hasQualifiedName("encoding/json", "Encoder", "Encode") }

  override predicate hasTaintFlow(FunctionInput inp, FunctionOutput outp) {
    inp.isParameter(0) and outp.isReceiver()
  }

}

I assumed that the Method would trace the data flow to the receiver, which would then trace it to the parameter.

[sauyon]

I don't think the FunctionOutput FunctionInput result flow works in this case, because we wouldn't normally track any flow coming from that value (since it's thrown away); you'll probably have to manually implement a taint step from an encode call to its corresponding encoder's value. Something similar was done for HTTP body write modeling, I believe, so take a look at that.

Also, since this seems like an oversight in our models, feel free to open a PR if you get it working 😄

EDIT: I wrote FunctionOutput when I meant FunctionInput. Also, a slightly more in-depth explanation, which I probably should have given earlier (in case you're interested):

The FunctionInput for results (which you're using because of inp.isResult()) has a getEntryNode predicate [1] that gets a PostUpdateNode. However, because the result of the call to json.NewEncoder is not actually used anywhere else, we don't actually generate a PostUpdateNode for that value [2]. Thus, getEntryNode on the FunctionInput for json.NewEncoder has no results, and therefore will not result in flow.

1: Note pun = result, where pun is a PostUpdateNode.

https://github.com/github/codeql-go/blob/c905149579ef7dd9d82dd746284d29c9c0ed3370/ql/src/semmle/go/dataflow/FunctionInputsAndOutputs.qll#L109-L120

2: Note there is no case here for a call, only for pointer logic, writes, and mutable arguments:

https://github.com/github/codeql-go/blob/c905149579ef7dd9d82dd746284d29c9c0ed3370/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll#L446-L465

EDIT2: Nevermind, it has to do with the SsaBit of [1] above; see the comment below.

[sauyon]

Anybody know why those links aren't embedding?

[max-schaefer]

Note there is no case here for a call, only for pointer logic, writes, and mutable arguments

But in this case, the call is a receiver, which, at the data-flow level, is treated as just another argument. The problem is rather that the result of the call is expected to be assigned to an SSA variable, which it isn't in this case.

[max-schaefer]

I assumed that the Method would trace the data flow to the receiver, which would then trace it to the parameter.

That is a very reasonable assumption. It appears that this doesn't currently work due to somewhat technical reasons. We will look into making it work.

[jason-invision]

I came up with this work around for now.

override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
    exists(DataFlow::CallNode f, DataFlow::CallNode m |
      f.getTarget().hasQualifiedName("encoding/json", "NewEncoder")
      and m.getTarget().(Method).hasQualifiedName("encoding/json", "Encoder", "Encode")
      and m.getReceiver() = f
      and pred = m.getArgument(0) and
      succ = f.getArgument(0).getAPredecessor()
    ) 
  
  }

[max-schaefer]

Also, since this seems like an oversight in our models, feel free to open a PR if you get it working smile

We actually have models both for NewEncoder and Encode, but this bug is preventing them from working as expected.

[sauyon]

Ah, should we move the EncodingJson models in Stdlib.qll there as well?

[max-schaefer]

I believe they have already been moved.

[sauyon]

I should probably pull main more often...

[max-schaefer]

Thanks for your report, @jason-invision! The just-merged github/codeql-go#348 should hopefully fix this; can you try again?