Conversation
Will fix later
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
The locations were shifted due to the module-level docstring attached at the beginning of the file. The counts of the rows remain the same.
lcartey
left a comment
lcartey
left a comment
There was a problem hiding this comment.
I've added some more requested changes related to property writes to object literals used as arguments to entries calls - I don't believe they are vulnerable, and I think we should remove them as sinks.
It also looks like the expected SARIF results need to be updated.
It is unclear at the time being if a string concatenation written to a property of its argument is a possible injection vector or not. Therefore, remove them from the test suite until the answer to the above question becomes clear.
This is also related to the controversial `entries` call.
lcartey
left a comment
lcartey
left a comment
There was a problem hiding this comment.
Two final changes - otherwise I think we will still flag some of the entity cases.
I think we should also resurrect the entity tests to show that we don't currently flag them.
| chainedMethodCall = this.getAChainedMethodCall(_) | ||
| | | ||
| result = chainedMethodCall.getAnArgument() or | ||
| result = chainedMethodCall.getAnArgument().(SourceNode).getAPropertyWrite().getRhs() |
There was a problem hiding this comment.
I think we want to remove this case, otherwise it will still be a sink via StringConcatParameterOfCqlShortcutMethodCall.
| chainedMethodCall = this.getAChainedMethodCall(_) | ||
| | | ||
| result = chainedMethodCall.getAnArgument() or | ||
| result = chainedMethodCall.getAnArgument().(SourceNode).getAPropertyWrite().getRhs() |
There was a problem hiding this comment.
I think we want to remove this case, otherwise it will still be a sink via StringConcatParameterOfCqlShortcutMethodCall.
| chainedMethodCall = this.getAChainedMethodCall(_) | ||
| | | ||
| result = chainedMethodCall.getAnArgument() or | ||
| result = chainedMethodCall.getAnArgument().(SourceNode).getAPropertyWrite().getRhs() |
There was a problem hiding this comment.
I think we want to remove this case, otherwise it will still be a sink via StringConcatParameterOfCqlShortcutMethodCall.
Even for INSERT, UPSERT, or CREATE calls, keep the tracking granularity low to prevent the CQL injection query making an alert on those cases with calls to `entries`.
1. These are now deemed safe (for now), so flip the labels. 2. Some cases are impossible: `entries` call only accepts only objects (as varargs or an array of them). So, if it can be fixed, then fix it; otherwise, remove it.
lcartey
left a comment
lcartey
left a comment
There was a problem hiding this comment.
Looks good to me, other than the unit test failure.
…s tainted
For example, this example was not alerted on:
``` javascript
this.on("send00234", async (req) => {
const { id } = req.data;
const { Service1Entity } = this.entities;
await UPDATE.entity(Service1Entity).set("col1 = col1 + " + id).where`ID = ${id}`; // UNSAFE: direct concatenation with `+`
});
```
For cases of `.run(...)`, the comment location was made on where the CQL was assembled. Shift the label to be where the actual sink, the call to `run`, is.
The previous query marked only the base shortcut call (srv.read, srv.update, ...) as the primary location. This is a bit misleading since the "this query" part would mean the entire chained method call. Therefore, make the chained method call at the very end as the primary location, thereby making the entire chained call including the base shortcut call as the primary location.
The actual "string concatenation" location is found in the path information.
|
Changes since last review:
|
3 participants
What this PR contributes
Improve CQL Injection Query.
CqlQueryRunnerCallnow expands and replacesTaintedCqlClause, now coveringcds.run,cds.db.run,srv.run, andtx.run..runand property readentities:EntityEntryis "absorbed" intoEntityReference. Plus,EntityReferencenow covers more examples, namely,cdsas a shortcut tocds.db.Add robust test cases (This is the gist of this PR, please take a look for the behavioral summary description of what this PR aims to implement).
cds.runand friends.await-ing the query.this.runand friends.Service2.runand friends.cds.ql.cds.parse.cql.CQL.Service2.tx( tx => tx.run(...) )and friends.this.tx( tx => tx.run(...) )and friends.cds.tx( tx => tx.run(...) )and friends.cds.db.tx( tx => tx.run(...) )and friends.Future works
cds.read(...).from(...), only thecds.read(...)part is alerted on, where the entire chained method call is more desirable as a alert location.SensitiveExposure.qlseems to be quite brittle, it needs a rewrite. This PR only updates the query's reference to old definitions that are no longer available.