query option to use parser.jj extension instead of regex

[walterddr]

Background

Currently Query Options are parsed via regex match and replaced with empty string. This causes SQLi issue.

Solution

• using parser.jj extension.
• backward-incompatible change, OPTIONS now can only be supported outside of a SQL statement (e.g. beginning or end of SQL string)

Release Note:

This is a backward incompatible change for utilizing OPTION keyword.

• no longer allowed OPTION(key=value) ANYWHERE in the SQL query string; with this change only allow at the beginning or the end of the SQL string payload.
• new syntax also requires identifier to literal syntax, for example:
  OPTION(foo=bar, alice=123, bob='potato', complex.key=valueLiteral)
  is now:
  OPTION(foo='bar', alice='123', bob='''potato''', "complex.key"='valueLiteral');
• Note that standard SQL syntax statement separateor ; must be added between statements. both among multiple OPTION definitions; and between OPTION definitions and SQL DQL/DML statement.

Additional Remark

This new syntax can theoretically support other literal types for example:

• OPTION(foo='bar', alice=123) --> Map<String, Object>.of("foo", (String) bar", "alice", (Integer) 123);
• OPTION(foo=true) --> Map<String, Object>.of("foo", (boolean) true);

However since currently all k-v options support by Pinot is a string/string mapping, we will defer this to a new feature if necessary

Appendix

Full SQL syntax algebra

statementList:
      [ OPTION '(' option [, option ]* ')'; ]
      statement;
      [ OPTION '(' option [, option ]* ')'; ]

option:
      identifier
      '='
      literal

identifier:
      key | "complex.key"

literal
      QuotedLiteralString

[richardstartin]

🔥

[siddharthteotia]

Not related to this PR --

Looks like this function was added when INSERT grammar extension was done and is being called from broker query endpoint in PinotClientRequest.java.

It seems like for DQL, the caller will parse the SQL statement twice. Once at line 153 when it calls this method in the parser to retrieve SqlNodeAndOptions and then if the type is not DML, it sends the query to requestHandler.handleRequest(sql) which will call the parser again (compileToPinotQuery(sql)) and the statement will be parsed again.

Not sure if this is expected. I just noticed it

[walterddr]

i dont think so but yeah I will take a look to address this as well

[walterddr]

actually the logic here is a bit more complex than simple reduce the compilation. I will create another PR to follow up.

[siddharthteotia]

Can we also add tests using examples called out in https://blog.doyensec.com/2022/06/09/apache-pinot-sqli-rce.html#query-options. Since they now don't fit in the grammar, we can check if they fail ?

[walterddr]

oh yes absolutely. I will add to the test set

[siddharthteotia]

I will spend sometime examining the production logs to see how OPTION is being used in production (if at all) and how it will break with this change. Will get back with the info by early next week.

[codecov-commenter]

Codecov Report

Merging #8880 (4183b88) into master (ea564f0) will decrease coverage by 0.11%.
The diff coverage is 76.92%.

@@             Coverage Diff              @@
##             master    #8880      +/-   ##
============================================
- Coverage     69.78%   69.67%   -0.12%     
- Complexity     4672     4680       +8     
============================================
  Files          1803     1807       +4     
  Lines         93752    94212     +460     
  Branches      13935    14053     +118     
============================================
+ Hits          65426    65643     +217     
- Misses        23790    24011     +221     
- Partials       4536     4558      +22     

Flag	Coverage Δ
integration1	26.31% <69.23%> (-0.06%)	⬇️
integration2	24.89% <65.38%> (-0.23%)	⬇️
unittests1	66.37% <77.55%> (-0.18%)	⬇️
unittests2	15.39% <0.00%> (-0.06%)	⬇️

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

Impacted Files	Coverage Δ
...rg/apache/pinot/sql/parsers/parser/SqlOptions.java	45.45% <45.45%> (ø)
...t/controller/api/resources/PinotQueryResource.java	59.05% <50.00%> (ø)
...org/apache/pinot/sql/parsers/CalciteSqlParser.java	87.66% <85.29%> (-0.30%)	⬇️
...pinot/broker/api/resources/PinotClientRequest.java	58.49% <100.00%> (ø)
...rg/apache/pinot/sql/parsers/SqlNodeAndOptions.java	100.00% <100.00%> (ø)
...g/apache/pinot/sql/parsers/dml/InsertIntoFile.java	85.18% <100.00%> (ø)
...raw/RawDoubleSingleColumnDistinctOnlyExecutor.java	61.11% <0.00%> (-38.89%)	⬇️
...y/distinct/raw/RawMultiColumnDistinctExecutor.java	52.04% <0.00%> (-36.85%)	⬇️
...ct/raw/RawIntSingleColumnDistinctOnlyExecutor.java	44.44% <0.00%> (-35.56%)	⬇️
...t/raw/RawLongSingleColumnDistinctOnlyExecutor.java	44.44% <0.00%> (-35.56%)	⬇️
... and 59 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ea564f0...4183b88. Read the comment docs.

[Jackie-Jiang]

This is awesome! Should we consider also supporting identifier as value? That way the current way of putting options can work

[kishoreg]

+1 thanks @walterddr for adding this

[Jackie-Jiang]

Actually after a second thought, I think it is okay to keep the current way without affecting current query options. The only query option value that is not number/boolean is FORCE_HLC which I believe no one is using. @siddharthteotia

There is a major issue not handled though. In PinotClientRequest.constructSqlQueryOptions(), the groupByMode and responseFormat value SQL cannot be parsed, and that can cause user not able to upgrade from 0.10.0. We need to quote the value if there is no easy way to support the previous format

[Jackie-Jiang]

Just noticed that seems it requires an extra ; to split the statements, then all the existing query option won't work... We should document this change in the PR description

[walterddr]

Actually after a second thought, I think it is okay to keep the current way without affecting current query options. The only query option value that is not number/boolean is FORCE_HLC which I believe no one is using. @siddharthteotia

adding OPTION(identifier = identifier) is possible but the identifier cannot start with a number so we are still not backward-compatible.

There is a major issue not handled though. In PinotClientRequest.constructSqlQueryOptions(), the groupByMode and responseFormat value SQL cannot be parsed, and that can cause user not able to upgrade from 0.10.0. We need to quote the value if there is no easy way to support the previous format

This is a good catch, will fix; but also do we know if there's any other places that adds the OPTION regex to the query?

Just noticed that seems it requires an extra ; to split the statements, then all the existing query option won't work... We should document this change in the PR description

Yes this is the standard SQL statement separator. however the ; before the <EOF> token can be skipped thus we don't have any issue so far. I will document this.

[walterddr]

closing this in favor of #8906 for a more systematic fix.