[BUG] win_lgpo does not account for spaces in XMLNS URLs #66992

willchenmark · 2024-10-23T19:40:39Z

Description
The win_lgpo module used does not currently attempt to escape spaces found in xmlns definitions.

Setup

on-prem machine (Windows 11 24H2)
VM (Virtualbox, KVM, etc. please specify)
VM running on a cloud service, please be explicit and add details
container (Kubernetes, Docker, containerd, etc. please specify)
or a combination, please be explicit
jails if it is FreeBSD
classic packaging
onedir packaging
used bootstrap to install

Steps to Reproduce the behavior
On a fresh Windows 11 24H2 system, attempt to apply any GPO that references the WindowsDefender-D0DE2C.adml definition, such as this user policy.

salt@master:/srv/salt# cat test.sls
"[BACKGROUND] Prevent desktop background from being changed":
  lgpo.set:
    - user_policy:
        Prevent changing desktop background: Enabled

The result will be an invalid URI error from lxml.

salt@master:/srv/salt# salt DEVEL-W-ea96054 state.apply test
DEVEL-W-ea96054:                                                                                                                                       
----------                                                                                                                                             
          ID: [BACKGROUND] Prevent desktop background from being changed                                                                               
    Function: lgpo.set                                                                                                                                 
      Result: False                                                                                                                                    
     Comment: An exception occurred in this state: Traceback (most recent call last):                                                                  
                File "C:\Program Files\Salt Project\Salt\Lib\site-packages\salt\modules\win_lgpo.py", line 5143, in _parse_xml                         
                  xml_tree = lxml.etree.parse(out_file, parser=parser)                                                                                 
                File "src\lxml\etree.pyx", line 3538, in lxml.etree.parse                                                                              
                File "src\lxml\parser.pxi", line 1876, in lxml.etree._parseDocument                                                                    
                File "src\lxml\parser.pxi", line 1902, in lxml.etree._parseDocumentFromURL                                                             
                File "src\lxml\parser.pxi", line 1805, in lxml.etree._parseDocFromFile                                                                 
                File "src\lxml\parser.pxi", line 1177, in lxml.etree._BaseParser._parseDocFromFile                                                     
                File "src\lxml\parser.pxi", line 615, in lxml.etree._ParserContext._handleParseResultDoc
                File "src\lxml\parser.pxi", line 725, in lxml.etree._handleParseResult                                        
                File "src\lxml\parser.pxi", line 654, in lxml.etree._raiseParseError
                File "file:/C:/ProgramData/Salt%20Project/Salt/var/cache/salt/minion/lgpo/policy_defs/WindowsDefender-D0DE2CD.adml", line 1
              lxml.etree.XMLSyntaxError: xmlns: 'http://schemas.microsoft.com/GroupPolicy/2006/07/Policysecurity intelligence' is not a valid URI, line
 1, column 246                                                                                                                                         
                                                                                                                                                       
              During handling of the above exception, another exception occurred:                 
                                                                                                                                                       
              Traceback (most recent call last):                                                                                                       
                File "C:\Program Files\Salt Project\Salt\Lib\site-packages\salt\state.py", line 2440, in call
                  ret = self.states[cdata["full"]](                                                                                                    
                File "C:\Program Files\Salt Project\Salt\Lib\site-packages\salt\loader\lazy.py", line 159, in __call__                                 
                  ret = self.loader.run(run_func, *args, **kwargs)         
                File "C:\Program Files\Salt Project\Salt\Lib\site-packages\salt\loader\lazy.py", line 1245, in run                                     
                  return self._last_context.run(self._run_as, _func_or_method, *args, **kwargs)
                File "C:\Program Files\Salt Project\Salt\Lib\site-packages\salt\loader\lazy.py", line 1260, in _run_as
                  ret = _func_or_method(*args, **kwargs)                
                File "C:\Program Files\Salt Project\Salt\Lib\site-packages\salt\loader\lazy.py", line 1296, in wrapper
                  return f(*args, **kwargs)        
                File "C:\Program Files\Salt Project\Salt\Lib\site-packages\salt\states\win_lgpo.py", line 412, in set_
                  lookup = __salt__["lgpo.get_policy_info"](      
                File "C:\Program Files\Salt Project\Salt\Lib\site-packages\salt\loader\lazy.py", line 159, in __call__
                  ret = self.loader.run(run_func, *args, **kwargs)                                                                                     
                File "C:\Program Files\Salt Project\Salt\Lib\site-packages\salt\loader\lazy.py", line 1245, in run    
                  return self._last_context.run(self._run_as, _func_or_method, *args, **kwargs)
                File "C:\Program Files\Salt Project\Salt\Lib\site-packages\salt\loader\lazy.py", line 1260, in _run_as
                  ret = _func_or_method(*args, **kwargs)
                File "C:\Program Files\Salt Project\Salt\Lib\site-packages\salt\modules\win_lgpo.py", line 8755, in get_policy_info
                  success, policy_xml_item, policy_name_list, message = _lookup_admin_template(
                File "C:\Program Files\Salt Project\Salt\Lib\site-packages\salt\modules\win_lgpo.py", line 8294, in _lookup_admin_template
                  admx_policy_definitions = _get_policy_definitions(language=adml_language)
                File "C:\Program Files\Salt Project\Salt\Lib\site-packages\salt\modules\win_lgpo.py", line 5326, in _get_policy_definitions
                  _load_policy_definitions(path=path, language=language)                                                                               
                File "C:\Program Files\Salt Project\Salt\Lib\site-packages\salt\modules\win_lgpo.py", line 5300, in _load_policy_definitions
                  xml_tree = _parse_xml(adml_file)      
                File "C:\Program Files\Salt Project\Salt\Lib\site-packages\salt\modules\win_lgpo.py", line 5147, in _parse_xml     
                  xml_tree = _remove_unicode_encoding(out_file)
                File "C:\Program Files\Salt Project\Salt\Lib\site-packages\salt\modules\win_lgpo.py", line 5040, in _remove_unicode_encoding
                  r' encoding=[\'"]+unicode[\'"]+', "", xml_content.decode("utf-16"), count=1
              UnicodeDecodeError: 'utf-16-le' codec can't decode byte 0x0a in position 134714: truncated data

That invalid URI error is caused by this adml file in the policy_defs cache:
C:\ProgramData\Salt Project\Salt\var\cache\salt\minion\lgpo\policy_defs\WindowsDefender-D0DE2C.adml

Specifically the Policysecurity intelligence on the first line.
https://github.com/microsoft/mdatp-devicecontrol/blob/main/windows/WindowsDefender.adml#L1

Expected behavior
The state should apply correctly, this would be the expected output.

DEVEL-W-ea96054:
----------
          ID: [BACKGROUND] Prevent desktop background from being changed
    Function: lgpo.set
      Result: True
     Comment: The following policies changed:
              Prevent changing desktop background
     Started: 14:35:35.471570
    Duration: 11736.57 ms
     Changes:   
              ----------
              new:
                  ----------
                  User Configuration:
                      ----------
                      Prevent changing desktop background:
                          Enabled
              old:
                  ----------
                  User Configuration:
                      ----------
                      Prevent changing desktop background:
                          Not Configured

Summary for DEVEL-W-ea96054
------------
Succeeded: 1 (changed=1)
Failed:    0
------------
Total states run:     1
Total run time:  11.737 s

More specifically any spaces that appear in xmlns urls should be escaped with '%20'. Such that
<policyDefinitionResources xmlns:xsd="http://www.w3.org/2001/XMLSchema" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" revision="1.0" schemaVersion="1.0" xmlns="http://schemas.microsoft.com/GroupPolicy/2006/07/Policysecurity intelligence">
becomes
<policyDefinitionResources xmlns:xsd="http://www.w3.org/2001/XMLSchema" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" revision="1.0" schemaVersion="1.0" xmlns="http://schemas.microsoft.com/GroupPolicy/2006/07/Policysecurity%20intelligence">

Versions Report

salt --versions-report

This is from the minion, as the issue is specific to the Windows salt-minion.

Salt Version:
          Salt: 3006.9
 
Python Version:
        Python: 3.10.14 (heads/main:9f7d197, Jun 26 2024, 11:42:40) [MSC v.1940 64 bit (AMD64)]
 
Dependency Versions:
          cffi: 1.14.6
      cherrypy: 18.6.1
  cryptography: 42.0.5
      dateutil: 2.8.1
     docker-py: Not Installed
         gitdb: 4.0.7
     gitpython: Not Installed
        Jinja2: 3.1.4
       libgit2: Not Installed
  looseversion: 1.0.2
      M2Crypto: Not Installed
          Mako: Not Installed
       msgpack: 1.0.2
  msgpack-pure: Not Installed
  mysql-python: Not Installed
     packaging: 22.0
     pycparser: 2.21
      pycrypto: Not Installed
  pycryptodome: 3.19.1
        pygit2: Not Installed
  python-gnupg: 0.4.8
        PyYAML: 6.0.1
         PyZMQ: 25.0.2
        relenv: 0.17.0
         smmap: 4.0.0
       timelib: 0.2.4
       Tornado: 4.5.3
           ZMQ: 4.3.4
 
System Versions:
          dist:   
        locale: utf-8
       machine: AMD64
       release: 10
        system: Windows
       version: 10 10.0.26100 SP0 Multiprocessor Free

Additional context

This might not be the best way to handle this, but I was able to correct the error by adding an additional function to modules/win_lgpo.py to escape those spaces, and then added it to the for line iterator in _parse_xml.

--- a/salt/modules/win_lgpo.py	2024-10-22 15:39:57.480400553 -0400
+++ b/salt/modules/win_lgpo.py	2024-10-23 15:29:04.600779526 -0400
@@ -5060,6 +5060,16 @@
     xml_tree = lxml.etree.parse(io.StringIO(modified_xml))
     return xml_tree
 
+def _encode_xmlns_url(match):
+    """
+    Escape spaces in xmlns urls
+    """
+    before_xmlns = match.group(1)
+    xmlns = match.group(2)
+    url = match.group(3)
+    after_url = match.group(4)
+    encoded_url = re.sub(r'\s+', '%20', url)
+    return f'{before_xmlns}{xmlns}="{encoded_url}"{after_url}'
 
 def _parse_xml(adm_file):
     """
@@ -5107,6 +5117,8 @@
                 encoding = "utf-16"
                 raw = raw.decode(encoding)
             for line in raw.split("\r\n"):
+                if 'xmlns="' in line:
+                    line = re.sub(r'(.*)(\bxmlns(?::\w+)?)\s*=\s*"([^"]+)"(.*)', _encode_xmlns_url, line)
                 if 'key="' in line:
                     start = line.index('key="')
                     q1 = line[start:].index('"') + start

The text was updated successfully, but these errors were encountered:

welcome · 2024-10-23T19:40:42Z

Hi there! Welcome to the Salt Community! Thank you for making your first contribution. We have a lengthy process for issues and PRs. Someone from the Core Team will follow up as soon as possible. In the meantime, here’s some information that may help as you continue your Salt journey.
Please be sure to review our Code of Conduct. Also, check out some of our community resources including:

There are lots of ways to get involved in our community. Every month, there are around a dozen opportunities to meet with other contributors and the Salt Core team and collaborate in real time. The best way to keep track is by subscribing to the Salt Community Events Calendar.
If you have additional questions, email us at [email protected]. We’re glad you’ve joined our community and look forward to doing awesome things with you!

dhs-rec · 2024-11-20T14:18:13Z

Just in case: Also happens on Server 2025, with Salt 3006.7.

bharrison-it · 2024-12-03T21:13:57Z

The patched module per @willchenmark didn't correct the problem during testing - returned output is the same.

willchenmark · 2024-12-03T21:19:10Z

@bharrison-it did you make the change on the minion or the master? Re-reading my issue, I failed to clarify that I made the change on the minion side.

bharrison-it · 2024-12-04T01:15:32Z

@willchenmark specifically I copied win_lgpo.py from /opt/saltstack/salt/lib/python3.10/site-packages/salt/modules (debian location) to /srv/salt/_modules, patched per the comment then synced the modules back to the minion with saltutil.sync_modules.

I confirmed the minion is indeed running the patched module, indicated by the execution path under 'extmods', but the same error occurs during the state run as with the vanilla win_lgpo mod.

      ID: windows_components_microsoft_defender_antivirus_policy
Function: lgpo.set
  Result: False
 Comment: An exception occurred in this state: Traceback (most recent call last):
            File "C:\ProgramData\Salt Project\Salt\var\cache\salt\minion\extmods\modules\win_lgpo.py", line 5143, in _parse_xml
              xml_tree = lxml.etree.parse(out_file, parser=parser)
            File "src\lxml\etree.pyx", line 3538, in lxml.etree.parse
            File "src\lxml\parser.pxi", line 1876, in lxml.etree._parseDocument
            File "src\lxml\parser.pxi", line 1902, in lxml.etree._parseDocumentFromURL
            File "src\lxml\parser.pxi", line 1805, in lxml.etree._parseDocFromFile
            File "src\lxml\parser.pxi", line 1177, in lxml.etree._BaseParser._parseDocFromFile
            File "src\lxml\parser.pxi", line 615, in lxml.etree._ParserContext._handleParseResultDoc
            File "src\lxml\parser.pxi", line 725, in lxml.etree._handleParseResult
            File "src\lxml\parser.pxi", line 654, in lxml.etree._raiseParseError
            File "file:/C:/ProgramData/Salt%20Project/Salt/var/cache/salt/minion/lgpo/policy_defs/WindowsDefender-D0DE2CD.adml", line 1
          lxml.etree.XMLSyntaxError: xmlns: 'http://schemas.microsoft.com/GroupPolicy/2006/07/Policysecurity intelligence' is not a valid URI, line 1, column 246

I did also confirm the copy of win_lgpo.py under extmods on the minion does contain the patched code.

Not sure what's going on as your function appears sound to me.

bharrison-it · 2024-12-04T03:12:46Z

@willchenmark My issue was pushing the patched win_lgpo.py to the minion and calling it without having removed the incorrectly parsed adml files from minion cache first.

For any minions which have run the un-patched module, saltutil.clear_cache (or, i assume, manual deletion of the affected adml file) can be utilized to clear the previously cached contents of the lgpo policy_defs folder and old, incorrectly parsed adml files.

A subsequent run of saltutil.sync_all resynced the patched win_lgpo module, and finally and a state call to lgpo.set prompted the patched win_lgpo module to parse the adml files and completed substitution as expected. State calls to lgpo.set now work as expected.

twangboy · 2025-01-06T19:44:40Z

@willchenmark Would you be able to make a PR for this?

willchenmark · 2025-01-06T19:54:19Z

@twangboy absolutely, happy to do so!

Opened PR #67129

I see that I'll need to add tests to the PR, not positive about docs though. The functionality of lgpo doesn't change materially, so I don't think any additional documentation is needed?

bdrx312 · 2025-01-09T04:39:24Z

An xmlns must be a URI and spaces are not valid in URIs (https://www.rfc-editor.org/rfc/rfc3986?form=MG0AV3) so lxml is correctly failing to parse that document. The correct solution is to correct the invalid adml document (https://github.com/microsoft/mdatp-devicecontrol/blob/main/windows/WindowsDefender.adml) with a pull request to change the space to a %20

willchenmark · 2025-01-09T15:22:24Z

While I agree that lxml is correctly failing to parse, and that the change should be made upstream (and I see you've already reported the issue, thank you), we should still include this patch to the lgpo module. It is not the only place where we are massaging the content of Microsoft's policy documents.

https://github.com/saltstack/salt/blob/master/salt/modules/win_lgpo.py#L5028-L5061

Escaping the string is a small additional check that aligns with that existing functionality, and it allows win.lgpo to actually apply the GPO policy affected by what is an obvious error. Otherwise we are at the whim of Microsoft to correct their mistake, and any version of Windows that uses the invalid policy document will fail to correctly apply GPOs through Salt.

When Microsoft corrects the issue the function could be patched out if so desired, but leaving it would prevent similar issues from arising in the future.

willchenmark added Bug broken, incorrect, or confusing behavior needs-triage labels Oct 23, 2024

twangboy self-assigned this Oct 29, 2024

doesitblend added the VMware label Dec 13, 2024

twangboy removed the needs-triage label Jan 6, 2025

willchenmark linked a pull request Jan 6, 2025 that will close this issue

Escape spaces found while parsing GPO XMLNS tags #67129

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BUG] win_lgpo does not account for spaces in XMLNS URLs #66992

[BUG] win_lgpo does not account for spaces in XMLNS URLs #66992

willchenmark commented Oct 23, 2024

welcome bot commented Oct 23, 2024

dhs-rec commented Nov 20, 2024

bharrison-it commented Dec 3, 2024

willchenmark commented Dec 3, 2024

bharrison-it commented Dec 4, 2024

bharrison-it commented Dec 4, 2024

twangboy commented Jan 6, 2025

willchenmark commented Jan 6, 2025 •

edited

Loading

bdrx312 commented Jan 9, 2025 •

edited

Loading

willchenmark commented Jan 9, 2025

[BUG] win_lgpo does not account for spaces in XMLNS URLs #66992

[BUG] win_lgpo does not account for spaces in XMLNS URLs #66992

Comments

willchenmark commented Oct 23, 2024

welcome bot commented Oct 23, 2024

dhs-rec commented Nov 20, 2024

bharrison-it commented Dec 3, 2024

willchenmark commented Dec 3, 2024

bharrison-it commented Dec 4, 2024

bharrison-it commented Dec 4, 2024

twangboy commented Jan 6, 2025

willchenmark commented Jan 6, 2025 • edited Loading

bdrx312 commented Jan 9, 2025 • edited Loading

willchenmark commented Jan 9, 2025

willchenmark commented Jan 6, 2025 •

edited

Loading

bdrx312 commented Jan 9, 2025 •

edited

Loading