I am responsible for securing an old Windows C++ project. It uses the MFC ODBC CDatabase class to interface with SQL-Server. All DB query and insert operations are done using parameterized stored-procedures, so it seems like good practice.
But almost all the DB C++ code prepares statements in this form:
//ParamStr1 and ParamStr2 are string-type API parameters
CString SQL;
CDatabase DB;
SQL.Format("{CALL InsertStoredProcedureXYZ('%s','%s')}", ParamStr1, ParamStr2);
DB.ExecuteSQL(SQL);
A similar thing is done for queries:
//ParamStr1 and ParamStr2 are string-type API parameters
CString SQL;
CRecordset Recordset;
SQL.Format("{CALL QueryStoredProcedure('%s','%s')}", ParamStr1, ParamStr2);
Recordset.Open(CRecordset::forwardOnly, SQL, CRecordset::readOnly);
My worry is that the parameters used for these statements often come directly from web API parameters. The code does some simple parameter screening, mainly to replace ' with '' and to verify string length.
I believe {CALL SP()} is an ODBC thing. I don't think SQL-Server has a CALL command. So is ODBC doing injection-safe parameter preparation before invoking the stored-procedure? I also know that ODBC can instead be used to call stored procedures using {CALL SP(?,?)} followed by parameter-binding. That seems to be better practice but changing to this would require a huge refactor of the many times that the project code prepares CALL statements.
This C++ MFC ODBC code seems quite old fashioned so I have failed to find anything that definitively tells me whether or not I can trust building {CALL SP()} strings directly from API parameters. Is this just bad old SQL statement building disguised as good practice?
This is not safe.
We don't see all the code, but since the injected values are surrounded in single quotes, we can conclude that they are being pasted into the SQL batch, rather than being sent as parameters.
SQL.Format("{CALL InsertStoredProcedureXYZ('%s','%s')}", ParamStr1, ParamStr2);