Skip to content
Permalink
Browse files

Don't fool detection of unsafe queries by comments. Closes #685

  • Loading branch information...
ansgarbecker committed Oct 29, 2019
1 parent ffb6170 commit 4b39eeb15398d3168e891778cc8b2306c3ea5635
Showing with 58 additions and 3 deletions.
  1. +54 −0 source/apphelpers.pas
  2. +4 −3 source/main.pas
@@ -53,10 +53,12 @@ TSQLSentence = class(TObject)
FOwner: TSQLBatch;
function GetSize: Integer;
function GetSQL: String;
function GetSQLWithoutComments: String;
public
LeftOffset, RightOffset: Integer;
constructor Create(Owner: TSQLBatch);
property SQL: String read GetSQL;
property SQLWithoutComments: String read GetSQLWithoutComments;
property Size: Integer read GetSize;
end;
TSQLBatch = class(TObjectList<TSQLSentence>)
@@ -3132,6 +3134,58 @@ function TSQLSentence.GetSQL: String;
end;


function TSQLSentence.GetSQLWithoutComments: String;

This comment has been minimized.

Copy link
@rentalhost

rentalhost Oct 29, 2019

Collaborator

What do you think about use of EXPLAIN FORMAT=JSON to identify WHERE usage for UPDATE? It will avoid all possibilities of false-positives.

This comment has been minimized.

Copy link
@ansgarbecker

ansgarbecker Oct 30, 2019

Author Collaborator

Can you be more specific, where EXPLAIN tells me the query has a WHERE clause? Is the Extra column what you mean, it contains "Using where" or "Using buffer" in some test cases here?

This comment has been minimized.

Copy link
@rentalhost

rentalhost Oct 30, 2019

Collaborator

You need set FORMAT=JSON, then you will receive something like:

EXPLAIN FORMAT=JSON
UPDATE table SET id = id WHERE id = 1
EXPLAIN =
{
    "query_block": {
        "select_id": 1,
        "table": {
            "update": 1,                             // UPDATE command
            "table_name": "table",                   // UPDATE table
            "access_type": "range",
            "possible_keys": ["PRIMARY"],
            "key": "PRIMARY",
            "key_length": "4",
            "used_key_parts": ["id"],
            "rows": 1,
            "attached_condition": "table.`id` = 1"   // WHERE table.id = 1
        }
    }
}

So when WHERE is omitted, it will returns only:

EXPLAIN = 
{
    "query_block": {
        "select_id": 1,
        "table": {
            "update": 1,
            "table_name": "table",
            "access_type": "index",
            "key": "PRIMARY",
            "key_length": "",
            "used_key_parts": ["id"],
            "rows": 43202
        }
    }
}

It will able you to check, for instance, how much rows is affected if you run. Maybe you can create an additional alert like "this query will affect all rows from table".

This comment has been minimized.

Copy link
@rentalhost

rentalhost Oct 30, 2019

Collaborator

Forget it, in some circunstancies it will generate a very different JSON output without attached_condition.

EXPLAIN FORMAT=JSON
UPDATE table SET table.id = 1 WHERE id IN (SELECT id FROM table)
EXPLAIN =
{
    "query_block": {
        "select_id": 1,
        "table": {
            "table_name": "table",
            "access_type": "ALL",
            "possible_keys": ["PRIMARY"],
            "rows": 43202,
            "filtered": 100
        },
        "table": {
            "table_name": "table",
            "access_type": "eq_ref",
            "possible_keys": ["PRIMARY"],
            "key": "PRIMARY",
            "key_length": "4",
            "used_key_parts": ["id"],
            "ref": ["db.table.id"],
            "rows": 1,
            "filtered": 100,
            "using_index": true
        }
    }
}

🤷‍♂

var
InLineComment, InMultiLineComment: Boolean;
AddCur: Boolean;
i: Integer;
FullSQL: String;
Cur, Prev1, Prev2: Char;
begin
// Strip comments out of SQL sentence
// TODO: leave quoted string literals and identifiers untouched
FullSQL := GetSQL;
Result := '';
InLineComment := False;
InMultiLineComment := False;
Prev1 := #0;
Prev2 := #0;
for i:=1 to Length(FullSQL) do begin
Cur := SQL[i];
AddCur := True;
if i > 1 then Prev1 := SQL[i-1];
if i > 2 then Prev2 := SQL[i-2];

if (Cur = '*') and (Prev1 = '/') then begin
InMultiLineComment := True;
Delete(Result, Length(Result), 1); // Delete comment chars
end
else if InMultiLineComment and (Cur = '/') and (Prev1 = '*') then begin
InMultiLineComment := False;
Delete(Result, Length(Result), 1);
AddCur := False;
end;

if not InMultiLineComment then begin
if InLineComment and ((Cur = #13) or (Cur = #10)) then begin
InLineComment := False; // Reset
end
else if Cur = '#' then begin
InLineComment := True;
end
else if (Cur = ' ') and (Prev1 = '-') and (Prev2 = '-') then begin
InLineComment := True;
Delete(Result, Length(Result)-1, 2); // Delete comment chars
end;
end;

if AddCur and (not InLineComment) and (not InMultiLineComment) then begin
Result := Result + Cur;
end;
end;
end;


{ TSQLBatch }

function TSQLBatch.GetSize: Integer;
@@ -2912,7 +2912,7 @@ procedure TMainForm.actExecuteQueryExecute(Sender: TObject);
Batch: TSQLBatch;
Tab: TQueryTab;
BindParamItem: Integer;
NewSQL, msg, Command: String;
NewSQL, msg, Command, SQLNoComments: String;
Query: TSQLSentence;
rx: TRegExpr;
ContainsUnsafeQueries, DoExecute: Boolean;
@@ -2957,8 +2957,9 @@ procedure TMainForm.actExecuteQueryExecute(Sender: TObject);
rx.Expression := '\sWHERE\s';
ContainsUnsafeQueries := False;
for Query in Batch do begin
Command := UpperCase(getFirstWord(Query.SQL));
if ((Command = 'UPDATE') or (Command = 'DELETE')) and (not rx.Exec(Query.SQL)) then begin
SQLNoComments := Query.SQLWithoutComments;
Command := UpperCase(getFirstWord(SQLNoComments));
if ((Command = 'UPDATE') or (Command = 'DELETE')) and (not rx.Exec(SQLNoComments)) then begin
ContainsUnsafeQueries := True;
Break;
end;

0 comments on commit 4b39eeb

Please sign in to comment.
You can’t perform that action at this time.