Fix/mssql xpcmdshell permission check

[azoxlpf]

Description

This PR fixes issue #959, where the enable_cmdshell MSSQL module displayed xp_cmdshell successfully enabled even when the user lacked RECONFIGURE permissions.

Now, before attempting to enable xp_cmdshell, the module performs:

A silent permission check using
HAS_PERMS_BY_NAME(NULL, 'SERVER', 'ALTER SETTINGS') and IS_SRVROLEMEMBER('sysadmin').

A verification of sys.configurations.value_in_use to confirm the actual runtime state.

If the user lacks the required permissions, NetExec will now correctly display:

[-] You do not have permission to enable xp_cmdshell.

Type of change

Insert an "x" inside the brackets for relevant items (do not delete options)

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Deprecation of feature or functionality
• This change requires a documentation update
• This requires a third party update (such as Impacket, Dploot, lsassy, etc)

Screenshots (if appropriate):

Before :

After :

Checklist:

Insert an "x" inside the brackets for completed and relevant items (do not delete options)

• I have ran Ruff against my changes (via poetry: poetry run python -m ruff check . --preview, use --fix to automatically fix what it can)
• I have added or updated the tests/e2e_commands.txt file if necessary (new modules or features are required to be added to the e2e tests)
• New and existing e2e tests pass locally with my changes
• If reliant on changes of third party dependencies, such as Impacket, dploot, lsassy, etc, I have linked the relevant PRs in those projects
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation (PR here: https://github.com/Pennyw0rth/NetExec-Wiki)

[NeffIsBack]

Thanks for the PR!
@azoxlpf do you have a good resource on what these permissions allows you to do? Maybe this should be included in the admin check itself.

[azoxlpf]

Thanks for the PR! @azoxlpf do you have a good resource on what these permissions allows you to do? Maybe this should be included in the admin check itself.

I don’t have any specific external resource apart from the official Microsoft documentation, but here are some useful references:
• HAS_PERMS_BY_NAME
• IS_SRVROLEMEMBER
• xp_cmdshell requirements

[NeffIsBack]

@azoxlpf please check the coding conventions with ruff. VSCode plugin highly recommended if you don't want to use the cli

[NeffIsBack]

A few other things:

• I don't think we should check permissions here. If we check them we should integrate that into the is_admin check as this will allow command execution.
• Variable names should always be descriptive. Single or double character variables are most likely not descriptive enough and should be renamed to meaningful variable names

[azoxlpf]

@azoxlpf please check the coding conventions with ruff. VSCode plugin highly recommended if you don't want to use the cli

Oh my bad, i’m already using the Ruff VS Code extension thought I’d fixed everything

[azoxlpf]

A few other things:

* I don't think we should check permissions here. If we check them we should integrate that into the is_admin check as this will allow command execution.

* Variable names should always be descriptive. Single or double character variables are most likely not descriptive enough and should be renamed to meaningful variable names

Got it, so you’d prefer to move the permission check into the check_if_admin function in mssql.py, right?

[NeffIsBack]

A few other things:

* I don't think we should check permissions here. If we check them we should integrate that into the is_admin check as this will allow command execution.

* Variable names should always be descriptive. Single or double character variables are most likely not descriptive enough and should be renamed to meaningful variable names

Got it, so you’d prefer to move the permission check into the check_if_admin function in mssql.py, right?

As this allows command execution i think that would be the best way, yes (but in a different PR). Can you provide a proof or configuration guide that i can configure it myself to check if it indeed does?

Just took a look at our wiki and saw that the explanation for MSSQL is missing: https://www.netexec.wiki/getting-started/using-credentials
When merged we should add a column row for that and explain that pwned means at least command execution.

[azoxlpf]

A few other things:

* I don't think we should check permissions here. If we check them we should integrate that into the is_admin check as this will allow command execution.

* Variable names should always be descriptive. Single or double character variables are most likely not descriptive enough and should be renamed to meaningful variable names

Got it, so you’d prefer to move the permission check into the check_if_admin function in mssql.py, right?

As this allows command execution i think that would be the best way, yes (but in a different PR). Can you provide a proof or configuration guide that i can configure it myself to check if it indeed does?

Just took a look at our wiki and saw that the explanation for MSSQL is missing: https://www.netexec.wiki/getting-started/using-credentials When merged we should add a column row for that and explain that pwned means at least command execution.

You can reproduce it easily by creating a login with the following SQL commands (run as sysadmin):

USE master;
CREATE LOGIN [user] WITH PASSWORD = 'Password';
GRANT ALTER SETTINGS TO [user];

This grants the minimum permission (ALTER SETTINGS) required for the module to enable xp_cmdshell without full sysadmin rights.

[NeffIsBack]

Hey i am not sure if this is the right approach. I don't think adding a bunch of try&except will solve the problem. I think the better solution is to:

1. Try executing the SQL query
2. With the merge of Set lastError when parsing mssql replies fortra/impacket#2082 check if an error occurred when executing the SQL query and display that appropriately to the user

See also: #997

Regarding the check on alter settings:
We should assess if being able to "alter" the settings means compromised the host. Likely it is if i understand that correctly because you can then execute commands right? If we decide so we should:

• Add that check to the is_admin() check of the protocol
• Add that information to the wiki: https://www.netexec.wiki/getting-started/using-credentials#using-credentials

[azoxlpf]

Hey i am not sure if this is the right approach. I don't think adding a bunch of try&except will solve the problem. I think the better solution is to:

1. Try executing the SQL query

2. With the merge of Set lastError when parsing mssql replies fortra/impacket#2082 check if an error occurred when executing the SQL query and display that appropriately to the user

See also: #997

Regarding the check on alter settings:

We should assess if being able to "alter" the settings means compromised the host. Likely it is if i understand that correctly because you can then execute commands right? If we decide so we should:

• Add that check to the is_admin() check of the protocol

• Add that information to the wiki: https://www.netexec.wiki/getting-started/using-credentials#using-credentials

Hi, that sounds like the best approach.
So if I understand correctly, for this PR we simply stick to the minimal fix: try the SQL query, rely on the execution result + final value_in_use state, and avoid adding extra permission logic or over-handling exceptions.

Then we handle the permission checks and the is_admin() update separately in another PR.

[azoxlpf]

And yes, an account with ALTER SETTINGS can enable xp_cmdshell but still be unable to actually execute commands through it unless it’s sysadmin (or explicitly configured via proxy).