9

What's a good function to use for making a string safe for inclusion in a SQL query? For example, apostrophes would need to be fixed, and there are doubtless other problems that could arise as well. I'd like a function that is rock-solid and would work given any possible input an evil-doer could devise.

Now, before the masses tell me to use query parameters and/or downvote/close this question, consider these points:

  • I am stuck using a 3rd-party library with a poorly-designed API. The API is supposed to be called as follows:

    dataObjectVariable.FindWhere("WHERE RecordID = '(your string here)'");
    

    Now, I agree with you 100% that this is not a good API as it (1) exposes internal database field names to the user which are implementation details, (2) offers no opportunity for using parameters which would avoid this problem in the first place, (3) really, you could say that SQL itself is an implementation detail and shouldn't have been exposed. But I am stuck using this API because it's required to integrate with one of the leading systems in its industry. We're not really in a position to ask them to change their API, either.

  • I searched this site for other questions pertaining to this issue, but found that answers tended to strongly suggest parameterized queries. Answers that tried to suggest writing a function to sanitize the string were often downvoted, not well thought out, etc. - I'm not sure if I trust them.

I'm only searching for strings and not other data types like numbers, dates, etc. Again, I'm 100% aware of the benefits of using parameterized queries and I wish I could use them, but I can't because my hands are tied on this one.

James Johnston
  • 9,264
  • 9
  • 48
  • 76
  • 1
    If you know the exact format of the RecordId you can refuse any other kind of text that does not matches the given formant. – msancho Oct 31 '12 at 19:15
  • 8
    Yeah..... see what you want to do here is... you want to use a parameterized query. – James Gaunt Oct 31 '12 at 19:17
  • Can the record ID contain single quotes / semi colons? If not, strip those from the string and I *think* you're injection proof. – JohnLBevan Oct 31 '12 at 19:19
  • @John that, and remove any line that starts with GO if CR/LF is allowed in the user input – RichardTheKiwi Oct 31 '12 at 19:33
  • 1
    What RDBMS are the queries running against? – Martin Smith Oct 31 '12 at 20:17
  • 1
    Bill Karwin has a [nice slideshow](http://www.slideshare.net/billkarwin/sql-injection-myths-and-fallacies) to give you ideas on what to defend against. Good luck. – Tim Lehner Oct 31 '12 at 20:45
  • 1
    If it's always a string, and it's always using MSSQL, you might be able to just replace `'` with `''`. – Matthew Oct 31 '12 at 20:51
  • 1
    FYI, unless you've stolen it or its an unsupported interface, you *are* in a position to ask them to change it. I understand that, yes, they may be unlikely to do so, and even less likely to do so in time for your project. But unless you, and other customers do ask (and forcefully), the vendor is never going to change the situation. They need to understand that their customers recognize it for the inept and unprofessional work that it is. – RBarryYoung Nov 01 '12 at 12:11
  • @msancho: there is no global. specified format of the record ID; it will vary depending on what the user's specification is. – James Johnston Nov 01 '12 at 14:20
  • @JamesGaunt: OK, how do I do that given this interface? :) – James Johnston Nov 01 '12 at 14:21
  • @MartinSmith: SQL Server, although the RDBMS is obviously an implementation detail. I doubt they'll change it anytime soon, though. – James Johnston Nov 01 '12 at 14:22

3 Answers3

4

I have to use a similar API in one of our applications. Here's the validation routine I use to manually circumvent SQL injection:



internal class SqlInjectionValidator
{

    internal static readonly List _s_keywords = new List
    {
        "alter",
        "begin",
        "commit",
        "create",
        "delete",
        "drop",
        "exec",
        "execute",
        "grant",
        "insert",
        "kill",
        "load",
        "revoke",
        "rollback",
        "shutdown",
        "truncate",
        "update",
        "use",
        "sysobjects"
    };

    private string _sql;
    private int _pos;
    private readonly Stack _literalQuotes = new Stack();
    private readonly Stack _identifierQuotes = new Stack();
    private int _statementCount;

