Syncrepl fails to parse SyncInfoMessage when the message is a syncIdSet

Firstyear · web-flow · commit 8f79376cfcb6 · 2020-06-18T11:03:28.000+02:00
Previously, the SyncInfoMessage parser used a for loop to test for the presence of each named choice with with the structure. However, because refreshDelete and refreshPresent both are able to be fully resolved as defaults, and due to how pyasn1 accesses named types, it was not checking the choice tag, and would return a fully populated refreshDelete to the caller instead. This fixes the parser to always return the inner component, and retrieves the name based on the current choice of the tagged item. Author: William Brown <william@blackhats.net.au> #351
diff --git a/Lib/ldap/syncrepl.py b/Lib/ldap/syncrepl.py
@@ -314,34 +314,35 @@ def __init__(self, encodedMessage):
         self.refreshPresent = None
         self.syncIdSet = None
 
-        for attr in ['newcookie', 'refreshDelete', 'refreshPresent', 'syncIdSet']:
-            comp = d[0].getComponentByName(attr)
-
-            if comp is not None and comp.hasValue():
-
-                if attr == 'newcookie':
-                    self.newcookie = str(comp)
-                    return
+        # Due to the way pyasn1 works, refreshDelete and refreshPresent are both
+        # valid in the components as they are fully populated defaults. We must
+        # get the component directly from the message, not by iteration.
+        attr = d[0].getName()
+        comp = d[0].getComponent()
+
+        if comp is not None and comp.hasValue():
+            if attr == 'newcookie':
+                self.newcookie = str(comp)
+                return
 
-                val = {}
+            val = {}
 
-                cookie = comp.getComponentByName('cookie')
-                if cookie.hasValue():
-                    val['cookie'] = str(cookie)
+            cookie = comp.getComponentByName('cookie')
+            if cookie.hasValue():
+                val['cookie'] = str(cookie)
 
-                if attr.startswith('refresh'):
-                    val['refreshDone'] = bool(comp.getComponentByName('refreshDone'))
-                elif attr == 'syncIdSet':
-                    uuids = []
-                    ids = comp.getComponentByName('syncUUIDs')
-                    for i in range(len(ids)):
-                        uuid = UUID(bytes=bytes(ids.getComponentByPosition(i)))
-                        uuids.append(str(uuid))
-                    val['syncUUIDs'] = uuids
-                    val['refreshDeletes'] = bool(comp.getComponentByName('refreshDeletes'))
+            if attr.startswith('refresh'):
+                val['refreshDone'] = bool(comp.getComponentByName('refreshDone'))
+            elif attr == 'syncIdSet':
+                uuids = []
+                ids = comp.getComponentByName('syncUUIDs')
+                for i in range(len(ids)):
+                    uuid = UUID(bytes=bytes(ids.getComponentByPosition(i)))
+                    uuids.append(str(uuid))
+                val['syncUUIDs'] = uuids
+                val['refreshDeletes'] = bool(comp.getComponentByName('refreshDeletes'))
 
-                setattr(self, attr, val)
-                return
+            setattr(self, attr, val)
 
 
 class SyncreplConsumer:
diff --git a/Tests/t_ldap_syncrepl.py b/Tests/t_ldap_syncrepl.py
@@ -10,6 +10,7 @@
 import shelve
 import sys
 import unittest
+import binascii
 
 if sys.version_info[0] <= 2:
     PY2 = True
@@ -21,7 +22,7 @@
 
 import ldap
 from ldap.ldapobject import SimpleLDAPObject
-from ldap.syncrepl import SyncreplConsumer
+from ldap.syncrepl import SyncreplConsumer, SyncInfoMessage
 
 from slapdtest import SlapdObject, SlapdTestCase
 
@@ -445,6 +446,47 @@ def setUp(self):
         )
         self.suffix = self.server.suffix.encode('utf-8')
 
+class DecodeSyncreplProtoTests(unittest.TestCase):
+    """
+    Tests of the ASN.1 decoder for tricky cases or past issues to ensure that
+    syncrepl messages are handled correctly.
+    """
+
+    def test_syncidset_message(self):
+        """
+        A syncrepl server may send a sync info message, with a syncIdSet
+        of uuids to delete. A regression was found in the original
+        sync info message implementation due to how the choice was
+        evaluated, because refreshPresent and refreshDelete were both
+        able to be fully expressed as defaults, causing the parser
+        to mistakenly catch a syncIdSet as a refreshPresent/refereshDelete.
+
+        This tests that a syncIdSet request is properly decoded.
+
+        reference: https://tools.ietf.org/html/rfc4533#section-2.5
+        """
+
+        # This is a dump of a syncidset message from wireshark + 389-ds
+        msg = """
+        a36b04526c6461706b64632e6578616d706c652e636f6d3a333839303123636e
+        3d6469726563746f7279206d616e616765723a64633d6578616d706c652c6463
+        3d636f6d3a286f626a656374436c6173733d2a2923330101ff311204108dc446
+        01a93611ea8aaff248c5fa5780
+        """.replace(' ', '').replace('\n', '')
+
+        msgraw = binascii.unhexlify(msg)
+        sim = SyncInfoMessage(msgraw)
+        self.assertEqual(sim.refreshDelete, None)
+        self.assertEqual(sim.refreshPresent, None)
+        self.assertEqual(sim.newcookie, None)
+        self.assertEqual(sim.syncIdSet,
+            {
+                'cookie': 'ldapkdc.example.com:38901#cn=directory manager:dc=example,dc=com:(objectClass=*)#3',
+                'syncUUIDs': ['8dc44601-a936-11ea-8aaf-f248c5fa5780'],
+                'refreshDeletes': True
+            }
+        )
+
 
 if __name__ == '__main__':
     unittest.main()
