`Get-DbaHelpIndex` - SQL Injection And Remove Unused Code

[mattcargile]

Verified issue does not already exist?

I have searched and found no existing issue

What error did you receive?

Errors weren't received. @TableName parameter should be parameterized.

https://github.com/dataplat/dbatools/blob/cb554f576758b915252a2a5b853aea9ac443b6dc/functions/Get-DbaHelpIndex.ps1#L144-L148

$SizesQuery2005 variable and corresponding if { } logic, Write-Message, etc should be removed because only version 10 and above is supported.

https://github.com/dataplat/dbatools/blob/cb554f576758b915252a2a5b853aea9ac443b6dc/functions/Get-DbaHelpIndex.ps1#L548

https://github.com/dataplat/dbatools/blob/cb554f576758b915252a2a5b853aea9ac443b6dc/functions/Get-DbaHelpIndex.ps1#L1000

https://github.com/dataplat/dbatools/blob/cb554f576758b915252a2a5b853aea9ac443b6dc/functions/Get-DbaHelpIndex.ps1#L1015-L1020

Because of the usage of Connect-DbaInstance at Line 1004 denoting the -MinimumVersion 10, the above if code section isn't necessary.

Steps to Reproduce

# General Usage
Get-DbaHelpIndex -SqlInstance sql -Database db -ObjectName 'dbo.t'
# SQL Injection
Get-DbaHelpIndex -SqlInstance sql -Database db -ObjectName 'dbo.t''; select 1 as c --'
# Produces below error.
# WARNING: [##:##:##[Get-DbaHelpIndex] Cannot process [db] on [sql] | Cannot convert null to type "System.DateTime".

Please confirm that you are running the most recent version of dbatools

1.1.142

Other details or mentions

For the SQL Injection issue, maybe Invoke-DbaQuery -SqlParameter could be used and the below section could include some if { } logic to include or exclude the predicate. Additionally, I'd like to see the removal of the CASE expression for a more ideal plan because this iteration produces an index scan in a small test.

https://github.com/dataplat/dbatools/blob/cb554f576758b915252a2a5b853aea9ac443b6dc/functions/Get-DbaHelpIndex.ps1#L274-L276

https://github.com/dataplat/dbatools/blob/cb554f576758b915252a2a5b853aea9ac443b6dc/functions/Get-DbaHelpIndex.ps1#L387-L390

For the 2005 code path, it should be fairly straight forward. I think $indexesQuery can be replaced with $SizesQuery or vice versa or something even more general.

What PowerShell host was used when producing this error

PowerShell Core (pwsh.exe)

PowerShell Host Version

Name                           Value
----                           -----
PSVersion                      7.3.1
PSEdition                      Core
GitCommitId                    7.3.1
OS                             Microsoft Windows 10.0.19045
Platform                       Win32NT
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0…}
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1
WSManStackVersion              3.0

SQL Server Edition and Build number

Microsoft SQL Server 2005 - 9.00.5324.00 (Intel X86) 
    Aug 24 2012 14:24:46 
    Copyright (c) 1988-2005 Microsoft Corporation
    Standard Edition on Windows NT 6.1 (Build 7601: Service Pack 1)

Microsoft SQL Server 2019 (RTM) - 15.0.2000.5 (X64) 
    Sep 24 2019 13:48:23 
    Copyright (C) 2019 Microsoft Corporation
    Express Edition (64-bit) on Windows 10 Enterprise 10.0 <X64> (Build 19045: ) (Hypervisor)

Microsoft SQL Server 2014 (SP3) (KB4022619) - 12.0.6024.0 (X64)
        Sep  7 2018 01:37:51
        Copyright (c) Microsoft Corporation
        Enterprise Edition: Core-based Licensing (64-bit) on Windows NT 6.3 <X64> (Build 9600: )

.NET Framework Version

.NET 6.0.8

.NET 7.0.0

[wsmelton]

$SizesQuery2005 variable and corresponding if { } logic, Write-Message, etc should be removed because only version 10 and above is supported.

When did we stop supporting Server 2005 officially? With 2.0 release we are dropping support of Windows PowerShell versions. I see no reason to do that for SQL Server versions because SMO (even latest release) still supports back to 2005 as well. @potatoqualitee will have to make the call that we are removing 2005 support on this command.

In regard to SQL Injection, that is a feature request and not a bug as we do not address that anywhere in our codebase. The pattern we use is most common with PowerShell by building dynamic T-SQL statements using string concatenation and variables. If we switch to true dynamic T-SQL statements (parameterized) then we lose debug ability to know what statements our code generates. I do not agree at all with replacing or starting the use of Invoke-DbaQuery everywhere; that will change performance of our commands dramatically. This means removing the use of the short Query() method we have in place everywhere.

My view we are not a web application or console service but a PowerShell module for database administrators. There are tighter controls over how values are passed into our module's commands by those DBAs and the scripts being written around them. We could put validators around our Query() method as well that would be easier to maintain than trying to change all the T-SQL statements to parameterized statements calling Invoke command.

Until someone can show that malicious activity could happen using our modules in the current T-SQL execution pattern we use I do not see any reason to put effort into changing it.

[mattcargile]

Thanks for the feedback, @wsmelton . I didn't mean to suggest that 2005 was out of support. I was making an (illformed) assumption based on code at line 1004, namely the use of -MiniumumVersion. We could just as easily change that to -MinimumVersion 9 as you know.

