-3

I wrote some code for mysql, at first i had it all parameterized. Then later a someone told me that it wasn't safe any-more. Its an old program i try to fix, but the standard input queries where not safe against injections. Though there are a lot of 'other' mysql things happening in the code, there are not much areas where new data is generated, or open user queries. So then i thought (not to endup in a battle of whats the best injection proof method), lets reformat the input strings so we never can get in such situations. I wrote a regex to always have properly formated varchar input fields

I am now using this for that:

public string AllowedAsci(string input, string symbol="*")
{
   return Regex.Replace(input, @"[^a-zA-Z0-9-+_@., ]", symbol);
}

That's basically a strict regex for basic Email and numbers, my question is can this regex be extended width other safe to use symbols.

Important update

  • The point of this question never was to raise a discussion about using mysql parameters, i knew that from the beginning, though politics are at work, here are branches of code who am i not allowed to touch. For the moment i have no intentions to get (again) into an argue at work nor to touch that code, i'l blame them in the end maybe but its political.

  • So please stay on topic what is a good regex to remove escape codes but on the other hand allow strings that dont allow for injection.

  • the regex rule does protect against all injections that i know of unless you can can proof me wrong width a better regex.
Peter
  • 2,043
  • 1
  • 21
  • 45
  • 9
    “Then later a someone told me that it wasn't safe any-more.” Someone was wrong or this is incomplete information – Ry- Apr 11 '18 at 06:36
  • 1
    Could it be that there was some kind of miscommunication between you and whoever told you that parametrized queries are *"not safe anymore"*? Can you include an example of your old code? – Manfred Radlwimmer Apr 11 '18 at 06:39
  • 1
    I'm voting to close this question as off-topic because it is based on a false premise that "parameterized code is not safe". – Nisarg Shah Apr 11 '18 at 06:42
  • 1
    Also, if this code is supposed to allow email addresses you need to include a lot more characters which would make your *Injection Proofing* kind of useless. – Manfred Radlwimmer Apr 11 '18 at 06:42
  • There are areas i may not rewrite (his code parts), – Peter Apr 11 '18 at 06:43
  • Well its not for Email, but it could be a simple Email filter as well. I started widht that, i disprove \ special characters though and no ' or " or ` – Peter Apr 11 '18 at 06:44
  • Fundamentally it sounds like the right thing to do is convince your colleague of the benefits of parameterized SQL. Given that mis-steps here open up security vulnerabilities, this is really important, and you should feel justified in escalating if necessary. The very first thing you should do is ask for more details about *why* they believe parameterization isn't safe. – Jon Skeet Apr 11 '18 at 06:46
  • i'm beyond that point, though this is still not a bad question either. Widht a proper regex one can be safe for injection. just only miss some symbols. – Peter Apr 11 '18 at 06:48
  • Just to clarify: So now you are planning to use parameterized SQL _and_ a Regex-Filter, correct? – Fildor Apr 11 '18 at 07:08
  • And just out of curiosity, I'm really interested in his sources that backup his claim - regardless of it being correct or not. It's the first time I read someone make that proposition. – Fildor Apr 11 '18 at 07:10
  • I dont wat to get into a conflict again, there should be some better regex i think that handles a bit more characters. I allready have several argues width him, but he's more a 'player' then i am, width our CEO. So in such cases i make sure he writes me, and at some day when i am in a better position then i confront him, for now I make sure he provides me digitally what he wants me to do. He's an outdated Cenior i'm just a replaceable junior....I just cannot raise the conflict again at this moment, and have to write a more simple fix for this. – Peter Apr 11 '18 at 07:35
  • 1
    @user3800527 *"I dont wat to get into a conflict again, there should be some better regex"* No, that's not the right way to go. You are risking security to avoid conflict and when something goes wrong you will be at fault. – Manfred Radlwimmer Apr 11 '18 at 07:37
  • By the way, all characters you need for basic SQL-Injection are allowed in email addresses (e.g. `'`, `"`, `-`, etc.). **Escape** your input, there is no way around this. There is **no** benefit to not using prepared statements. You should really include the *old* code, maybe you and your colleague were talking about completely different things. – Manfred Radlwimmer Apr 11 '18 at 07:39
  • ' and " are escape chars but without a slash "-" is not a escape character but like ' " _ is a escape char (if a slash could be provided), which the regex doesnt. – Peter Apr 11 '18 at 08:09
  • @user3800527 I think you may have misunderstood my last comment. Have you ever tried SQL-Injection yourself? Understanding how an attack works is essential to guarding against it. – Manfred Radlwimmer Apr 11 '18 at 08:14
  • For reference: [Little Bobby Tables](https://stackoverflow.com/questions/332365/how-does-the-sql-injection-from-the-bobby-tables-xkcd-comic-work?rq=1) – Manfred Radlwimmer Apr 11 '18 at 08:16
  • yes i've tried it, i dont know of any attacks that use a - or can do without \ / or can do without ' or can do without ", essentially not allowing a \ does not allow common escape characters like in https://dev.mysql.com/doc/refman/5.7/en/string-literals.html And although this is not for a html page, % and & are also not allowed to create special characters either. – Peter Apr 11 '18 at 08:47
  • *"i dont know of any attacks that [...] can do without '* **Exactly my point**. You can't simply filter out "the usual suspects" (`'`) because that's a valid character in email addresses! Unconventional? Certainly! But **valid**! Furthermore your logic is flawed: Sql-Injection is not just "that one attack", it comes in all sorts of flavors. If you don't let the framework escape your input, **you have to do it yourself**. – Manfred Radlwimmer Apr 11 '18 at 08:55
  • Did you read my question, all i said it was based upon a email addres syntax, *based upon* I did not say it is an Email syntax filter, one might use it for a Email but then it would not work for all email adresses, Not only in me writing this question but also its not in the code of my question. where the regex was improved to deal with my scenario. I only wondered if it could be improved more. PLease stay in scop of this question if you dont like the scope thats fine, if you dont know of a better regex its fine too dont bother this question then. The question is valid though. – Peter Apr 11 '18 at 09:15
  • 1
    I see you have updated the scope of your questions, so here is my updated answer/opinion on that: Don't **replace** the input. If something doesn't work you'll never find the error. Instead throw an error that the input contains invalid characters so the user knows that your system doesn't accept his input instead of quietly changing the input itself. If that isn't an option either: Good Luck, you'll probably be blamed for the consequences either way. If possible try to get the decision and reason behind it in writing so you have a way out when the shit hits the fan. – Manfred Radlwimmer Apr 11 '18 at 09:18
  • Just so I have a better understanding of you situation: You are forced to do something that you know is stupid and you are looking for ways to make it *less* stupid, correct? – Manfred Radlwimmer Apr 11 '18 at 09:19
  • For help with the situation that lead to this problem in the first place, I suggest you give https://workplace.stackexchange.com/ a try. – Manfred Radlwimmer Apr 11 '18 at 09:21
  • yes i'm kinda forced to for the moment but i'll join the link. – Peter Apr 11 '18 at 09:26

1 Answers1

0

I guess we could answer that it doesn't matter if your function is safe, because your coworker's premise isn't safe to begin with.

One problem is that even if you allow only "safe" characters, there are ways of encoding malicious input using the safe characters, yet it does something unsafe.

I found this example of a malicious input in this old answer: https://stackoverflow.com/a/45099/20860:

DECLARE%20@S%20VARCHAR(4000);SET%20@S=CAST(0x4445434C415 245204054205641524348415228323535292C40432056415243

(I've only shown part of the malicious payload. The point is it's possible to exploit SQL injection through a filtered set of characters you would assume is safe.)

I'll also point out that if your expression is meant to allow email addresses, it won't. There are many other characters that are legal parts of email addresses, including some like " that would result in SQL injection vulnerabilities (even without the encoding trick I mentioned above).

See a long list of test data for email address patterns here: https://fightingforalostcause.net/content/misc/2006/compare-email-regex.php

If you work for someone who would fight you instead of thank you for identifying a security flaw, you should talk to your mutual manager privately and ask to be transferred to another department. If that can't be done, start looking for a different job.

In the meantime, do what they tell you to do, but insist on a code review. Get them to approve it, and write an email to your manager with evidence that your code was approved even with the security flaws.

Bill Karwin
  • 538,548
  • 86
  • 673
  • 828
  • That was a reason for me to not include the % and the ; also not included & and ( ) ; your right about the other job, but finding one here is a bit complex, i could get a job elsewhere (big city) but my personal life binds me to a traffic jam free 20km range (my GF is often ill supporting here is my first goal). – Peter Apr 11 '18 at 15:08
  • You might just have to swallow your pride, code this function they way they want you to, and forget about it. But keep some evidence that it was their decision, for the court case. :-) – Bill Karwin Apr 11 '18 at 15:10