6

I'm a desktop developer writing for internal users, so I'm not worried about malicious hackers, but I would like to know if there's anything they could enter when updating a value that would execute sql on the server.

The business defines their content schema and I have a CRUD application for them that doesn't have to be changed when their schema changes because the validation details are table-driven and the updates are with dynamic SQL. I have to support single quotes in their data entry, so when they enter them, I double them before the SQL is executed on the server. From what I've read, however, this shouldn't be enough to stop an injection.

So my question is, what text could they enter in a free-form text field that could change something on the server instead of being stored as a literal value?

Basically, I'm building an SQL statement at runtime that follows the pattern:

update table set field = value where pkField = pkVal

with this VB.NET code:

Friend Function updateVal(ByVal newVal As String) As Integer
    Dim params As Collection
    Dim SQL As String
    Dim ret As Integer

    SQL = _updateSQL(newVal)
    params = New Collection
    params.Add(SQLClientAccess.instance.sqlParam("@SQL", DbType.String, 0, SQL))
    Try
        ret = SQLClientAccess.instance.execSP("usp_execSQL", params)
    Catch ex As Exception
        Throw New Exception(ex.Message)
    End Try
    Return ret
End Function

Private Function _updateSQL(ByVal newVal As String) As String
    Dim SQL As String
    Dim useDelimiter As Boolean = (_formatType = DisplaySet.formatTypes.text)
    Dim position As Integer = InStr(newVal, "'")
    Do Until position = 0
        newVal = Left(newVal, position) + Mid(newVal, position)       ' double embedded single quotes '
        position = InStr(position + 2, newVal, "'")
    Loop
    If _formatType = DisplaySet.formatTypes.memo Then
        SQL = "declare @ptrval binary(16)"
        SQL = SQL & " select @ptrval = textptr(" & _fieldName & ")"
        SQL = SQL & " from " & _updateTableName & _PKWhereClauses
        SQL = SQL & " updatetext " & _updateTableName & "." & _fieldName & " @ptrval 0 null '" & newVal & "'"
    Else
        SQL = "Update " & _updateTableName & " set " & _fieldName & " = "
        If useDelimiter Then
            SQL = SQL & "'"
        End If
        SQL = SQL & newVal
        If useDelimiter Then
            SQL = SQL & "'"
        End If
        SQL = SQL & _PKWhereClauses
    End If
    Return SQL
End Function

when I update a text field to the value

Redmond'; drop table OrdersTable--

it generates:

Update caseFile set notes = 'Redmond''; drop table OrdersTable--' where guardianshipID = '001168-3'

and updates the value to the literal value they entered.

What else could they enter that would inject SQL?

Again, I'm not worried that someone wants to hack the server at their job, but would like to know how if they could accidentally paste text from somewhere else and break something.

Thanks.

abatishchev
  • 98,240
  • 88
  • 296
  • 433
Beth
  • 9,531
  • 1
  • 24
  • 43
  • 4
    "I'm not worried that someone wants to hack the server at their job" - you'd better be, disgruntled employees make pretty nasty attackers. – Matti Virkkunen Aug 27 '10 at 20:22
  • 4
    If you were one, what sql would you use? – Beth Aug 27 '10 at 20:36
  • 1
    +1 For a good question. This is something I've wondered about myself. I've seen a lot of vague assertions in text books about this being insufficient but never any explanation why. I'd be delighted if someone could answer this properly with a definitive example of user input that would cause a problem. I'm willing to bet now that this isn't going to happen though and complete non answers **which do not address the very specific question** recommending parameterised SQL will get voted to the top. – Martin Smith Aug 27 '10 at 21:00
  • Changing single quotes to doubled-single quotes might be all fine for VB.NET, but when Beth uses another language this might not work properly. It's best to get in the habit of writing proper injection-proof code using parameterized SQL instead of writing input-cleaning code that is language-specific. All it takes is forgetting to escape something out once, and your tables just got dropped. Beth is wondering if there are any other inputs that could be used to hack her database. With parameterized SQL she doesn't have to wonder. – CanSpice Aug 27 '10 at 21:46
  • 2
    @CanSpice - I understand your point but there are some circumstances in which dynamic SQL through string concatenation is inevitable. Not everything in SQL can be parameterised. For those circumstances it is important we understand why something isn't safe so we understand and can mitigate the risks. – Martin Smith Aug 27 '10 at 21:50
  • @CanSpice, from what I've read, parameterized SQL is safer, but not bulletproof. I've read that using just this approach alone isn't safe enough. Maybe that is language-specific? MS doesn't want developers writing code easily hacked, so they have posts saying nothing on the web is safe from motivated hackers. I agree. I'm not developing for the web. To what extent do these rules apply to me? Only if I stay on the desktop with internally authenticated users? – Beth Aug 27 '10 at 22:02
  • Well, you should never trust your users, no matter who they are. You don't know who the next disgruntled employee is going to be at your company. Intranet or internet, it doesn't matter. What happens if your company decides to release this product on the web? Or if one of your co-workers says "Hey, Beth made this awesome site, I'm going to use some of the code for it on my web-facing project"? – CanSpice Aug 27 '10 at 22:19
  • If someone else wants to reject a dynamic sql approach for their project, I have no problem with that. It works for me and I see the advantages and the risks are unlikely enough that I'm satisfied with the approach. If someone else wants to rewrite it without dynamic sql, that's fine, it will just take them more resource to maintain. When the business content schema changes, I don't change source code, I change data in tables. Very cheap. – Beth Aug 30 '10 at 17:39