https://github.com/dataplat/dbatools/blob/cb554f576758b915252a2a5b853aea9ac443b6dc/functions/Get-DbaHelpIndex.ps1#L1004

For SQL Injection, my concern would be if folks are using JEA or pwsh -ConfigurationFile or I guess mainly a PSSessionConfiguration with the RunAs feature. I couldn't quite think of a problem with JEA because folks would still be using their own credentials to connect. With all this in mind, I could see someone over granting the access on a run as account then they would assume they could block problematic access via JEA on that same endpoint or the like. Also, PowerShell Universal comes to mind as an entry point for injection.

All this to say, I guess it comes down to who is going to take the responsibility. Maybe there could be a warning placed about SQL injection in the docs to say that this module doesn't completely check all input? Or maybe we can use some good faith checks on the Query method?

For the performance side, would there still be a hit if one passed in the Connect-DbaInstance object to Invoke-DbaQuery -SqlInstance?

[wsmelton]

Can you actually show an attack or malicious example that works with command(s) in the module?

Even if we parameterize the statements, it won't mean they are protected from SQL injection.

[mattcargile]

I have an example in my original post which is the below. It only throws an error. One could drop a table or pull data with this method. It would take some brute force to create the correct structure, though.

Get-DbaHelpIndex -SqlInstance sql -Database db -ObjectName 'dbo.t''; select 1 as c --'

If we formally parametrize the query, how will SQL injection occur? Are you referring to folks tinkering with the ps1, psm1, etc files inside the module?

[wsmelton]

One could drop a table or pull data with this method. It would take some brute force to create the correct structure, though.

Yes, one can drop a table by running the following and this works with almost everyone of our commands that run T-SQL.

Get-DbaHelpIndex -SqlInstance $sql22Cn -Database testing -ObjectName 'dbo.t''; drop table test123 --'

Are you referring to folks tinkering with the ps1, psm1, etc files inside the module?

Yes, anyone with privileges can simply delete our allcommands file, and we go back to use the function files. Even if you have files signed and someone modifies them PowerShell does not tell you "hey that signature is invalid". It is just a signature in the file, not a hash or checksum value to tell the system the file is not the same. PowerShell won't tell you anything about it.

I can delete the allcommands.ps1 from 1.1.143 module path on my machine, then remove help content from the Get-DbaDatabase.ps1 (modified date shows updated). I import the module, connect to an instance, and then run the command. There is no warning or indicator that the file was modified. This is a single reason many module authors do not bother signing their modules, because it offers no protection.

Is it worth the effort to maintain parameterizing T-SQL statements in all of our T-SQL based commands? It is a faster method for malicious action to modify the PS1 without anyone knowing it.

[mattcargile]

Thanks for going in depth. 💯 I agree all bets are off if the bad actor can touch the ps files. The issue is in a "run as" scenario like JEA or other PSSession configurations, where a user only has an entry point of Get-DbaHelpIndex and couldn't readily modify the files.

Is it worth the effort to maintain parameterizing T-SQL statements in all of our T-SQL based commands? It is a faster method for malicious action to modify the PS1 without anyone knowing it.

Yes, this is the greater question. I would argue for parameterization across all applicable commands.

On another note, should I make another Issue for the 2005 support for Get-DbaHelpIndex?

[potatoqualitee]

I'm not super familiar with JEA (tested it once) and am not convinced that SQL injection should be a consideration for dbatools but I'm open to the idea.

In this case, a user would have to be prohibited from using New-Object to create a SqlConnection/SqlCmd which can execute SQL queries natively in .NET, and prohibited from using Invoke-DbaQuery and prohibited from using SSMS. Perhaps this is a scenario that works in JEA. I'll assume it does.

In such an environment, would it not default to SQL Server authorization to ensure that they are prohibited from accessing data they aren't authorized to see or manipulate?

this module doesn't completely check all input?

This module is intended for database administrators and developers. We'd weaken our own abilities make use of this tool if we prohibited keywords in this manner, no? I've always seen dbatools like SSMS in this way. It's not intended for end-users, but if end-users do use it, it's up to SQL Server to provide protection. Not the tool itself.

[wsmelton]

I don’t see our module being used as an attack service considering all a bad actor does to scripts is troll for credentials. Once they have those they are not going to waste their time figuring out how our module executes T-SQL commands while they can go directly to the source with the credentials they obtained.

Overall I see it as unnecessary extra code to maintain. The way statements are built as well in some commands would likely need a complete rewrite (thinking backup and restore commands).

[mattcargile]

In this case, a user would have to be prohibited from using New-Object to create a SqlConnection/SqlCmd which can execute SQL queries natively in .NET, and prohibited from using Invoke-DbaQuery and prohibited from using SSMS. Perhaps this is a scenario that works in JEA. I'll assume it does.

I'm thinking about this scenario with a runas account, primarily. I would assume one would only allow certain cmdlets to be executed where one would default to block all or most cmdlets.

Where possible, I still think parameterization should be attempted or considered especially on seemingly simpler cases as this cmdlet. I apologize if I am oversimplifying it or not accounting for problems with performance. Jumping out to Invoke-DbaQuery from .Query would complicate things a little more.

If anything, for this cmdlet, can we add a $ObjectName -replace "'" , "''" at the very least to prevent any unexpected behavior if someone creates a table with a single quote? Or maybe change .Query to include this quote behavior? I know this is super on the edge and folks should not be creating tables with names like this.

Then there is the 2005 support...