    // Returns true if s does not contain SQL keywords.
    public SqlValidationStatus Validate(string s)
    {
        if (String.IsNullOrEmpty(s))
        {
            return SqlValidationStatus.Ok;
        }

        _pos = 0;
        _sql = s.ToLower();
        _literalQuotes.Clear();
        _identifierQuotes.Clear();
        _statementCount = 0;

        List chars = new List();

        SqlValidationStatus svs;
        while (_pos = _sql.Length)
            {
                break;
            }

            if (_statementCount != 0)
            {
                return SqlValidationStatus.SqlBatchNotAllowed;
            }

            char c = _sql[_pos];
            if (IsEmbeddedQuote(c))
            {
                _pos++;
                chars.Add(_sql[_pos]);
                _pos++;
                continue;
            }

            if (c != '\'' &&
                    IsQuotedString())
            {
                chars.Add(c);
                _pos++;
                continue;
            }

            if (c != ']' &&
                    c != '[' &&
                    c != '"' &&
                    IsQuotedIdentifier())
            {
                chars.Add(c);
                _pos++;
                continue;
            }

            switch (c)
            {
                case '[':
                    if (_identifierQuotes.Count != 0)
                    {
                        return SqlValidationStatus.MismatchedIdentifierQuote;
                    }
                    svs = DisallowWord(chars);
                    if (svs != SqlValidationStatus.Ok)
                    {
                        return svs;
                    }
                    _identifierQuotes.Push(c);
                    break;

                case ']':
                    if (_identifierQuotes.Count != 1 ||
                            _identifierQuotes.Peek() != '[')
                    {
                        return SqlValidationStatus.MismatchedIdentifierQuote;
                    }
                    svs = DisallowWord(chars);
                    if (svs != SqlValidationStatus.Ok)
                    {
                        return svs;
                    }
                    _identifierQuotes.Pop();
                    break;

                case '"':
                    if (_identifierQuotes.Count == 0)
                    {
                        svs = DisallowWord(chars);
                        if (svs != SqlValidationStatus.Ok)
                        {
                            return svs;
                        }
                        _identifierQuotes.Push(c);
                    }
                    else if (_identifierQuotes.Count == 1)
                    {
                        svs = DisallowWord(chars);
                        if (svs != SqlValidationStatus.Ok)
                        {
                            return svs;
                        }
                        _identifierQuotes.Pop();
                    }
                    else
                    {
                        return SqlValidationStatus.MismatchedIdentifierQuote;
                    }
                    break;

                case '\'':
                    if (_literalQuotes.Count == 0)
                    {
                        svs = DisallowWord(chars);
                        if (svs != SqlValidationStatus.Ok)
                        {
                            return svs;
                        }
                        _literalQuotes.Push(c);
                    }
                    else if (_literalQuotes.Count == 1 &&
                            _literalQuotes.Peek() == c)
                    {
                        _literalQuotes.Pop();
                        chars.Clear();
                    }
                    else
                    {
                        return SqlValidationStatus.MismatchedLiteralQuote;
                    }
                    break;

                default:
                    if (Char.IsLetterOrDigit(c) ||
                            c == '-')
                    {
                        chars.Add(c);
                    }
                    else if (Char.IsWhiteSpace(c) ||
                            Char.IsControl(c) ||
                            Char.IsPunctuation(c))
                    {
                        svs = DisallowWord(chars);
                        if (svs != SqlValidationStatus.Ok)
                        {
                            return svs;
                        }
                        if (c == ';')
                        {
                            _statementCount++;
                        }
                    }
                    break;
            }

            _pos++;
        }

        if (_literalQuotes.Count != 0)
        {
            return SqlValidationStatus.MismatchedLiteralQuote;
        }

        if (_identifierQuotes.Count != 0)
        {
            return SqlValidationStatus.MismatchedIdentifierQuote;
        }

