Fix: Fix and enhance SQL injection prevention #40

oscar60310 · 2022-08-24T09:45:47Z

What's fixed

After Feature: SQL Injection Prevention (Flow Simulation) #23, we replace the input parameters with parameterized values like $1, $2 ...etc. But we need the raw value with template logic, for example:
```
select * from user
{% if context.params.age %} where age > {{ context.params.age }} {% endif %}
```
The above template will always render the where query because context.params.age is "$1". This PR fixed this issue, it replaces values only when needed.
We sent useless binding with sub-queries. For example:
```
 {% req user %}
    select id from users where userName = {{ context.params.name }}
{% endreq %}
select * from groups where groupName = {{ context.params.groupName }}
```
We sent select id from users where userName = $1 with binding ['someUserName', 'someGroupName'] at the first query, which might cause driver failure (binding length is not equal to query usage). This PR sends binding with only required parameters.

Enhancement

Parameterize all the values including sub queries' values, user attributes...

{% req user %}
select id from users where userName = {{ context.params.name }}
{% endreq %}
select * from groups where userId = {{ user.value()[0].id }}; --- this value will be parameterized too.

Provide a filter raw to force us to render the raw values.

{{ context.params.name }} --- $1
{{ context.params.name | raw }} --- someName

codecov-commenter · 2022-08-31T03:32:38Z

Codecov Report

Base: 91.36% // Head: 92.40% // Increases project coverage by +1.03% 🎉

Coverage data is based on head (4285267) compared to base (dc2cf86).
Patch coverage: 90.85% of modified lines in pull request are covered.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop      #40      +/-   ##
===========================================
+ Coverage    91.36%   92.40%   +1.03%     
===========================================
  Files          199      221      +22     
  Lines         2630     3040     +410     
  Branches       280      354      +74     
===========================================
+ Hits          2403     2809     +406     
+ Misses         177      169       -8     
- Partials        50       62      +12

