Looking for security vulnerabilities in database code

I've always been concerned with security and I've always stressed the importance of auditing the REAL user context not just the current user (see this post on EXECUTE AS and auditing). So, I generally try to avoid using dynamic string execution and if necessary create well tested/protected parameters (fyi – using QUOTENAME can be a fantasic solution to protectng identifiers as input parameters but it can't protect more complex strings).

Having said that, what if I'm looking at a database for the first time… just poking around trying to see if there's anything that needs further attention? I've come up with a quick query… And, while it's not going to "solve" your problem (as that's going to take some re-writing of code) or even truly verify if you're vulnerable, it gives you a "quick list" of where you should look first! If your code uses dynamic strings AND it's elevated – then start there! 

SELECT OBJECT_NAME(object_id) AS [Procedure Name],
  CASE
      WHEN sm.definition LIKE '%EXEC (%' OR sm.definition LIKE '%EXEC(%' THEN 'WARNING: code contains EXEC'
      WHEN sm.definition LIKE '%EXECUTE (%' OR sm.definition LIKE '%EXECUTE(%' THEN 'WARNING: code contains EXECUTE'
  END AS [Dynamic Strings],
  CASE
      WHEN execute_as_principal_id IS NOT NULL THEN N'WARNING: EXECUTE AS ' + user_name(execute_as_principal_id)
      ELSE 'Code to run as caller – check connection context'
  END AS [Execution Context Status]
FROM sys.sql_modules AS sm
ORDER BY [Procedure Name]

Is this enough? Anything else you'd check? What do you think?

THANKS!
kt

16 thoughts on “Looking for security vulnerabilities in database code

  1. Hmm, this code, as-is, is going to find all nested stored proc calls. Not sure if that’s a very good thing since in many cases it will return almost all of the stored procedures in the system (a lot of times I see central config procs that need to be called at the start of every other proc to get values to use). I think you could get around this easily enough. Search for either of the following:

    %EXEC(@%
    %EXEC(‘%

    That way it will find both:

    EXEC(@dynamic_sql_variable)

    and

    EXEC(‘some dynamic sql here’)

    …it should also search for %sp_executesql%.

  2. On further thought, it should also ignore white space, so that it can catch, e.g.:

    EXEC
    (
    ‘some dynamic sql here’
    )

    … that’s the way I tend to format my code, so I would escape scrutiny right away – and we wouldn’t want that!

    I think the easiest way to handle that is to do REPLACE on the ‘definition’ column and change chracters 9, 10, 13, and 32 to empty strings before doing the LIKE comparison. Probably want to encapsulate that in a CTE or derived table?

  3. Did you mean for your OR statements to be identical?

    eg:

    WHEN sm.definition LIKE ‘%EXEC (%’ OR sm.definition LIKE ‘%EXEC(%’ THEN ‘WARNING: code contains EXEC’

  4. Todd – I was trying to differentiate between a single space and no space. However, I didn’t want to put % or _ there because I didn’t want to get other words that begin with EXEC.

    Adam – Yeah, you’re right… but, I can’t come up with a better way to look for that unless I do the above (using %). But, I do like your idea of changing tabs and spaces… maybe I can come up with something better that replaces all of that with a space (just for comparison) and then checks against that. hmmm.

    THANKS!
    kt

  5. Oh, and as for sp_executesql – this only allows parameters where parameters would normally be allowed (i.e. it’s not *AS* prone to SQL Injection unless it’s combined with EXEC) so, I can certainly add that BUT, I probably wouldn’t be as concerned about that…

    SELECT OBJECT_NAME(object_id) AS [Procedure Name],
    CASE
    WHEN sm.definition LIKE ‘%EXEC%(%’ THEN ‘WARNING: code contains EXEC’
    WHEN sm.definition LIKE ‘%EXECUTE%(%’ THEN ‘WARNING: code contains EXECUTE’
    WHEN sm.definition LIKE ‘%sp_executesql%’ THEN ‘WARNING: code contains sp_executesql’
    END AS [Dynamic Strings],
    CASE
    WHEN execute_as_principal_id IS NOT NULL THEN N’WARNING: EXECUTE AS ‘ + user_name(execute_as_principal_id)
    ELSE ‘Code to run as caller – check connection context’
    END AS [Execution Context Status]
    FROM sys.sql_modules AS sm
    ORDER BY [Procedure Name]

    Here’s the latest (changing spaces to % so that I cut it down to one expression) and adding sp_executesql.

    kt

  6. Shoot… that’s not going to work. Now we’re back to the first thing you mentioned (Adam) where we’d catch all of the other stored procedures being called, etc.

    So… for now, I’ll go back to the original BUT, if there are tabs, etc. (as Adam mentioned – chars 9, 10, 13, and 32 to empty strings before doing the LIKE comparison) then it won’t get them.

    Ugh, I don’t have the bandwidth to do this now/today but I’ll try to do a quick rewrite. Working on a few other things at the same time too… sigh!

    Cheers,
    kt

  7. Well, apparently I’m in PITA mode because I gave this some thought on my commute home and I’m going to expose even more problems. :-)

    Comments:

    EXEC
    (/*run the sql*/ @sql)

    EXEC –run the sql
    (@sql)

    … and Unicode

    EXEC(N’select ”this is a seriously dynamic query!”’)

    And now I’ll go away without proposing a solution (because I haven’t figured one out yet) :-)

  8. Totally agreed Rob. Maybe that’s our best bet – create some SQLCLR function that uses REGEX to analyze the definition column.

    I’ve been trying to think of how I can use ^ but I can’t use it like % where I say any number of characters but NOT a-z or A-Z. But, I’m about to give up for today anyway. ;-) Your day has just started so maybe you’ll have this figured out before I get back up again?! ;-)

    Cheers!!
    kt

  9. You know, if T-SQL had good support for Regular Expressions, this would be much easier.

    Or if a flag got set somewhere when the procedure was created – a "potentially uses dynamic SQL" flag.

    Rob

  10. Wouldn’t it be possible to use powershell for this? It can connect to sql and it can use .net RegularExpressions, right? I’ve never tried it myself so I can’t say for sure, just a thought.

  11. When I’m reviewing code I also look for SETUSER, GRANT and CREATE LOGIN ‘s & the backward compatible sp_grantdbaccess, sp_grantlogin sprocs ect.

  12. drop procedure testme
    go
    create procedure testme
    as
    begin
    exec sp_who
    select GETDATE()
    end
    go
    SELECT OBJECT_NAME(object_id) AS [Procedure Name],
    CASE
    WHEN sm.definition LIKE ‘%EXEC%(%’ THEN ‘WARNING: code contains EXEC’
    WHEN sm.definition LIKE ‘%EXECUTE%(%’ THEN ‘WARNING: code contains EXECUTE’
    WHEN sm.definition LIKE ‘%sp_executesql%’ THEN ‘WARNING: code contains sp_executesql’
    END AS [Dynamic Strings]

    FROM sys.sql_modules AS sm
    ORDER BY [Procedure Name]

    Hhhhmmm. this proc does not have any dynamic SQL, still it comes up in warning !!!

  13. The solution is to have a T-SQL parser and decide when it really is running dangerous code or dynamic code.

    It is impossible to construct a regular expression to find all the possible alternatives. If the search is not rough, there are many difficult problems to solve.

    Greetings

Leave a Reply

Your email address will not be published. Required fields are marked *

Other articles

SQLskills SQL101: Partitioning

Continuing on our path to understanding the basics and core concepts, there’s a big topic that’s often greatly misunderstood and that’s partitioning. I know I’m

Explore

Imagine feeling confident enough to handle whatever your database throws at you.

With training and consulting from SQLskills, you’ll be able to solve big problems, elevate your team’s capacity, and take control of your data career.