Post reply

A "safe" replacement for xp_CmdShell - for test purposes

  • August 6, 2018 at 3:34 am

    #414811

    Hi,

    I'm currently reverse-engineering a system which makes use of xp_cmdshell to move files around a network.

    As part of a rewrite to remove dependency on xp_cmdshell, we'd like to have a "safe" version of xp_cmdshell available to our databases which is a "log-only" form of the real system stored proc i.e. no execution occurs, just the command passed is stored and logged. Somehow all calls to xp_cmdshell should be routed / diverted to the "safe" version. So far my thoughts for options are:

    • Modify master.sys.xp_cmdshell, and let all existing stored procs call it as they currently do.
      - I've read enough articles to know this is a BAD idea to modify system procs.
    • Create a "safe" stored proc: xp_cmdshell_safe which takes the same parameters as xp_cmdshell, then global-replace all calls in my development database to call the safe version instead of the real version
      - possible, but I haven't yet worked out how to either
             script the original,
             or to make a replacement inside master that can be called like a system stored proc
    • Create a "safe" local version inside each one of my databases (there are several), then the same global-replace as above to make all the operational stored procs call my safe version
      - seems a bit wasteful to make multiple copies, but would probably work
    • Other - i.e. reach out to the community for suggestions!

    At the moment we're focusing on the 2nd or 3rd option (a safe copy) but would like to ask:
    - does this sound like the right way to be going
    - how to get the DDL script of the original in order to write a replacement with the same input params
    - whether it's better to make a new system proc in master, or local one in each DB.

    Many thanks,
        Glen.

  • August 6, 2018 at 4:18 am

    #2000383

    glen205 - Monday, August 6, 2018 3:34 AM

    Hi,

    I'm currently reverse-engineering a system which makes use of xp_cmdshell to move files around a network.

    • Modify master.sys.xp_cmdshell, and let all existing stored procs call it as they currently do.
      - I've read enough articles to know this is a BAD idea to modify system procs.
    • Create a "safe" stored proc: xp_cmdshell_safe which takes the same parameters as xp_cmdshell, then global-replace all calls in my development database to call the safe version instead of the real version
      - possible, but I haven't yet worked out how to either
             script the original,
             or to make a replacement inside master that can be called like a system stored proc
    • Create a "safe" local version inside each one of my databases (there are several), then the same global-replace as above to make all the operational stored procs call my safe version
      - seems a bit wasteful to make multiple copies, but would probably work
    • Other - i.e. reach out to the community for suggestions!

    At the moment we're focusing on the 2nd or 3rd option (a safe copy) but would like to ask:
    - does this sound like the right way to be going
    - how to get the DDL script of the original in order to write a replacement with the same input params
    - whether it's better to make a new system proc in master, or local one in each DB.

    Many thanks,
        Glen.

    Quick question, why do any of this if you can safely manage xp_cmdshell with proper permissions?
    😎

  • August 6, 2018 at 4:30 am

    #2000384

    Eirikur Eiriksson - Monday, August 6, 2018 4:18 AM

    glen205 - Monday, August 6, 2018 3:34 AM

    Hi,

    I'm currently reverse-engineering a system which makes use of xp_cmdshell to move files around a network.

    • Modify master.sys.xp_cmdshell, and let all existing stored procs call it as they currently do.
      - I've read enough articles to know this is a BAD idea to modify system procs.
    • Create a "safe" stored proc: xp_cmdshell_safe which takes the same parameters as xp_cmdshell, then global-replace all calls in my development database to call the safe version instead of the real version
      - possible, but I haven't yet worked out how to either
             script the original,
             or to make a replacement inside master that can be called like a system stored proc
    • Create a "safe" local version inside each one of my databases (there are several), then the same global-replace as above to make all the operational stored procs call my safe version
      - seems a bit wasteful to make multiple copies, but would probably work
    • Other - i.e. reach out to the community for suggestions!

    At the moment we're focusing on the 2nd or 3rd option (a safe copy) but would like to ask:
    - does this sound like the right way to be going
    - how to get the DDL script of the original in order to write a replacement with the same input params
    - whether it's better to make a new system proc in master, or local one in each DB.

    Many thanks,
        Glen.

    Quick question, why do any of this if you can safely manage xp_cmdshell with proper permissions?
    😎

    No problem - I think I can clarify.
    The calls to xp_cmdshell use dynamically-built commands and the business wants to mitigate against SQL injection risk.

    We want to replace the calls with a different file-copy mechanism and would like to catalog / log the command output without executing the commands. This will help us with testing the replacement.

  • August 6, 2018 at 6:14 am

    #2000397

    glen205 - Monday, August 6, 2018 3:34 AM

    Hi,

    I'm currently reverse-engineering a system which makes use of xp_cmdshell to move files around a network.

    • Modify master.sys.xp_cmdshell, and let all existing stored procs call it as they currently do.
      - I've read enough articles to know this is a BAD idea to modify system procs.
    • Create a "safe" stored proc: xp_cmdshell_safe which takes the same parameters as xp_cmdshell, then global-replace all calls in my development database to call the safe version instead of the real version
      - possible, but I haven't yet worked out how to either
             script the original,
             or to make a replacement inside master that can be called like a system stored proc
    • Create a "safe" local version inside each one of my databases (there are several), then the same global-replace as above to make all the operational stored procs call my safe version
      - seems a bit wasteful to make multiple copies, but would probably work
    • Other - i.e. reach out to the community for suggestions!

    At the moment we're focusing on the 2nd or 3rd option (a safe copy) but would like to ask:
    - does this sound like the right way to be going
    - how to get the DDL script of the original in order to write a replacement with the same input params
    - whether it's better to make a new system proc in master, or local one in each DB.

    Many thanks,
        Glen.

    I believe Option 2 is possible.  It seems like overkill though.  It's pretty easy to write stored procedures that use xp_CmdShell safely.  I have a presentation on the subject with instructions and a "DOS Injection" checker.  I'll see what I can do after work tonight.

    --Jeff Moden

    RBAR is pronounced "ree-bar" and is a "Modenism" for Row-By-Agonizing-Row.
    First step towards the paradigm shift of writing Set Based code:
    ________Stop thinking about what you want to do to a ROW... think, instead, of what you want to do to a COLUMN.

    Change is inevitable... Change for the better is not.

    Helpful Links:
    How to post code problems
    How to Post Performance Problems
    Create a Tally Function (fnTally)

  • August 7, 2018 at 12:07 am

    #2000547

    Option 2 is probably what I'd go with initially to get a feel for what is going on in the code. No need to figure out what xp_cmdshell actually does though - just wrap it:
    Use Master
    Go
    Drop Table If Exists cmdshelllog
    Create Table cmdshelllog(Id Int Identity(1,1), LogTime DateTime Not Null Default(GetDate()), Cmd VarChar(Max))
    Go
    Create Or Alter Procedure sp_cmdshell_safe @cmd Varchar(Max)
    As
    Begin
     Declare @result Int
     Insert Into cmdshelllog(Cmd) Values(@cmd)
     Exec @result = xp_cmdshell @cmd
     Return IsNull(@result, 0)
    End
    Go
    Use Scratch -- switch to some other database
    Go
    Exec sp_cmdshell_safe 'dir /p'
    Select * From Master.dbo.cmdshelllog

    By creating a stored proc in Master whose name begins with sp_ SQL Server will automatically look there and call it if you're in another database (which is why you shouldn't normally use an sp_ prefix) so you don't need to create copies in every database. It may be worth augmenting it with additional defaulted parameters like calling procedure name or other details you may want to log as well - for example you can do an initial find-replace with the code as-is then modify procedures to pass in Object_Name(@@ProcID) as you start debugging to help identify what is doing what and when.

    While it certainly is possible to use xp_cmdshell safely, it does make sense to centralise the functionality of doing things like copying files into things like a central CopyFiles procedure, even if that just uses xp_cmdshell behind the scenes - if for no other reason than making it simpler for devs to work with (I've seen some nasty code for parsing dir output that I wouldn't want copied all over the place!)

  • August 7, 2018 at 5:51 am

    #2000561

    Jeff Moden - Monday, August 6, 2018 6:14 AM

    glen205 - Monday, August 6, 2018 3:34 AM

    Hi,

    I'm currently reverse-engineering a system which makes use of xp_cmdshell to move files around a network.

    • Modify master.sys.xp_cmdshell, and let all existing stored procs call it as they currently do.
      - I've read enough articles to know this is a BAD idea to modify system procs.
    • Create a "safe" stored proc: xp_cmdshell_safe which takes the same parameters as xp_cmdshell, then global-replace all calls in my development database to call the safe version instead of the real version
      - possible, but I haven't yet worked out how to either
             script the original,
             or to make a replacement inside master that can be called like a system stored proc
    • Create a "safe" local version inside each one of my databases (there are several), then the same global-replace as above to make all the operational stored procs call my safe version
      - seems a bit wasteful to make multiple copies, but would probably work
    • Other - i.e. reach out to the community for suggestions!

    At the moment we're focusing on the 2nd or 3rd option (a safe copy) but would like to ask:
    - does this sound like the right way to be going
    - how to get the DDL script of the original in order to write a replacement with the same input params
    - whether it's better to make a new system proc in master, or local one in each DB.

    Many thanks,
        Glen.

    I believe Option 2 is possible.  It seems like overkill though.  It's pretty easy to write stored procedures that use xp_CmdShell safely.  I have a presentation on the subject with instructions and a "DOS Injection" checker.  I'll see what I can do after work tonight.

    I couldn't find where I uploaded my presentation on this site but SlideServe was kind enough to make an illegal copy.  You can find it here.  The "meat" of how to use it safely starts on slide #53.  All of the stuff before that is a progression of slides that hopefully leads the reader into understanding that it is actually safe to enable and use if you do it right.  Like I said, one easy way to do it right starts on page #53 and includes coded examples and also how to easily protect against DOS Injection especially when it comes to dynamic file names.

    https://www.slideserve.com/garima/disabling-xp-cmdshell-is-it-really-a-best-practice

    --Jeff Moden

    RBAR is pronounced "ree-bar" and is a "Modenism" for Row-By-Agonizing-Row.
    First step towards the paradigm shift of writing Set Based code:
    ________Stop thinking about what you want to do to a ROW... think, instead, of what you want to do to a COLUMN.

    Change is inevitable... Change for the better is not.

    Helpful Links:
    How to post code problems
    How to Post Performance Problems
    Create a Tally Function (fnTally)

  • August 7, 2018 at 9:52 am

    #2000639

    andycadley - Tuesday, August 7, 2018 12:07 AM

    Option 2 is probably what I'd go with initially to get a feel for what is going on in the code. No need to figure out what xp_cmdshell actually does though - just wrap it:
    Use Master
    Go
    Drop Table If Exists cmdshelllog
    Create Table cmdshelllog(Id Int Identity(1,1), LogTime DateTime Not Null Default(GetDate()), Cmd VarChar(Max))
    Go
    Create Or Alter Procedure sp_cmdshell_safe @cmd Varchar(Max)
    As
    Begin
     Declare @result Int
     Insert Into cmdshelllog(Cmd) Values(@cmd)
     Exec @result = xp_cmdshell @cmd
     Return IsNull(@result, 0)
    End
    Go
    Use Scratch -- switch to some other database
    Go
    Exec sp_cmdshell_safe 'dir /p'
    Select * From Master.dbo.cmdshelllog

    By creating a stored proc in Master whose name begins with sp_ SQL Server will automatically look there and call it if you're in another database (which is why you shouldn't normally use an sp_ prefix) so you don't need to create copies in every database. It may be worth augmenting it with additional defaulted parameters like calling procedure name or other details you may want to log as well - for example you can do an initial find-replace with the code as-is then modify procedures to pass in Object_Name(@@ProcID) as you start debugging to help identify what is doing what and when.

    While it certainly is possible to use xp_cmdshell safely, it does make sense to centralise the functionality of doing things like copying files into things like a central CopyFiles procedure, even if that just uses xp_cmdshell behind the scenes - if for no other reason than making it simpler for devs to work with (I've seen some nasty code for parsing dir output that I wouldn't want copied all over the place!)

    Thanks for this andycadley, I don't suppose I really wanted to DDL script the original stored procedure... Thinking about it I only ever needed to know the number + type of its parameters to create a wrapper as you've done here!

  • August 7, 2018 at 9:54 am

    #2000641

    Jeff Moden - Tuesday, August 7, 2018 5:51 AM

    I couldn't find where I uploaded my presentation on this site but SlideServe was kind enough to make an illegal copy.  You can find it here.  The "meat" of how to use it safely starts on slide #53.  All of the stuff before that is a progression of slides that hopefully leads the reader into understanding that it is actually safe to enable and use if you do it right.  Like I said, one easy way to do it right starts on page #53 and includes coded examples and also how to easily protect against DOS Injection especially when it comes to dynamic file names.

    https://www.slideserve.com/garima/disabling-xp-cmdshell-is-it-really-a-best-practice

    Thanks Jeff, we went through the whole presentation and were very interested in your analysis of the risk of xp_cmdshell. I will be doing some analysis comparing the already-proposed replacement of these stored procs against the potential cost of just wrapping them and tightening up the users' permissions.

    Many thanks!

Viewing 8 posts - 1 through 7 (of 7 total)

You must be logged in to reply to this topic. Login to reply