"Your Code Has a SQL Injection!" | Code Cop #007

as usual an enlightening and useful topic and presentation - great job, Nick.

Today's thing is: doesn't matter if what you have to say is right or not, just go and say it. The more important thing is to create engagement. 🤷🏻‍♂️

Verbatim strings are useful when one of the supported SQL engines is SAP HANA, but you still need to make the query compatible with the plain old MS SQL

"If you're not a security expert, don't try to provide security advice!" - goes on and provides security advice :))
Just nitpicking, don't worry, the content is great. :)

I strongly feel that since we have all those AI code generators, we have a serious increase of people very new to programming who really believe they have suddenly become actual great engineers, while they just share randomly composed stuff without even trying to understand it themselves. 🧐 🤦‍♂

So let me see if I understand what you're saying.
The first example fails because we have an int passed in , and therefore no injection is possible. HOWEVER, if the argument was a string , and the call was not parameterized such that the string was directly added to the Sql command -- then that WOULD be an exploitable vulnerability. Is that correct?

If so, I think this is far less of a problem than it's made out to be. While the specific example is poorly chosen because we're using an int, the code is not less safe because we follow the pattern set in the bottom #2 example. I argue we should still follow the #2 example anyway, even if it doesn't add much security, simply because I would rather developers understand one pattern that works every time as opposed to trying to write their methods cleverly because the additional boilerplate isn't strictly necessary. That way lies unnecessary code variation, and that way lies mistakes.

I agree with most what is said... but a 12 minute video about "it's an int!" is a bit over the board. I don't agree with the sentence that you need to be a security expert to post stuff like this. This is not some advance hack (like modifying the prediction branches of the CPU to get values out of registers), it's basic common sense.

Though to be honest, I haven't run a straight up SQL query from C# in lord knows how many years. It's mostly LINQ driven now (e.g. EFCore).

Yep, I had to fix a SQL injection issue that was raised by a pen test. It's what happens when you let front end developers write C#.

A bit of a stretch since the solution to injectable code you came to is the same one in the article. Tho I also easily identified why the bad example is not injectable, there’s no harm in convincing someone to always parameterize and never interpolate for queries. My biggest issue with the overly verbose way it was written….

When you copy paste an example from untyped PHP.

I have been tempted to go around commenting "Congratulations your post has been featured in r/LinkedinLunatics."

Now I'm also tempted to comment "Congratulations your post has been featured in Nick's Code Cop series."

You should also say something about why you want to use parameterized queries even when SQL injection is not possible (with an int parameter)

When you pass a SQL statement to a database it will (have to) compile that SQL statement and create an execution plan for it. If you use string interpolation for the query, the database will not be able to cache the execution plan, so it will do a compile of the SQL statement each time. With a parameterized query the database will cache and re-use the execution plan even when the parameters changes, which means that subsequent queries (even with different parameters) will be significantly more efficient.

But after two decades of corporate development, I've noted that managers need an intervention or something... 3rd party frameworks have value in limited scenarios and hiring better people is the correct path, not more mediocre slackers...

I have horror stories about REALLY BAD software management...

Man, leave the bad advice there, let those non-technical, lame "developers" use the bad advice, let them help one another with bad advice until they all lose their jobs. A lame "developer" who can't discern between good and such very-very bad advice, sorry but they'd better stop pretending to be programmers and go get a cashier job at Tesco.

I mean...
- in the last 5-6 years, the IT market good flooded, everybody and their Mother is now in IT, 75% of these new "developers" are in fact non-technical guys who do more harm than good
- recruitment agents lack any skills, for them all developers are exactly that, developers. Even more: the cheapest, the better
- bad programmers from yesteryear "evolved" and are now managers in charge of hiring other developers but they lack any skill to discern the good from the bad
- when non-technical recruiters and non-technical IT management can't discern the good from the bad developers, who do you think will they prefer to hire? The technical ones they have nothing in common with, or the non-technical useless pricks who remind them of a younger version of themselves?

And that's how all those lame, non-technical "developers" get jobs.
Then, on the job, they use free code samples, free advice, a bit of help from there, maybe ask a colleague, maybe copy-paste some code off Google, maybe ask ChatGPT... That's how they do their jobs.