6 Answers6

2

Regardless of how you cleanse the user input increasing the attack surface is the real problem with what you're doing. If you look back at the history of SQL Injection you'll notice that new and even more creative ways to wreak havoc via them have emerged over time. While you may have avoided the known it's always what's lurking just around the corner that makes this type of code difficult to productionize. You'd be better to simply use a different approach.

Keith Adler
  • 20,880
  • 28
  • 119
  • 189
  • I understand for a web application where updates are open to the world, this makes sense, but the primary thing my users want is to have the custom application they need to view and modify their data, which dynamic sql allows me to do without changing code when their db schema changes. – Beth Aug 27 '10 at 20:33
  • I understand. Another problem with dynamic SQL is the injection of characters that cause side effects. Parameterized queries or leveraging LINQ to SQL/Entity Framework just makes your data access fail-proof. – Keith Adler Aug 28 '10 at 15:45
2

You can also evaluate an alternative solution. Dynamic generation of SQL with parameters. Something like this:

// snippet just for get the idea
var parameters = new Dictionary<string, object>();
GetParametersFromUI(parameters);


if (parameters.ContainsKey("@id")) {
    whereBuilder.Append(" AND id = @id");
    cmd.Parameters.AddWithValue("@id", parameters["@id"]);
}
...
Jhonny D. Cano -Leftware-
  • 17,663
  • 14
  • 81
  • 103
  • I know the PK value because they selected the row and column to edit, so it's only the value they want to update that's passed directly to the server. – Beth Aug 27 '10 at 20:50
  • The reason I'm not doing this is because I want a generic approach to updates across business content. I can use this same code for any update on any field in any project. Introducing static elements means changing/compiling/testing/deploying updated code, which I want to avoid if all they do is change their content schema or validation rules. – Beth Aug 29 '10 at 14:53
1

Assuming you escape string literals (which from what you said you are doing), you should be safe. The only other thing I can think of is if you use a unicode-based character set to communicate with the database, make sure the strings you send are valid in that encoding.

Cᴏʀʏ
  • 105,112
  • 20
  • 162
  • 194
Hammerite
  • 21,755
  • 6
  • 70
  • 91
  • can you give me an example of an invalid string I can try pasting into my textbox? – Beth Aug 28 '10 at 15:05
  • If I attempt to post an invalid string here, it is liable to be corrected by the site so that invalid UTF-8 isn't stored in StackOverflow's database. So I can provide you with a recipe for an invalid string, but I can't actually post one. There are multiple ways in which a sequence of bytes can fail to be valid UTF-8; see the wikipedia page. For example, a byte value of 254 or 255 cannot appear in valid UTF-8. – Hammerite Aug 28 '10 at 20:05
  • The encoding errors you should probably be most concerned about are overlong encodings of ASCII characters, such as the sequence of bytes (192, 166) (which is an overlong encoding of a single-quote). – Hammerite Aug 28 '10 at 20:05
1

As ugly as your doubling up code is (:p - Try String.Replace instead.) I'm pretty sure that will do the job.

DaveShaw
  • 52,123
  • 16
  • 112
  • 141
  • 1
    yes, it works just fine. I keep hearing how you should never, ever, ever use dynamic sql and would like to better understand why, if I handle the obvious case. I've read that just handling this case is not enough, but not seen the example showing why. – Beth Aug 27 '10 at 20:46
1

The only safe assumption is that if you're not using parameterized queries (and you're not, exclusively, here, because you're concatenating the input string into your sql), then you're not safe.

