Merge pull request #143 from blacklanternsecurity/fix-target-bug

Fix scope calculation dropping existing assets when target is edited

Author: TheTechromancer

bbot_server/modules/targets/targets_api.py

@@ -113,7 +113,7 @@ async def refresh_asset_scope(self, host: str, target: BBOTTarget, target_id: UU
         scope_result_type = getattr(scope_result, "type", None)
         if scope_result_type == "NEW_IN_SCOPE_ASSET":
             asset_scope = sorted(set(asset_scope) | set([target_id]))
-        else:
+        elif scope_result_type == "ASSET_SCOPE_CHANGED":
             asset_scope = sorted(set(asset_scope) - set([target_id]))
         asset_results = await self.root.assets.collection.update_many(
             {"host": host},
@@ -358,17 +358,17 @@ async def _check_scope(self, host, resolved_hosts, target: BBOTTarget, target_id
         try:
             # we take the main host and its A/AAAA DNS records into account
             for rdtype, hosts in resolved_hosts.items():
-                for host in hosts:
+                for h in hosts:
                     # if any of the hosts are blacklisted, abort immediately
-                    if target.blacklisted(host):
-                        blacklisted_reason = f"{rdtype}->{host}"
+                    if target.blacklisted(h):
+                        blacklisted_reason = f"{rdtype}->{h}"
                         in_target_reason = ""
                         # break out of the loop
                         raise BlacklistedError
                     # check against whitelist
                     if not in_target_reason:
-                        if target.in_target(host):
-                            in_target_reason = f"{rdtype}->{host}"
+                        if target.in_target(h):
+                            in_target_reason = f"{rdtype}->{h}"
         except BlacklistedError:
             pass
 

tests/test_applets/test_applet_targets.py

@@ -482,15 +482,68 @@ async def after_scan_2(self):
         assets = [a async for a in self.bbot_server.list_assets()]
 
         # evilcorp.azure.com (127.0.0.3) and evilcorp.amazonaws.com (127.0.0.4) are now part of the target
-        # 127.0.0.1 no longer exists (it is nowhere to be found in scan 2)
         # localhost.evilcorp.com (127.0.0.2) is still blacklisted
         target_2_assets = {a.host for a in assets if self.target2.id in a.scope}
-        assert target_2_assets == {"evilcorp.azure.com", "evilcorp.amazonaws.com"}
+        assert target_2_assets == {"127.0.0.1", "evilcorp.azure.com", "evilcorp.amazonaws.com"}
 
     async def after_archive(self):
         pass
 
 
+class TestTargetAddDomainPreservesExistingScope(BaseAppletTest):
+    """
+    Regression test for bug where adding a domain to an existing target
+    caused all previously-in-scope assets to lose their scope, leaving only
+    the newly added domain in scope.
+
+    Root cause: refresh_asset_scope treated a None return from _check_scope
+    (meaning "no change") as "out of scope", incorrectly removing the target
+    from assets that were already in scope.
+    """
+
+    needs_worker = True
+
+    async def setup(self):
+        assert await self.bbot_server.get_hosts() == []
+        assert await self.bbot_server.get_targets() == []
+
+        # create a target with just evilcorp.com
+        self.target = await self.bbot_server.create_target(
+            name="evilcorp",
+            description="evilcorp target",
+            target=["evilcorp.com"],
+        )
+
+    async def after_scan_1(self):
+        # verify evilcorp.com assets are in scope
+        assets = [a async for a in self.bbot_server.list_assets()]
+        target_assets = {a.host for a in assets if self.target.id in a.scope}
+        assert "evilcorp.com" in target_assets
+        assert len(target_assets) > 1, f"Expected multiple evilcorp.com assets in scope, got: {target_assets}"
+        self.original_target_assets = target_assets
+
+        # BUG REPRODUCTION: add a new domain to the target while keeping the existing one
+        self.target.target = ["evilcorp.com", "testevilcorp.com"]
+        await self.bbot_server.update_target(self.target.id, self.target)
+        await asyncio.sleep(1.0)
+
+        # verify that existing evilcorp.com assets are STILL in scope
+        assets = [a async for a in self.bbot_server.list_assets()]
+        target_assets_after = {a.host for a in assets if self.target.id in a.scope}
+
+        # the new domain should also be in scope
+        assert "testevilcorp.com" in target_assets_after, (
+            f"Newly added domain testevilcorp.com should be in scope, got: {target_assets_after}"
+        )
+
+        # all previously in-scope assets should still be in scope
+        missing = self.original_target_assets - target_assets_after
+        assert not missing, (
+            f"BUG: These assets lost their scope after adding a domain to the target: {missing}. "
+            f"Before: {self.original_target_assets}, After: {target_assets_after}"
+        )
+
+
 class TestTargetUpdateRemovesTargetFromAssets(BaseAppletTest):
     """
     Regression test for bug where editing or deleting a target to remove a domain