        if (chars.Count > 0)
        {
            svs = DisallowWord(chars);
            if (svs != SqlValidationStatus.Ok)
            {
                return svs;
            }
        }

        return SqlValidationStatus.Ok;
    }

    // Returns true if the string representation of the sequence of characters in
    // chars is a SQL keyword.
    private SqlValidationStatus DisallowWord(List chars)
    {
        if (chars.Count == 0)
        {
            return SqlValidationStatus.Ok;
        }

        string s = new String(chars.ToArray()).Trim();
        chars.Clear();

        return DisallowWord(s);
    }

    private SqlValidationStatus DisallowWord(string word)
    {
        if (word.Contains("--"))
        {
            return SqlValidationStatus.CommentNotAllowed;
        }
        if (_s_keywords.Contains(word))
        {
            return SqlValidationStatus.KeywordNotAllowed;
        }
        if (_statementCount > 0)
        {
            return SqlValidationStatus.SqlBatchNotAllowed;
        }
        if (word.Equals("go"))
        {
            _statementCount++;
        }

        return SqlValidationStatus.Ok;
    }

    private bool IsEmbeddedQuote(char curChar)
    {
        if (curChar != '\'' ||
                !IsQuotedString() ||
                IsQuotedIdentifier())
        {
            return false;
        }

        if (_literalQuotes.Peek() == curChar &&
                Peek() == curChar)
        {
            return true;
        }

        return false;
    }

    private bool IsQuotedString()
    {
        return _literalQuotes.Count > 0;
    }

    private bool IsQuotedIdentifier()
    {
        return _identifierQuotes.Count > 0;
    }

    private char Peek()
    {
        if (_pos + 1 < _sql.Length)
        {
            return _sql[_pos + 1];
        }

        return '\0';
    }

}
Brenda Bell
  • 1,139
  • 8
  • 10
  • Not really looking for a blacklist solution; it shouldn't be necessary. – James Johnston Nov 01 '12 at 15:07
  • @JamesJohnston Perhaps I misunderstood your question, but it's unclear as to whether you can avoid it. Using your own example, you'd want to reject one of the following calls and accept the other one: dataObjectVariable.FindWhere("WHERE RecordID = '1000'; DELETE * from foo;"); dataObjectVariable.FindWhere("WHERE RecordID = '1000DELETE'"); As msancho said, if RecordId is always of a specific pattern, then you can use pattern matching. If it isn't, you should probably worry about SQL injection too. – Brenda Bell Nov 02 '12 at 16:50
0

I wound up digging some more and found this "brute-force" solution which seems to work. How to escape strings in SQL Server using PHP?

Basically, just send the string as hexadecimal instead of a string. For example, instead of sending:

WHERE RecordID = 'DEMO'

send this:

WHERE RecordID = 0x44454D4F

If the string is empty, then as a special case I just send ''

This seems reasonable safe; I can't think of an attack that would work.

In general, be careful of this solution because it might not work with non-ANSI characters. However, I found that the column I am comparing against is "varchar" and not "nvarchar", so apparently Unicode wasn't really on the agenda when they designed their database. I'd guess if it was "nvarchar", I'd need to be sending a UTF-16 string (or whatever Unicode encoding is used with nvarchar).

Community
  • 1
  • 1
James Johnston
  • 9,264
  • 9
  • 48
  • 76
0

Assuming you're expecting only plain text (for a good record), and if you can make the assumption that you will only receive normal characters (i.e. keyboard-characters), you can take your input, convert to ASCII, then limit it to the characters that you want (by providing a list of allowable characters and filtering from that), and also ensure the string length is appropriate.

Then build a new string with only your acceptable characters, and pass that string in. Once you're there, the single-quote is definitely your only concern.

If you can make those requirements, then this should work for you. There'd be no way to inject funky Unicode characters, or overflow the SQL buffer, or do any of that other crazy stuff that people do to cause problems - you'd be in 100% control of the characters that go into the query.

Joe Enos
  • 39,478
  • 11
  • 80
  • 136