Mark
  • 11,257
  • 11
  • 61
  • 97
  • 4
    This does not answer the question. Which is why is it not safe? Can you demonstrate this? – Martin Smith Aug 27 '10 at 20:29
  • ok, so what text could they enter that would affect the server? – Beth Aug 27 '10 at 20:29
  • 2
    See http://stackoverflow.com/questions/139199/can-i-protect-against-sql-injection-by-escaping-single-quote-and-surrounding-user for theoretical cases. I didn't say you weren't protected, I just said the only *SAFE* assumption is that you're only safe if you're using parametrized queries for ALL data coming from users. – Mark Aug 27 '10 at 21:55
  • @Mark, thanks for the link. I knew I wasn't the first person to ask, but couldn't find the relevant post. – Beth Aug 27 '10 at 22:13
  • 1
    @Mark - Good link. The Buffer truncation doesn't apply here though. The MySQL backslash thing doesn't apply. The Unicode one is very interesting. I've tested on SQL Server and it is indeed true that it will convert the ` ʼ` [U+02BC](http://www.fileformat.info/info/unicode/char/2bc/index.htm) character to a regular apostrophe. This relies on a cast to char/varchar (as opposed to Unicode) happening at some point though which won't happen when executing a regular query. – Martin Smith Aug 27 '10 at 22:54
  • Martin, are you saying if I paste your char into my textbox the VB code won't recognize it as a single quote, but the server will? This is exactly the kind of answer I'm looking for. I want to test if they accidentally paste something, even if they're disgruntled and hunting for ways to break/test the server. I'll test it Mon. – Beth Aug 28 '10 at 15:02
  • @Beth - Yes it would get converted to a regular apostrophe but only if you cast the user input to `char` at some point. If it's kept in Unicode this issue won't arise. – Martin Smith Aug 28 '10 at 16:50
  • Thanks, @Martin Smith. I'm not casting anything else, just passing the sql through, so I think I should have everything covered, as some others have said. – Beth Aug 29 '10 at 14:50
1

You never never ever never want to build a SQL statement using user input that will be then directly executed. This leads to SQL injection attacks, as you've found. It would be trivial for someone to drop a table in your database, as you've described.

You want to use parameterized queries, where you build an SQL string using placeholders for the values, then pass the values in for those parameters.

Using VB you'd do something like:

'Define our sql query'
Dim sSQL As String = "SELECT FirstName, LastName, Title " & _
                     "FROM Employees " & _
                     "WHERE ((EmployeeID > ? AND HireDate > ?) AND Country = ?)"

'Populate Command Object'
Dim oCmd As New OledbCommand(sSQL, oCnn)

'Add up the parameter, associated it with its value'
oCmd.Parameters.Add("EmployeeID", sEmpId)
oCmd.Parameters.Add("HireDate", sHireDate)
oCmd.Parameters.Add("Country", sCountry)

(example taken from here) (also not I'm not a VB programmer so this might not be proper syntax, but it gets the point across)

abatishchev
  • 98,240
  • 88
  • 296
  • 433
CanSpice
  • 34,814
  • 10
  • 72
  • 86
  • 2
    @canspice - I don't think you read the question properly. It is not trivial because by escaping the single quote what is sent should be treated as data. – Martin Smith Aug 27 '10 at 20:38
  • 2
    Actually, I'm trying to find the attack. The SQL in the question does not cause one. Can you produce sql that does? – Beth Aug 27 '10 at 20:38
  • Like I said, I'm not a VB programmer. What does `_formatType = DisplaySet.formatTypes.text` do? Does it only set `useDelimiter` if the input is text? I guess what I'm asking is "how does `useDelimiter` get set to false?" – CanSpice Aug 27 '10 at 21:24
  • if they're expected to enter a numeric value, useDelimiter is false. My displaySet object contains a formatTypes enum with three values, numeric, text, and memo, which determine how to format the update statement. A memo type is stored as a TEXT type on the sql server, not varchar(), so the syntax is different. Numeric vales don't need the surrounding delimiter. – Beth Aug 27 '10 at 21:37
  • Okay, so if they're expected to enter a numeric value, then the string "0; drop table OrdersTable--" just got you in trouble because your SQL statement is now `Update caseFile set numberField = 0; drop table OrdersTable--`. – CanSpice Aug 27 '10 at 21:49
  • @CanSpice You're assuming I don't validate numeric input, which I do. Not only does it have to be numeric, but also within the range the biz requires. This isn't a web app, so I can ignore invalid keystrokes. – Beth Aug 28 '10 at 14:59