Flag	Coverage Δ
build	`94.87% <100.00%> (+0.63%)`	⬆️
core	`93.13% <94.26%> (+2.01%)`	⬆️
extension-dbt	`97.43% <ø> (ø)`
extension-debug-tools	`98.11% <98.11%> (?)`
integration-testing	`95.00% <100.00%> (-1.43%)`	⬇️
serve	`88.78% <88.19%> (+0.67%)`	⬆️
test-utility	`∅ <ø> (∅)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
packages/build/src/containers/types.ts	`100.00% <ø> (ø)`
...generator/spec-generator/oas3/oas3SpecGenerator.ts	`97.33% <ø> (-0.04%)`	⬇️
packages/build/src/models/index.ts	`100.00% <ø> (ø)`
packages/build/src/options/index.ts	`100.00% <ø> (ø)`
packages/core/src/containers/types.ts	`100.00% <ø> (ø)`
packages/core/src/lib/data-source/pg.ts	`42.85% <0.00%> (+22.85%)`	⬆️
packages/core/src/models/extensions/dataSource.ts	`100.00% <ø> (ø)`
...ackages/core/src/models/extensions/filterRunner.ts	`100.00% <ø> (ø)`
packages/core/src/models/extensions/tagRunner.ts	`95.23% <ø> (ø)`
packages/serve/src/containers/types.ts	`100.00% <ø> (ø)`
... and 78 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

- passing context to filter runner - add santizier filter builder and runner - add TemplateInput class to control parameterize logics - update req runner to inject parameterizers

…ention - remove duplicated type definitions. - hide data source in executer. - remove useless exports. - add some comments. - move some logic into functions.

kokokuo

Besides some questions ( I do not figure out the part...), others LGTM, Awsome 👍

kokokuo · 2022-09-07T04:01:24Z

packages/core/src/models/extensions/dataSource.ts

+  /** The index (starts from 1) of parameters, it's useful to generate parameter id like $1, $2 ...etc. */
+  parameterIndex: number;


Should we also consider the non-index parameters, like big query seems only support ? placeholder and naming placeholder e.g: @name, @url ... and so on.

We can simply ignore this input argument and return the identifier whatever you want:

public async prepare() { return `@${someRandom()}` }

I can't give you some meaningful parameter name because it might not be an input parameter, for example:

{% for val in someArray %} {{ val }} --- I don't know the name of the value, only know the order (index) {% endfor %}

So for Big Query, one alternative is generating naming identifiers by the index too:

public async prepare({parameterIndex}) { return `@var${parameterIndex}` }

Thanks @oscar60310 for replying my question and explain it!

kokokuo · 2022-09-07T06:05:16Z

packages/core/src/models/extensions/dataSource.ts

  bindParams: BindParameters;
  pagination?: Pagination;
 }

+export type PrepareParameter = { (param: RequestParameter): Promise<string> };


Could we rename to PrepareParameterFunc or PrepareParameterAsyncFunc to represent explicitly ?

Or it looks like a type object, so that when seeing the prepare: PrepareParameter in IExecutor interface, not easy to attend it until discovering not add the await at dataSource.prepare under executor prepare method.

Sure, I'll update it to PrepareParameterFunc, thanks!

Thanks so much !

kokokuo · 2022-09-07T06:42:18Z

packages/core/src/lib/template-engine/nunjucksCompiler.ts

+        function (this: any, value: any, ...args) {
+          // use classic function to receive context
+          extension.__transform(this, value, ...args);
+        },


Why do we change this part, not figure out what the influence is, could you introduce it ? Thanks!

Nunjucks binding "context" to "this" (js this) https://github.com/mozilla/nunjucks/blob/master/nunjucks/src/compiler.js#L499

I'd like to pass context to transform functions, but we bound extension object to __transform function, it makes this become extension instead of the context provided by upstream.

extension.__transform.bind(extension),

So I used a classic function here to receive this first then pass to ___transform function as an argument.

kokokuo · 2022-09-07T07:05:24Z

packages/core/src/models/extensions/dataSource.ts

-  // the value is real param data
-  [identifier: string]: string;
-};
+export type BindParameters = Map<string, string>;

 export type IdentifierParameters = {


We could also remove it, because seems it will not use to after the PR.

Removed, thanks!

Thanks @oscar60310's help!

kokokuo · 2022-09-07T07:22:28Z

packages/core/src/lib/template-engine/built-in-extensions/query-builder/sanitizerBuilder.ts

+import { RAW_FILTER_NAME, SANITIZER_NAME } from './constants';
+
+@VulcanInternalExtension()
+export class SanitizerBuilder extends FilterBuilder {


Could you introduce what is the SanitizerBuilder responsible for, why we need to add sanitizer, and put the symbol sanitize? Not figure out the workflow and the domain context for sanitize and not see the description introduce it, thanks so much !

This builder automatically adds the filter 'sanilizer` after the "lookup" like nodes, e.g. LookupVal, FunctionCall ...etc. (so do other filter builders). In order to do sql injection prevention.

{{ context.params.id }} -> {{ context.params.id | sanitizer }}

I've add these comments for the class too.

Thanks @oscar60310 for replying my question

kokokuo · 2022-09-07T07:31:57Z

packages/core/src/lib/template-engine/built-in-extensions/query-builder/sanitizerBuilder.ts

