parser: fix possible attack vector#4297
Conversation
The force_eof rule used in the parser allowed for arbitrary statements to be appended to a DDL after a ';', which gets passed through to mysql without any validation. This basically means that someone could append a statement that bypasses the ACL checks in vttablets. This is not a big deal because people that are allowed to execute DDLs generally have all privileges. But it's better to play this safe. Also, the skip to end was looking for ';' characters instead of an actual ';' token, which could also lead to exploitation. I had to refactor the code a bit because we also ignore all errors on partial DDL parse. We have to make sure there are no new tokens past the ';' before accepting partial ddls. In the process, I've renamed 'force eof' -> 'skip to end'. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
demmer
left a comment
demmer
left a comment
There was a problem hiding this comment.
Overall I think this looks fine but I have a couple detailed questions about the changes that would love clarification on.
| // we should not accept partially parsed DDLs. They | ||
| // should instead result in parser errors. See the | ||
| // Parse function to see how this is handled. | ||
| tkn.partialDDL = nil |
There was a problem hiding this comment.
What is this particular case trying to protect against? It's not immediately obvious to me why this is necessary since the check in Parse() seems like it would be sufficient.
There was a problem hiding this comment.
Without this reset, a statement like this would get quietly accepted by vitess: incomplete ddl; evil query.
This is the case where a ; tirggers a parser error. That will cause the Parse function to detect that it was a partial DDL. So, it will accept the entire string, including evil query. It's the second test case in TestSkipToEnd.
There was a problem hiding this comment.
I guess I'm confused about the specific sequence we expect to happen in that evil example above.
From my read of the code, it flows like this:
- The incomplete DDL sets
SkipToEnd - The next time through the lexer it should scan ahead to the
;character usingskipStatement, but no further - It then returns to
Parsewith the partialDDL flag set. - Parse then calls
Scanagain expecting only a 0 but finds more tokens fromevil queryand fails then.
Presumably I'm missing something but can you clarify where my logic is wrong?
There was a problem hiding this comment.
After some DM discussion in Slack I now understand.
| if tkn.lastChar != ';' { | ||
| tkn.skipStatement() | ||
| } | ||
| tkn.skipStatement() |
There was a problem hiding this comment.
Why did you remove the if tkn.lastChar != ';' guard here?
Would some input likeselect invalid; select ok from table; end up skipping the 2nd statement?
There was a problem hiding this comment.
This is because skipStatement has been refactored to take care of that same check.
2 participants
The force_eof rule used in the parser allowed for arbitrary
statements to be appended to a DDL after a ';', which gets
passed through to mysql without any validation.
This basically means that someone could append a statement
that bypasses the ACL checks in vttablets. This is not a big
deal because people that are allowed to execute DDLs generally
have all privileges. But it's better to play this safe.
Also, the skip to end was looking for ';' characters instead
of an actual ';' token, which could also lead to exploitation.
I had to refactor the code a bit because we also ignore all
errors on partial DDL parse. We have to make sure there are
no new tokens past the ';' before accepting partial ddls.
In the process, I've renamed 'force eof' -> 'skip to end'.
Signed-off-by: Sugu Sougoumarane ssougou@gmail.com