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

Getting programming advice from LinkedIn is like getting your news from Facebook.

You can almost _feel_ the anger emanating from Nick. I can relate.

Well, I' only a few minutes in and I've just seen the code snippet and I look forward to seeing how a C# Integer can be used for an injection attack...... I love what you're doing with these videos, sick of seeing authoritatively presented nonsense by people who clearly don't have the first clue about what they're talking about. They're clearly just regurgitating things they've read without understanding

well, I believe that the guy who created this example simply forgot to change int to string, and that his idea was just to say to people to use parameters. But I agree, he should be more careful in public places.

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'"

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.

Have to say I absolutely LOVE this code cop series. There's SO much bad advice out there. I'd love to see a rating system for advice but that would also be problematic since there are an infinite number of opinions on code so a rating system would be difficult. Thank you for keeping this series going.

Parameterization is important for another reason. In MsSQL (and in different databases it is similar, I suppose): it makes SQL reuse the execution plan, which is way more efficient.

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

In Germany, we say "dangerous half-knowledge". Nonetheless i would use a parameterized query even for the integer argument to have a constant query string for various performance reasons. You also have to think about other effects: What if the conversion to string introduces thousands separators or other formatting artifacts? The tip from LinkedIn is not wrong, but the reason is vague.

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

I actually learned something, and learned i was conflating two ideas as well. Thanks for the clarification.

I really hate that people still think that input sanitation is a real solution to sql injections. Don't get me wrong, it can be, but it's not a good solution if you want to be sure. Instead, people should understand that the problem with sql injections is, that the user input can influence the parsing of the query, because adding the user input happens before the parsing of the query. And the obvious solution to that is, that you invent a way to parse the "unfinished" query before the user input is added to is, and that's exactly what parameterization is. Now, there are situations when you cannot do that, where the query actually dynamically depends on the user input, but not in a way that can be exploitet by the user.

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

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).

Oh Bobby Tables, is that you? 😂

Coming from back in the day of Classic ASP, SQL injection was a much bigger issue. Plain old ADO was capable of parameterized queries, but old legacy "do it fast" code was often guilty of ugly string concats. As you demonstrated, once C# as a statically typed language came on the scene, some of those concerns were indirectly addressed. Ultimately using parameters to properly escape and sanitize values was the right answer. Appreciate you going through and demonstrating how the attack actually works. Not everyone has seen the horrors of what an adhoc string concat query can do to your database.

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!

There is also a lesson around least privilege security. I have seen systems where the application could make schema changes, like drop table :(

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

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...

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.

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.

The correct way to handle this situation is NOT merely to parameterize queries, as the LinkedIn author awkwardly stated, it is to use a database account that cannot modify data in the first place. Why should a query ever use a DB user that can alter data?

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

Problems with the first snippet 1) text to int implicit type conversion can cause performance issues 2) database server can't reuse query plans when the value is hardcoded in the sql query string 3) select * instead of explicitly specifying only the required columns

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)

Using stored procedures to protect against SQL Injection is something I've heard before, but you can still be vulnerable, and I've seen many live examples of that when developers are concatenating strings in T-SQL, meaning that they do proper parameterization from the C# code, but then take those parameters to generate SQL without parameterization before running sp_executesql. I think using stored procedures actually makes you more likely to introduce SQL Injection attacks since you are limited to what T-SQL offer and you many times need to resort to string concatenation and forget about using parameterization for sp_executesql. And I'm not sure security scanning tools like SAST will be able to help (like they can with C# code) since you might store your T-SQL code somewhere that the tool won't find it..

Bobby Tables would be VERY upset.

Thanks, Chap. This was actually IMO the best video in the series because it highlights something very fundamental whereas the first ones were more about syntax

As an EFcore user I don't really think about sql injections. But I'm still glad I learned about them and how to avoid them in university.

The code example as it doesn't suffer of SQL injections YET. But, as code grows, some junior developer might step in and refactor this method (say, extract WHERE part following specification pattern), then them there is a place for human error. Explicitly wrapping input parameters to prevent the issue is what the SqlParameter made for and it makes it clear even for juniors.

Your third db entry made me chuckle, a proper oldskool LOL 😂