+      }
+
+      if (this.isNodeNeedToBeSanitize(child)) {
+        if (!parentHasOutputNode && !(node instanceof nunjucks.nodes.Output))


Does it mean we only add sanitize when the node is the output or the parent has an output node? But why do we add the sanitize after the output?

Does it mean we only add sanitize when the node is the output or the parent has an output node?

Yes.

This is an ordinary AST tree of template {{ context.params.id }}:

When we traversal the AST from root:

We first meet the output node

And we'll find its child is a LookupVal

We wrap the lookupVal node in filter sanitizer. (via the replace function) {{ context.params.id | sanitizer }}

This is what this builder mainly does.

But why do we add the sanitize after the output?

Output nodes mean to "render" strings, in our case, they also mean to generate the sql to execute, these values should be sanitized (parameterized) before executing, so we add the filter before the output result.

Thanks @oscar60310 for introducing detail so that I could understand it 😃 !!

kokokuo · 2022-09-07T08:27:31Z

packages/core/src/lib/template-engine/built-in-extensions/query-builder/reqTagRunner.ts

    query = query
      .split(/\r?\n/)
      .filter((line) => line.trim().length > 0)
      .join('\n');
    // Get bind real parameters and pass to data query builder for data source used.
-    const binds = (context.ctx || {})['_paramBinds'] || {};
+    const binds = parameterizer.getBinding();


We call parameterizer.getBinding(), but the binds has contains the idToValueMapping data which SanitizerRunner ran await input.parameterize(parameterizer) ?

I'm really confused the each filter's execute order and when calling transform and calling run ~ " ~

We call parameterizer.getBinding(), but the binds has contains the idToValueMapping data which SanitizerRunner ran await input.parameterize(parameterizer) ?

Yes! The magic happened because of the shared context. Please have a look at the image below.

I'm really confused the each filter's execute order and when calling transform and calling run ~ " ~

We're using DFS to compile and execute the nodes, so we run req tag first, then filter.

kokokuo · 2022-09-07T08:31:38Z

packages/core/src/lib/template-engine/built-in-extensions/query-builder/reqTagRunner.ts

+    // parameterizer from parent, we should set it back after rendered our context.
+    const parentParameterizer = context.lookup(PARAMETERIZER_VAR_NAME);
+    context.setVariable(PARAMETERIZER_VAR_NAME, parameterizer);


Sorry, I don't understand why we set the parameterizer back to our context when parameterizer from parent, could you tell more information? Thanks

Because the parent and children shared the same context, if the children don't set it back, their parent will lost its parameterize because it will be overriden.

kokokuo · 2022-09-07T08:46:37Z

packages/core/src/lib/template-engine/built-in-extensions/query-builder/sanitizerRunner.ts

+    // Parameterizer should be set by req tag runner
+    const parameterizer = context.lookup<Parameterizer>(PARAMETERIZER_VAR_NAME);
+    if (!parameterizer) throw new Error(`No parameterizer found`);
+    return await input.parameterize(parameterizer);


I'm curious the parameter only set by req tag runner, the req tag seems to the sample you show on description:

{% req user %} select id from users where userName = {{ context.params.name }} {% endreq %}

but I didn't see where to handle the normal {{ ... }} case, e.g:

select * from user where name = {{ context.params.name }}

Or does the req tag runner also include the case?

Or does the req tag runner also include the case?

Yes, we'll wrap the template with a builder if there is no main builder.
https://github.com/Canner/vulcan-sql/blob/develop/packages/core/src/lib/template-engine/built-in-extensions/query-builder/reqTagBuilder.ts#L146-L158

oscar60310 · 2022-09-13T08:42:21Z

Hi @kokokuo , I've drawn a diagram to describe what's happened with our filters and tags.

oscar60310 · 2022-09-13T08:52:14Z

Hi @kokokuo all issues have been fixed.

kokokuo · 2022-09-13T10:41:36Z

Hi @kokokuo , I've drawn a diagram to describe what's happened with our filters and tags.

Thanks for drawing the detailed steps picture, and explaining to me the flow, it's really helpful for me, thanks so much 😃

oscar60310 force-pushed the chore/vulcan-lab branch 2 times, most recently from 5e131f8 to e01221b Compare August 25, 2022 03:38

oscar60310 force-pushed the fix/sampler-params branch 4 times, most recently from 0bbedc7 to 7b8451f Compare August 30, 2022 09:20

oscar60310 changed the title ~~[WIP] Fix: Fix sampler parameters inputs~~ [WIP] Fix: Fix and enhance SQL injection prevention Aug 31, 2022

oscar60310 force-pushed the chore/vulcan-lab branch from 390df54 to c70ec05 Compare August 31, 2022 03:19

oscar60310 force-pushed the fix/sampler-params branch from fc6fb74 to 5c8ec3a Compare August 31, 2022 03:28

oscar60310 force-pushed the fix/sampler-params branch from 5b52324 to d5f772e Compare August 31, 2022 07:22

oscar60310 marked this pull request as ready for review August 31, 2022 08:04

oscar60310 changed the title ~~[WIP] Fix: Fix and enhance SQL injection prevention~~ Fix: Fix and enhance SQL injection prevention Aug 31, 2022

oscar60310 requested a review from kokokuo August 31, 2022 08:04

oscar60310 force-pushed the chore/vulcan-lab branch from c70ec05 to 5730b0b Compare August 31, 2022 09:18

oscar60310 added 10 commits August 31, 2022 17:39

feat(core,build): add sanitizer and parameterizer

bc94893

- passing context to filter runner - add santizier filter builder and runner - add TemplateInput class to control parameterize logics - update req runner to inject parameterizers

test(core): add test for parameterize

5f4e049

fix(core,build,serve): fix testing for incoming changes

d1098cc

fix(int-test): fix testing of integrate testings

8aad751

fix(core,serve): fix linting, testing issues

4b32905

refactor(build,core): refeactor codes relatived to sql injection prev…

5d878aa

…ention - remove duplicated type definitions. - hide data source in executer. - remove useless exports. - add some comments. - move some logic into functions.

refactor(core): update nunjucks types for context

70d41ff

fix(lab): update duck driver

27f6232

feat(core): add raw filter to stop sanitizer

5e9499a

refactor(core): refactor testing of parameterize

5ff11a6

oscar60310 force-pushed the fix/sampler-params branch from c0f8d3f to 5ff11a6 Compare August 31, 2022 09:40

Base automatically changed from chore/vulcan-lab to develop September 6, 2022 08:28

kokokuo approved these changes Sep 7, 2022

View reviewed changes

refactor(core): remove useless types and rename some functions

4285267

kokokuo merged commit b727780 into develop Sep 13, 2022

kokokuo deleted the fix/sampler-params branch September 13, 2022 10:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Fix and enhance SQL injection prevention #40

Fix: Fix and enhance SQL injection prevention #40

oscar60310 commented Aug 24, 2022 •

edited

Loading

codecov-commenter commented Aug 31, 2022 •

edited

Loading

kokokuo left a comment

kokokuo Sep 7, 2022

oscar60310 Sep 13, 2022 •

edited

Loading

kokokuo Sep 13, 2022

kokokuo Sep 7, 2022

oscar60310 Sep 13, 2022 •

edited

Loading

kokokuo Sep 13, 2022

kokokuo Sep 7, 2022

oscar60310 Sep 13, 2022 •

edited

Loading

kokokuo Sep 7, 2022

oscar60310 Sep 13, 2022

kokokuo Sep 13, 2022

kokokuo Sep 7, 2022

oscar60310 Sep 13, 2022

kokokuo Sep 13, 2022

kokokuo Sep 7, 2022

oscar60310 Sep 13, 2022 •

edited

Loading

kokokuo Sep 13, 2022

kokokuo Sep 7, 2022

oscar60310 Sep 13, 2022 •

edited

Loading

kokokuo Sep 7, 2022

oscar60310 Sep 13, 2022

kokokuo Sep 7, 2022

oscar60310 Sep 13, 2022

oscar60310 commented Sep 13, 2022 •

edited

Loading

oscar60310 commented Sep 13, 2022

kokokuo commented Sep 13, 2022

		/** The index (starts from 1) of parameters, it's useful to generate parameter id like $1, $2 ...etc. */
		parameterIndex: number;

Fix: Fix and enhance SQL injection prevention #40

Fix: Fix and enhance SQL injection prevention #40

Conversation

oscar60310 commented Aug 24, 2022 • edited Loading

What's fixed

Enhancement

codecov-commenter commented Aug 31, 2022 • edited Loading

Codecov Report

kokokuo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oscar60310 Sep 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oscar60310 Sep 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oscar60310 Sep 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oscar60310 Sep 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oscar60310 Sep 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oscar60310 commented Sep 13, 2022 • edited Loading

oscar60310 commented Sep 13, 2022

kokokuo commented Sep 13, 2022

oscar60310 commented Aug 24, 2022 •

edited

Loading

codecov-commenter commented Aug 31, 2022 •

edited

Loading

oscar60310 Sep 13, 2022 •

edited

Loading

oscar60310 Sep 13, 2022 •

edited

Loading

oscar60310 Sep 13, 2022 •

edited

Loading

oscar60310 Sep 13, 2022 •

edited

Loading

oscar60310 Sep 13, 2022 •

edited

Loading

oscar60310 commented Sep 13, 2022 •

edited

Loading