From 0fd952ae2b8550a3656ffc38bad4036221021f3e Mon Sep 17 00:00:00 2001 From: Sandiyo Christan <55909152+sandiyochristan@users.noreply.github.com> Date: Thu, 21 May 2026 19:47:23 +0530 Subject: [PATCH] chore(m365): use PowerShell best practices for quoting credential variables (#9997) Co-authored-by: Hugo P.Brito --- prowler/CHANGELOG.md | 1 + .../m365/lib/powershell/m365_powershell.py | 22 ++- .../lib/powershell/m365_powershell_test.py | 134 +++++++++++++++++- 3 files changed, 148 insertions(+), 9 deletions(-) diff --git a/prowler/CHANGELOG.md b/prowler/CHANGELOG.md index 95b4ee6712..87ab42f4a7 100644 --- a/prowler/CHANGELOG.md +++ b/prowler/CHANGELOG.md @@ -17,6 +17,7 @@ All notable changes to the **Prowler SDK** are documented in this file. ### 🔄 Changed - `OktaProvider.test_connection` accepts an optional `provider_id` (org domain) and raises `OktaInvalidProviderIdError` (14007) when it doesn't match the authenticated org — guards against stored UID drifting from the credentials' org [(#11184)](https://github.com/prowler-cloud/prowler/pull/11184) +- Use single-quoted strings for credential variables in the M365 provider PowerShell session, following PowerShell best practices for literal values [(#9997)](https://github.com/prowler-cloud/prowler/pull/9997) --- diff --git a/prowler/providers/m365/lib/powershell/m365_powershell.py b/prowler/providers/m365/lib/powershell/m365_powershell.py index 9cc7207f20..1bc8aa4075 100644 --- a/prowler/providers/m365/lib/powershell/m365_powershell.py +++ b/prowler/providers/m365/lib/powershell/m365_powershell.py @@ -104,8 +104,11 @@ class M365PowerShell(PowerShellSession): authentication information. Note: - The credentials are sanitized to prevent command injection and - stored securely in the PowerShell session. + ``client_id`` and ``tenant_id`` are sanitized via ``sanitize()`` since + they are UUIDs. ``client_secret`` is assigned with a single-quoted + string (escaping any embedded single quote as ``''``) following + PowerShell best practices for literal values, so its content is taken + verbatim with no variable expansion or subexpression evaluation. """ # Certificate Auth if credentials.certificate_content and credentials.client_id: @@ -135,9 +138,16 @@ class M365PowerShell(PowerShellSession): else: # Application Auth - self.execute(f'$clientID = "{credentials.client_id}"') - self.execute(f'$clientSecret = "{credentials.client_secret}"') - self.execute(f'$tenantID = "{credentials.tenant_id}"') + sanitized_client_id = self.sanitize(credentials.client_id) + sanitized_tenant_id = self.sanitize(credentials.tenant_id) + self.execute(f"$clientID = '{sanitized_client_id}'") + # Single-quoted strings are the PowerShell convention for literals: + # the content is taken verbatim with no variable expansion. Escape any + # embedded single quote as '' and do not sanitize() so the value is + # preserved exactly. + sanitized_secret = (credentials.client_secret or "").replace("'", "''") + self.execute(f"$clientSecret = '{sanitized_secret}'") + self.execute(f"$tenantID = '{sanitized_tenant_id}'") self.execute( '$graphtokenBody = @{ Grant_Type = "client_credentials"; Scope = "https://graph.microsoft.com/.default"; Client_Id = $clientID; Client_Secret = $clientSecret }' ) @@ -196,7 +206,7 @@ class M365PowerShell(PowerShellSession): """Test Exchange Online API connection and raise exception if it fails.""" try: self.execute( - '$SecureSecret = ConvertTo-SecureString "$clientSecret" -AsPlainText -Force' + "$SecureSecret = ConvertTo-SecureString $clientSecret -AsPlainText -Force" ) self.execute( '$exchangeToken = Get-MsalToken -clientID "$clientID" -tenantID "$tenantID" -clientSecret $SecureSecret -Scopes "https://outlook.office365.com/.default"' diff --git a/tests/providers/m365/lib/powershell/m365_powershell_test.py b/tests/providers/m365/lib/powershell/m365_powershell_test.py index 84ae2ab129..30e2eedcad 100644 --- a/tests/providers/m365/lib/powershell/m365_powershell_test.py +++ b/tests/providers/m365/lib/powershell/m365_powershell_test.py @@ -101,9 +101,9 @@ class Testm365PowerShell: # Call original init_credential to verify application authentication setup M365PowerShell.init_credential(session, credentials) - session.execute.assert_any_call('$clientID = "test_client_id"') - session.execute.assert_any_call('$clientSecret = "test_client_secret"') - session.execute.assert_any_call('$tenantID = "test_tenant_id"') + session.execute.assert_any_call("$clientID = 'test_client_id'") + session.execute.assert_any_call("$clientSecret = 'test_client_secret'") + session.execute.assert_any_call("$tenantID = 'test_tenant_id'") session.execute.assert_any_call( '$graphtokenBody = @{ Grant_Type = "client_credentials"; Scope = "https://graph.microsoft.com/.default"; Client_Id = $clientID; Client_Secret = $clientSecret }' ) @@ -112,6 +112,128 @@ class Testm365PowerShell: ) session.close() + @patch("subprocess.Popen") + def test_init_credential_special_chars_in_secret(self, mock_popen): + """Test that secrets with $, !, #, and ' are preserved via single-quote escaping.""" + mock_process = MagicMock() + mock_popen.return_value = mock_process + credentials = M365Credentials( + client_id="test_client_id", + client_secret="Pa$$w0rd!#'", + tenant_id="test_tenant_id", + ) + identity = M365IdentityInfo( + identity_id="test_id", + identity_type="Service Principal", + tenant_id="test_tenant", + tenant_domain="example.com", + tenant_domains=["example.com"], + location="test_location", + ) + with patch.object(M365PowerShell, "init_credential"): + session = M365PowerShell(credentials, identity) + + session.execute = MagicMock() + M365PowerShell.init_credential(session, credentials) + + # Single quotes prevent PowerShell $$ expansion; + # embedded ' is escaped as '' per PowerShell convention + session.execute.assert_any_call("$clientSecret = 'Pa$$w0rd!#'''") + + @pytest.mark.parametrize( + "secret, expected_command", + [ + # Plain secret: single quotes behave like the old double quotes. + ("simplesecret", "$clientSecret = 'simplesecret'"), + # $ must NOT expand as a PowerShell variable. + ("Pa$$w0rd", "$clientSecret = 'Pa$$w0rd'"), + # $(...) subexpression must stay literal and never execute. + ("a$(whoami)b", "$clientSecret = 'a$(whoami)b'"), + # ${var} expansion must stay literal too. + ("a${env:PATH}b", "$clientSecret = 'a${env:PATH}b'"), + # Backtick is a literal char inside single quotes (not an escape). + ("pass`word", "$clientSecret = 'pass`word'"), + # Double quotes are literal inside single quotes. + ('pa"ss"word', "$clientSecret = 'pa\"ss\"word'"), + # A single quote is doubled per PowerShell escaping rules. + ("O'Brien", "$clientSecret = 'O''Brien'"), + # Consecutive single quotes each get doubled. + ("a''b", "$clientSecret = 'a''''b'"), + # A payload with quotes and metacharacters stays a single literal + # string and cannot break out of the quoting. + ( + "'; Remove-Item -Recurse -Force; '", + "$clientSecret = '''; Remove-Item -Recurse -Force; '''", + ), + # Other shell metacharacters are preserved verbatim (no sanitize()). + ("p@ss!#%&;w0rd", "$clientSecret = 'p@ss!#%&;w0rd'"), + # Newline embedded in the secret is preserved verbatim. + ("line1\nline2", "$clientSecret = 'line1\nline2'"), + # Empty secret renders an empty single-quoted string. + ("", "$clientSecret = ''"), + # None secret is coerced to an empty single-quoted string. + (None, "$clientSecret = ''"), + ], + ) + @patch("subprocess.Popen") + def test_init_credential_secret_escaping_edge_cases( + self, mock_popen, secret, expected_command + ): + """The client_secret is single-quote escaped, preserving every special + character verbatim with no PowerShell expansion or subexpression + evaluation.""" + mock_process = MagicMock() + mock_popen.return_value = mock_process + credentials = M365Credentials( + client_id="test_client_id", + client_secret=secret, + tenant_id="test_tenant_id", + ) + identity = M365IdentityInfo( + identity_id="test_id", + identity_type="Service Principal", + tenant_id="test_tenant", + tenant_domain="example.com", + tenant_domains=["example.com"], + location="test_location", + ) + with patch.object(M365PowerShell, "init_credential"): + session = M365PowerShell(credentials, identity) + + session.execute = MagicMock() + M365PowerShell.init_credential(session, credentials) + + session.execute.assert_any_call(expected_command) + + @patch("subprocess.Popen") + def test_init_credential_sanitizes_client_and_tenant_id(self, mock_popen): + """client_id and tenant_id are sanitized in the Application Auth path, + stripping shell metacharacters while keeping UUID-safe characters.""" + mock_process = MagicMock() + mock_popen.return_value = mock_process + credentials = M365Credentials( + client_id="abc-123; Remove-Item", + client_secret="secret", + tenant_id="def-456 && whoami", + ) + identity = M365IdentityInfo( + identity_id="test_id", + identity_type="Service Principal", + tenant_id="test_tenant", + tenant_domain="example.com", + tenant_domains=["example.com"], + location="test_location", + ) + with patch.object(M365PowerShell, "init_credential"): + session = M365PowerShell(credentials, identity) + + session.execute = MagicMock() + M365PowerShell.init_credential(session, credentials) + + # sanitize() removes ';', spaces and '&' but keeps letters, digits and '-'. + session.execute.assert_any_call("$clientID = 'abc-123Remove-Item'") + session.execute.assert_any_call("$tenantID = 'def-456whoami'") + @patch("subprocess.Popen") def test_remove_ansi(self, mock_popen): credentials = M365Credentials( @@ -562,6 +684,12 @@ class Testm365PowerShell: assert result is True assert session.execute.call_count == 3 + # The secret must be referenced as a bare PowerShell variable. Wrapping it + # in double quotes ("$clientSecret") would re-expand special chars that were + # correctly escaped during assignment, so assert the exact command. + session.execute.assert_any_call( + "$SecureSecret = ConvertTo-SecureString $clientSecret -AsPlainText -Force" + ) session.execute_connect.assert_called_once_with( 'Connect-ExchangeOnline -AccessToken $exchangeToken.AccessToken -Organization "$tenantID"' )