I have actually found cases where bit or integer fields actually can benefit from string interpolation or concatenation instead of parameterized. This can happen if the database is very unbalanced between the different values. Since a parametrized query shares the first query plan created, if that plan is created based on an edge case value that for example only matches one or a few lines (the opposite is also bad but usually not ad bad), another query later that matches several thousand lines, the query plan can be very bad for the second query taking many seconds or even minutes to run instead of fractions of a second if the query plan is built based on a query with multiple matches. If you interpolate that value it will instead result in different query plans each optimized for the number of hits for that value. If you have very many different values this can still be a problem since you pollute the query cache and in that case you might need another approach, like identifying the different groups of values and deliberately create different queries, or maybe ad an extra field for the type of value that you then interpolate into the query to force different query plans. Now, this is probably quite rare in most cases so generally you should definitely use parameters and spare the generation of multiple query plans and gaining performance. But in a few cases we found that interpolating some specific values (it was always bit or integer fields in our case) we improved overall performance 10-50 times :) SO if you have a query that sometimes start taking a lot of time and other times is very fast despite using the same input data it could be that the query plan was created based on some edge case value that does not represent the normal queries. Or if you have a query that is generally fast but for one or a few values becomes very slow, then it could be worth investigating this and maybe interpolating some values can be the solution. As for having to deal with SQL injection, I had the benefit of getting introduced to it by a security expert that asked me what WOULD happen if he tried a query so I got to fix the problem before anyone abused it (at least so far as we knew at the time). That was some 30 years ago and as far as I know I have not created a vulnerable query since then ;)

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.

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...

Nicely presented and great examples, thank you!

Beautifully demonstrated. Totally agree that this was irresponsible advice, being misleading or imprecise when it comes to this stuff is a big deal, SQL injection is a real thing in existing production codebases. You have to understand it and know what you're doing when remediating it. Junk examples like this are not helping. Thank you for calling this stuff out and showing a correct approach

I thought you were about to blow my mind by explaining how injection is done with an integer parameter. haha. That's what I get for not having coffee yet.

I loved when you made the third newsletter 😂

If someone can inject SQL into your code through a C# int parameter then you have bigger issues than that.

I get emails all the time from scam "security researchers" saying: Your app has SQL Injection issue, you send us 5000US we tell you fix. Half the time they try a DDoS as their proof of an issue.

We need another channel for Chap Nicksas please.

This one kinda got me because i was homing in from the go on the parameter being an int and my brain was trying to come up with a scenario to turn that into injection. By the time it got to the description lightbulb went on and "oh, he messed the example...lol". Anyway, call me paranoid but i do integrity checks on my methods, pass as parameters to stored procedures and the SP checks it's inputs because... dumb mistakes happen and dumb people happen even more. And add sane constraints into the table definition, can save you a lot of headaches.

To make the example exploitable you need to convince some country to adopt base85 or something like that for representing integer numbers and then .NET to implement that change when converting integers to string under that culture. I'd image it would be pretty hard to do in practice.

Please do a video on ConfigureAwait

This one really grindededed my gears

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.

Great explanation!

Stored procedures (done properly) are another usedul layer to help prevent SQL injections. Even if someone was able to inject something, in theory the user you're running the SP as wouldn't have access to SELECT from a table or DROP one

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.

It would be good example if they would talk about query plans (each integer would generate a new plan) but not as SQL injection as Nick said :D Of course... it depends for what purposes you made this query, because it can also evolve as parameter sniffing, but most of the ppl don't even know what this is :D

The third entry in the database was fucking savage 😂

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 can certainly imagine a situation in which a developer that does understand security wrote a quick LinkedIn post, but the code that he happened to quickly copy used an integer. In general, the core of the advice is to parameterize queries, which of course is a sound strategy. That said, it is much more likely that you are right and that he has no clue about security because in my experience that is true for almost 100% of developers. Still though, the core advice about parameterizing queries is sound.

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#.

It's still good advice to always parameterize things if you can, for performance reasons and to prevent you/other devs from making mistakes if the API changes. But yeh, there is no actual in the moment risk of an SQL injection for putting an integer straight into the query.

6:29 I get what you’re saying, and get that the example using int is a bad choice, but the advice is still good for a couple of reasons. First, if some future dev who doesn’t know better decides to copy and repurpose the safe interpolated or concatenated code (because of the int) for another query that takes some other data type like a sting… boom! SQL injection! Just because in one place it is safe to use interpolation or concatenation doesn’t mean you should do that. Second, each time a dev looks upon that method, they have to reevaluate “Is this safe”? And go through the mental gymnastics to validate that it is. When instead, if it has parameterised placeholder, it’s obvious. It’s better to err on the side of caution. It’s better to make the devs job easier and reduce the cognitive load. Think of it as a code standard that should be enforced by a linter. Just because you can, doesn’t mean you should. It’s poor workmanship.

Bruh the third record in the DB was vicious lmaooooo

I think the main problem would be other developers “reusing” the same code and not using parameters in other methods where the input was a string. So unless there was a very strong case for that method not using parameters, I would flag that as a potential problem

The original poster definitely saw something about concatenating being bad, but didn't realise that it applied to strings (which could be anything) rather than ints. No idea why someone would then share it as if they are a genius saving us from SQL injection woes...

Yes In multiple cases analysis tools employed by or security wonks flagged things in our applications as injectable, This was followed by numerous meetings with those same wonks to justify why they were not exploitable. This was followed by a paper trail of accepted "MEAT SPACE" mitigations. Which of course lasted until that same mindless engine that flagged the code initially was updated where the whole process started again. A process that continues to this day. So unfortunately, my team has had to go in and convert everything to using parameters in all cases just so we don't have to go through all this wasteful nonsense every few months... Endlessly frustrating to be sure... Kudos to you...