You've noticed how, in recent years, under the mask of "Agile", in fact very buggy code gets deployed live, the client ends up doing the testing, and the client even pays back the supplier to fix the bugs! Then, while fixing the bug, the lamest developer introduces another two bugs, the client finds these one month later in PROD, and pays again to fix, on and on.
This is the result of all non-technical, lame managers and developers who are calling themselves "developers" but are unable to change their screen resolution in Windows.

So why help those lame guys, for free, with good advice?
Better would be to let them use bad advice until everybody in the industry, including clients, finally realize that such non-technical people should not be called "developers", fire them, then hire the real ones to do the job.

Your take is terrible.
Listed examples (concatenating query with parameter value is wrong, using parametrized query is correct) were perfectly fine. You've made the whole case around nitpicking that parameter type is 'int' rather than 'string' - however author's intention wasn't to teach how to do sql injection, it was to teach how to avoid being sql injected.
You've spend half of the video mocking the author using groundless assumptions about his knowledge. The most upvoted comments under this video are made by yes-mans and are mirroring your toxic demeanor. Is this type of content you want to make and type of community you want to grow?

I remember Jon Skeet's blog post "The BobbyTables Culture" where he created a contrived example where integers could cause SQL injection.

I'm not going to skip ads for this series. Thanks, Nick. Love your content.

Would have loved to see an example of exfiltrating data from another table, just to drive it harder into peoples minds that no table is "unimportant" and care is always required

