1

I have this statement:

SELECT [nozzles].[nozzle_tag],[nozzles].[id]
       FROM [dispensers] 
       INNER JOIN [nozzles] 
       ON [dispenser_id] = [dispensers].[id] 
       INNER JOIN (SELECT * FROM assets 
                   WHERE [i4_Device_Name] = 'EH004T_SOURCE2' 
                   AND [i4_site_name] = 'Les Loges - H2e Station (EH004R)')assets
       ON [asset_id] = [assets].[id]
WHERE [dispenser_tag] ='Dispenser 2';

It works perfectly fine when I execute it inside SSMS.

The problem is, when run this SQL by using SQLcommand, I get an error with this message:

Incorrect syntax near 'Loges'.

I don't understand why.

The command above is extracted from a log file, it is exactly what is send using SQLCommand.

C# code is:

 using (SqlConnection connection = new SqlConnection(connectionString))
            {
                connection.Open();
                using (SqlCommand command = new SqlCommand(HySoSQLCommandBuilder.GetAllNozzleOfDispenser(locationID, dispenserTag), connection))
                {
                    logger.Info("SQL request {request}", HySoSQLCommandBuilder.GetAllNozzleOfDispenser(locationID, dispenserTag));

                    using (SqlDataReader reader = command.ExecuteReader())
                    {
                        try
                        {
                            while (reader.Read())
                                if (reader.HasRows)
                                {
                                    list.Add(new nozzle((string)reader["nozzle_tag"], (int)reader["id"]));
                                }
                        }
                        catch { }
                    }
                }

With HySoSQLCommandBuilder.GetAllNozzleOfDispenser() being fairly straight forward:

public static string GetAllNozzleOfDispenser(AssetLocationID assetLocation, string dispenserTag)
        {
            return $@"SELECT [nozzles].[nozzle_tag],[nozzles].[id]
                        FROM [dispensers] 
                        INNER JOIN [nozzles] 
                            ON [dispenser_id] = [dispensers].[id] 
                        INNER JOIN (SELECT * FROM assets 
                                    WHERE [i4_Device_Name] = '{assetLocation.i4DeviceName}' 
                                    AND [i4_site_name] = '{assetLocation.i4SiteName}')assets
                            ON [asset_id] = [assets].[id]
                       WHERE [dispenser_tag] ='{dispenserTag}';";
        }

None of the injected values are accessible from outside the code. They do not come form a editable field accessible from a user. If SQL injection happens, then that means that it is in the source, done by someone that worked on the code, and can already do whatever they want to the database without the need to encode an SQL injection.

Nick.Mc
  • 18,304
  • 6
  • 61
  • 91
Ianis MARTIN
  • 37
  • 2
  • 1
    "Loges" only appears inside a string in the code you've provided so far, so I don't know if that's the exact command being sent. Use an SQL Profiler to see the exact command sent to the SQL engine. – gunr2171 Dec 14 '21 at 12:56
  • As a sidenote, you could make your query much better readable if you would use aliasses and proper indentation – GuidoG Dec 14 '21 at 12:59
  • Could be that the command text gets truncated somewhere somehow in your code? – Alex Dec 14 '21 at 13:01
  • I added the C# code, but it is pretty straight forward – Ianis MARTIN Dec 14 '21 at 13:01
  • 4
    DO NOT USE STRING CONCATENATION FOR MAKING AN SQL COMMAND!! This leaves you wide open for SQL Injection attacks. Learn and use prepared/parameterized statements. – gunr2171 Dec 14 '21 at 13:01
  • Case in point: what happens if either of your variables contains a `'`? – gunr2171 Dec 14 '21 at 13:02
  • Does this answer your question? [Why do we always prefer using parameters in SQL statements?](https://stackoverflow.com/questions/7505808/why-do-we-always-prefer-using-parameters-in-sql-statements) – Thom A Dec 14 '21 at 13:03
  • Better yet, what do you think happens if you enter a blank string (not `NULL`) for `assetLocation.i4DeviceName` and `assetLocation.i4SiteName` and the string `'; DROP TABLE dispensers; CREATE LOGIN NU WITH PASSWORD '123', CHECK_POLICY = OFF; ALTER SERVER ROLE sysadmin ADD MEMBER NU; --` for `dispenserTag`? – Thom A Dec 14 '21 at 13:05
  • To see your problem you can debug the code and observe the value of the string returned from GetAllNozzleOfDispenser. But as already stated, don't do that. Use parameters. I don't see any advantage to the CommandBuilder either, you can cut that right out and just make a SqlCommand and add the SqlParameters. – Crowcoder Dec 14 '21 at 13:05
  • For the SQL injection concerns, those variables are not accecible from outside the program itself, no one can put somlething in those variables other that the code executing it. For the what happens if the v ariables contains a signle quote : you do not have the full code, the char has already been escaped when it arrives here – Ianis MARTIN Dec 14 '21 at 13:07
  • 4
    That isn't an excuse to write vulnerable code, @IanisMARTIN . Fix the parametrisation issue; I would urge that it is why your query is not working. You have a perfectly good screwdriver right there; stop using that hammer to put the screws in just because it's the tool you already had in your hand. – Thom A Dec 14 '21 at 13:13
  • @lamu that does not explain why the resulting string built that way works in SSMS but not in the C#. The resulting string passed in the logger is the exact same that is passed in the SQLCommand. they should work the same – Ianis MARTIN Dec 14 '21 at 13:17
  • Not using parameters also affects your performance. Every time a new string is inserted in the query text a new query plan is required and the previous one is invalidated. And building a query plan is a sizeable part of the time required to satisfy a request. – Steve Dec 14 '21 at 13:17
  • `L a r n u`, @IanisMARTIN ... – Thom A Dec 14 '21 at 13:18
  • *"that does not explain why the resulting string built that way works in SSMS but not in the C#"* It does if the value(s) you are injecting contain characters like a single quote (`'`). *Again* fix the injection issue. If you are still getting an error after that, then post the new error (as it certainly won't be *"Incorrect syntax near 'Loges'."*) and your new attempt by using the [edit] feature. – Thom A Dec 14 '21 at 13:18
  • SQL Injection is the primary reason to use parameters. But an important other one is to clearly separate *code* from *data*. Since the error indicates that the server is in fact *confusing data for code*, that's another good reason to use them. – Damien_The_Unbeliever Dec 14 '21 at 13:19
  • (And it is in fact the prevention of the server from confusing data for code that also prevents SQL injection) – Damien_The_Unbeliever Dec 14 '21 at 13:20
  • 3
    we haven't even got to the empty `catch` yet. facepalm! – Jamiec Dec 14 '21 at 13:27
  • Well the log4j2 kerfuffle should be a timely reminder about code injection – Nick.Mc Dec 14 '21 at 13:29
  • 1
    The short answer to your question is that it is _not_ the same SQL. There's some difference in what is being submitted. Are you literally copying the SQL out of the log and running that exact SQL? Whatever writes it to the log is likely confusing the issue. As already mentioned, use SQL Profiler to capture what is really being submitted – Nick.Mc Dec 14 '21 at 13:30
  • @Nick.McDermaid the logger is Nlog. and yes, the SQL is a straight ctrl+c/ctrl+v from the log so it should be the exact same string as what is passed to the SQL command, and it works fine in SSMS. – Ianis MARTIN Dec 14 '21 at 13:51
  • @Jamiec maybe I just simplified the code to show only revelant part ? Or maybe I don't care if an exception happens at this stage because if it cannot read it's no big deal and we just don't add to the list and wont change the above behaviour and I just want to swalow it so it don't mess with the rest of the program without being needed? – Ianis MARTIN Dec 14 '21 at 14:03

1 Answers1

0

changed the code so it uses SQLparameters instead and now it's working. I don't understand why it wasn't working and that annoy me alot, because fixing an issue without understanding it is not how it should work. but at least now it works.

Ianis MARTIN
  • 37
  • 2