I prefer drop table instead of returning everything. Some nasty person registered a sql injection as a company name and made some headache to the registration office

Security newsletter!!

"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. :)

The funny thing is that the wrong part is not exploitable, and the author of the post doesn't really understand what SQL injection really is 🤣 They need to look into "Exploits of a Mom" featuring little Bobby Tables. 😎

I've seen simple php websites where user input is directly used for a search. These were small tools for internal usage. A small pdf generator, for example. I think the full select statement may have been built on the client side. I didn't have the heart to tell it to the (non-professional) developer.

as soon as i see the code snippet, i said wait, this is not injectable.

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).

So tired of seeing all these clowns on linkedin sharing nonsense. They are not able to give good examples even. They don't understand how to parameterize a query. And yes, query injection is not something to joke about. One could delete all your data, or worse: steal it. One could discover the whole dB schema using this. If it's a shop then someone could just change prices. Not looking to give you ideas, but to make you aware of the consequences of not understanding what you do. Thank you for this series.

Nick said it.

Always leave IT/Cyber-security to people smarter than yourself - that's my LinkedIn Advice

@2:20 are you saying you are a security expert?

Please share more security tips and how to protect against malicious attacks

Sql injection is just the tip of the iceberg... Unfortunately, I see a lot of devs that think their code is safe because they're using EF.

While I agree that the advice is a bit odd because it´s not actually SQL injectable. The example might be flawed, but the core advice is good. Especially, when talking to 'not so great' devs I try to give them black and white answers to things when feasable. If they wanna dive deep great! But otherwise, you know. No harm done by doing parameterized queries everywhere. Personally working in a company that is like 50/50 mixed either programmers (meaning once that actually know what they are doing) and domain experts for the field that also program a bit. Without the latter the program would not be possible either. But it means a lot of the stuff they interact with has to be designed to be mostly idiot prove and often worked over by somebody else regardless. Black and white answers are flawed in programming but if the drawback isn´t really there (like in this example) they work out just fine and are followed more likely.

While I agree with you on this 100%, I still think the Linkedin advice is sound. And its mainly because of beginners and junior developers. You can have developers which try to be "smart" by doing the integer and saying "yeah there is no way SQL inject here!" and then later in the project some non-knowing dev changes that to a string because they didn't read the function far enough or understood it and bam, you got a SQL injection. Just because of this I would change the advice in this youtube video "Absolutely no matter what the reason is - use Parametized queries, please!". Don't make clever solutions, use proper standards :)

Nice one. SQL injection in a nutshell

and they say PHP is not secure seems like the problem is between the chair and the monitor the first one for example is funny cause your method accepts Int variable, if you pass a string you have a runtime exception or whatever C# throws

I was watching just waiting for that semicolon.

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

Hey Nick, I'm curious. Have some of the bad advise authors reached to you and give you some feedback etc? Or have you checked if after publishing video, they deleted their posts?

Fortunately even though the example and the explanation are bad the advice is good and (almost) fully correct. It's worth mentioning that string interpolation can be a good way to prevent SQL injection if such parameter is handled correctly by the method accepting it. EF Core has a method FromSql and another one FromSqlInterpolated. Both are doing it the right way. Be careful though. If you make a string out of it yourself before calling the method you will end up with a potential for SQL injection. Using parameters is always safe.

Hi Nick, but in any case the validation should be done either in presentation layer and in data access layer?

What is the Software you/he are/is using?

What I got out of this video isn't that it's bad advice, because in the end the advice was the same: use parameters. The problem with this LinkedIn advice was that it was just a bad example. Is the example use the string in the first place, most of is moot

Usually, my preferred way of doing SQL injections is to add a double dash to the end of my injection, treating anything after that as a comment.

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….

One of the first jobs I had handled sql injection by rewrapping every parameter in single quotes. Didn't matter what it was, it would get another set of double quotes to 'prevent' the rampant drop table injection. They did a lot of things in a very... interesting way. Sometimes I wish I could go back to that position just to fix those problems with the experience I have now.

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!

Had a laugh at this one. Thanks for all the videos :)

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

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. 🧐 🤦‍♂

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!

LinkedIn guy : don't talk about things you don't understand 😂

unfortunately, people that do not understand anything, do not stop to talk and give bad advices. Sometimes that is very toxic.

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

ever since i saw your first episode of copecop i wondered how long this example would come by. When i saw this article on LI i had a hard time not to laugh out loud because of how bad of an example the example code was, along with how the parameter code was still in the bad example. For me this was a clear example of OP creating content just for the sake of creating content interaction, without actually putting any sort of effort in. It is sad that LI has such a strong bias to content interaction creates content interaction rather than content quality creates content interaction

When you copy paste an example from untyped PHP.

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!