parametrized queries are very useful, not only for preventing from SQL injections; but it also gives the ability to store them as constants, which reduces memory usage and increase reusability. Saves you headache from type conversions (CLR to DB), and it can also help the DBMS Query Optimizer to create a one query plan, and reuse it for that query. (using string interpolation will create a query plan for every value for the same query, which is something you don't want).

The advice was not only terrible because of the parameter type, but because it promoted inline sql in c# code. Although you can use inline sql in c# code, I fail to understand why people as smart as you Nick promote it.
Writing Sql in stored procedures gives you, the programmer, control and influence over the query optimiser. For simply queries (1 to 3 table joins), an ORM can do a decent job, but when you are dealing with a much more complex query (think 10 to 20 table joins) then there is no chance that an ORM can produce a decent query.
When dealing with such a complex query, you want to have a sub-query using only those tables that perform the filter, then in an outer query add in those other tables that just add *fluff*. This allows multiple indexes to be used on a single table and can greatly improve the performance of the query.
I have recently (past 6 months) improved the performance of a number of stored procedures this way by 90% - yes, the cost (logical reads, cpu, time) of the manually created stored procedure is 10% of the time of the original!

Lmao ppl still saying sequel

English is not my first language, but I'd say I can speak and understand it quite decently. Nick is often a bit fast for me, but I can usualy keep his pace. Now, the Code Cop series is a whole different story: you immediatelly realise when some "advisors" really piss him off - it's like fast forward, and he's breaking the sound barrier of talking. Even my girlfriend can't keep up with him!
Aside from this: the content is so good - I am a happy subscriber!

To be honest I don't really like these Code Cop videos. Usually they aren't as interesting as other videos from you Nick. Thanks for showing us how exactly SQL injection works, but generally the video can be shortened to something like "Hey, you have an int parameter in the method, so SQL injection isn't really possible. And btw you shouldn't use DynamicParameters is such simple cases". Without hate, just expressing my thoughts. Maybe somebody will agree with me and it will be helpful for you. Cheers!

I was laughing even before finishing the video because the guy who posted that one on LinkedIn knows nothing about SQL injection.

I'm not sure about C#, but in some languages (like Java) the string parameterization is dependent on the database driver. Meaning that in some (rare) cases the driver could just concat the string together before sending it to the db, rather than going through the prepared statement step.

My point is that proper prepared statement behavior is not NECESSARILY guaranteed. While using prepared/parameterized statements makes code more robust against SQL Injection, it may not always be sufficient.

So you should also always check / cast your inputs as well. (Casting to an int, for example is great protection)

I would also show what kind of request ultimately comes to Postgres in the Docker container, using the example of SQL injection (with try to delete data from table)

Smirked reading comment section. Parametrization is not a silver bullet and most devs blindly rely on it without a second thought. Ofc it's fine and dandy in a simple query inside a value of where clause. Try it on a column name or table name or custom producere name/arguments, nested subqueries etc. Alot of db-drivers/orm/you-name-it can't properly handle such scenarios thus leading to SQLi.
Don't get me wrong you definitely should use parametrization but still pay attention at your exact query. Remember parametrization purpose is not to create dynamic queries. It's to separate query and data.

Sqlinjection with integers, nice

What ide is this? Or is it a visual studio theme

the example is pretty bad, but the given advice to use parameterization is still valid in my opinion. I've seen too many young developers not adopting query parameterization, which on one hand is a huge benefit for database performance. Also imagine, if the app changes later and there will be a second method retrieving the newsletters by text search. What will a developer do? reinvent the wheel? No, he will just copy/paste the existing method and change the argument. boom!

The real question is: Who does direct SQL querying in their C# application in 2023? 🤣

Chap Nicksas 🤣🤣

Nick the code above would have pinged on a tool like checkmarx due to not using parameters and the input is not validated though technically int wont inject unwanted input

What are you doing to get your program to start so quick? Mine typically takes a while to build no matter how small the program or how much resources I have on my machine! But as you are changing code and then running, you'd have to go though that build process right? So it would normally be slow...

nobody gonna talk about raw dogging SQL queries rarther than executing a SP?

Oh, yes... at a past company, 15 years ago at this point, we absolutely had issues with SQL injection. Our software's login page could accept and execute sql. The lead software engineer of our company demonstrated how to reset passwords for all our users' in one our staging environments. Well, he THOUGHT he was on one of the staging environments. At some point he got bounced to production and didn't realize it. So, yea... that was a fun day for support 😂 As far as we knew, WE were the biggest threat to our production security, so I guess that's good at least.

Security newsletter!!

Biggest linkedin basher😃

There is actually a way to get the int version to be injectable, though it is much harder and requires access to the machine running the code. If you can set the culture settings, for example: culture.NumberFormat.NegativeSign = "1' OR 1=1 --";
Then pass in a negative number, that would output "SELECT * FROM Newsletter WHERE Id = '1' OR 1=1 --5'"

"I shouldn't talk about things I don't understand" – LinkedIn 🤣

Nice one. SQL injection in a nutshell

Good old Johnny drop tables

Sure, Id might be int now, but what if in future someone changes it to a uuid, which is a string, and then doesn't update the code to parameterize the inputs? Then you've got an SQL injection vulnerability. Assuming of course that the Id you're passing there comes from something an end user can control, e.g. a URL parameter.

linkedIn author is destroyed :D

You can just expose your api with a db user who does not have the rights to anything they shouldn't be allowed to. For example your public facing api shouldn't have access to the user table unless it's specifically an authorization service. Note: this is my opinion I also am not a security expert.

One of the biggest problems with these posts on social media is that they confuse beginner programmers, and as a result, they don't know what is right and what is not. Because a beginner who sees a post on LinkedIn assumes it is correct since they believe it is on a professional platform and no one would write nonsense there. In the past, we had it with forums and blogs, now with posts on LinkedIn and newsletters. Very good work, keep it up.

Can't say too much about the specific cases, but yes, I've had to deal with SQL injection in production code due to lack of parameterized queries.
The worst example was actually a login form where I had full access to the database through the username field. Did a demo to the product owner by creating a new table storing information schema for the entire database into it, extracting the information through another vulnerability and cleaning up after myself so nothing suspicious was left except for some log entries.
Needless to say, we fixed it quite quickly after I showcased the issue.

I do agree that parameters where you use int, float etc is not directly vulnerable to SQL injection, but using parameterized queries is still preferred in my opinion. If i'm in the habit of parameterizing all queries all the time, its far less likely I'll forget it when I need to parameterize. IMO that also applies to procedures, triggers and jobs running on the database where data might be enriched or otherwise handled by scripts etc. as that also might read a text field from one table that might cause an attack inside the procedure as well. Not as easy to exploit for a third party attacker, but I'm not entirely convinced its safe either, as it depends on the use-case and the determination of the attacker.

I have to say that I see this as an example of a wider problem that has annoyed me for decades. Practical examples in software engineering books or online that are often shoddy, clearly don't compile/execute, get tested and are frequently misleading at best. Even some of the best, "every programmer should have one" books are horrific in this regard. My advice is always test everything and understand the problem you're trying to solve before accepting advice or a solution blindly. Even the best programmers amongst us make mistakes.