Skip to content

fix(amass): replace hardcoded /tmp/amass with a private per-task scratch dir - #2020

Closed
Subramaniyajothi6 wants to merge 956 commits into
utksh1:mainfrom
Subramaniyajothi6:fix/amass-tmp-path
Closed

fix(amass): replace hardcoded /tmp/amass with a private per-task scratch dir#2020
Subramaniyajothi6 wants to merge 956 commits into
utksh1:mainfrom
Subramaniyajothi6:fix/amass-tmp-path

Conversation

@Subramaniyajothi6

Copy link
Copy Markdown
Contributor

Summary

Closes #1803.

The amass plugin's command_template hardcoded -dir /tmp/amass. That path is
world-writable and predictable, which allows symlink/hijack attacks and cross-user
collisions, and it is never cleaned up between runs.

Changes

  • plugins/amass/metadata.json — replace the hardcoded /tmp/amass with the
    reserved token %SECUSCAN_SCRATCH_DIR% (checksum refreshed). The token contains no
    {...}, so it passes plugin validation and command-template interpolation through
    untouched as an opaque literal.
  • backend/secuscan/executor.py — add a small, reusable per-task scratch-dir
    primitive:
    • SCRATCH_DIR_PLACEHOLDER constant + _substitute_scratch_dir() helper, which
      creates a fresh tempfile.mkdtemp(prefix="secuscan-<plugin>-") directory (mode
      0700, unpredictable name) and substitutes every placeholder token with its path.
    • _execute_standard_scanner() resolves the placeholder right after building the
      command and wraps command execution in try/finally so the directory is removed
      when the scan ends — on success, egress-validation failure, docker error, timeout,
      or crash.

Any plugin that needs a private, per-task working directory can now use the same
reserved token instead of a hardcoded path.

Why this shape

Command-template interpolation only substitutes declared-field {placeholders} (and
runs them through sanitize_input), and the plugin validator rejects {placeholders}
that don't map to a field — so a runtime-computed path can't be injected cleanly through
a field. A reserved literal resolved centrally in the executor (which owns the run
lifecycle and can guarantee cleanup) is the minimal, robust fit, and matches the issue's
"per-task sandbox dir" suggestion.

amass's parser only consumes stdout, so the -dir graph store is pure scratch and safe
to remove after the run. In Docker mode the container is --rm; the host scratch dir is
still created unpredictably and cleaned up.

Tests

  • Updated the three amass command-render tests that asserted /tmp/amass (they encoded
    the vulnerable path) and added a regression assert that it's gone and the sentinel is
    present.
  • New testing/backend/unit/test_executor_scratch_dir.py: no-op without the token; the
    token resolves to a real existing directory; distinct/unpredictable across calls; all
    occurrences replaced; directory is 0700-private on POSIX.

pytest test_amass_plugin.py unit/test_executor_scratch_dir.py → 27 passed, 1 skipped.
Executor suite (48), plugin integrity/checksum (20) pass. validate_plugins.py → all 59
valid. ruff clean.

Notes

  • No default guard weakened; this only removes a predictable-path weakness and adds
    cleanup. PLUGINS.md catalogue row for amass is unchanged.

@utksh1 please review this pr

Rakshak05 and others added 30 commits July 1, 2026 21:58
…1546)

Co-authored-by: tmdeveloper007 <tmdeveloper007@users.noreply.github.com>
Co-authored-by: tmdeveloper007 <tmdeveloper007@users.noreply.github.com>
Co-authored-by: tmdeveloper007 <tmdeveloper007@users.noreply.github.com>
…h1#1542)

Co-authored-by: tmdeveloper007 <tmdeveloper007@users.noreply.github.com>
Co-authored-by: tmdeveloper007 <tmdeveloper007@users.noreply.github.com>
Co-authored-by: tmdeveloper007 <tmdeveloper007@users.noreply.github.com>
Co-authored-by: tmdeveloper007 <tmdeveloper007@users.noreply.github.com>
Co-authored-by: tmdeveloper007 <tmdeveloper007@users.noreply.github.com>
Co-authored-by: tmdeveloper007 <tmdeveloper007@users.noreply.github.com>
…odule (utksh1#1524)

The extract_target helper in executor.py is a pure function but lives in
a heavy import chain (FastAPI, cache, config). Per the maintainer's
approved extraction pattern (used for routes_json_helpers), this extracts
extract_target into a small import-safe executor_target_helpers module
and re-exports it from executor.py so existing call sites keep working.

Closes utksh1#1389.

Co-authored-by: tmdeveloper007 <tmdeveloper007@users.noreply.github.com>
Co-authored-by: Tomeshwari-02 <tomeshwari903@gmail.com>
…h1#1563)

Co-authored-by: tmdeveloper007 <tmdeveloper007@users.noreply.github.com>
Co-authored-by: tmdeveloper007 <tmdeveloper007@users.noreply.github.com>
Co-authored-by: tmdeveloper007 <tmdeveloper007@users.noreply.github.com>
Co-authored-by: tmdeveloper007 <tmdeveloper007@users.noreply.github.com>
NaitikVerma6776 and others added 3 commits July 22, 2026 08:16
…ne-standardize-9bb6

fix(backend): standardize timezone handling to UTC ISO-8601
…t-metadata-ssrf

Fix utksh1#1748: make cloud-metadata/private-range denylist non-overridable
Fix: add auth and owner isolation to saved views API (closes utksh1#1743)

@utksh1 utksh1 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the Docker bind mount after the prior review. Please rebase and fix the failing backend-unit check, then add/confirm a regression test that the Docker command mounts the scratch directory at the exact path passed to the tool and that cleanup still occurs on a failed command.

@utksh1 utksh1 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the Docker bind mount after the prior review. Please rebase and fix the failing backend-unit check, then add/confirm a regression test that the Docker command mounts the scratch directory at the exact path passed to the tool and that cleanup still occurs on a failed command.

@utksh1 utksh1 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Docker bind-mount follow-up addresses the original concern, but the branch is still behind main and fails backend-unit. Please rebase, fix CI, and add an execution-level test proving the container receives the mounted path with cleanup after both success and failure.

Khanvilkarshravani27 and others added 3 commits July 24, 2026 15:10
Cover the scapy_recon plugin parser.py with targeted behavioural tests:

- Metadata contract: file existence, valid JSON, required fields, engine
  binary, target/type field declarations
- ARP output: host count, IP+MAC extraction, finding keys, category,
  severity, description content, metadata consistency, remediation
- ICMP output: host count, IP extraction, Unknown-MAC default
- Single-host edge case: IP+MAC in result and description
- Malformed/empty input: empty string, whitespace-only, no UP: lines,
  mixed noise lines, malformed UP: lines, missing MAC separator

No changes to backend source; test file only.
…tch dir

The amass command_template hardcoded `-dir /tmp/amass`. That path is
world-writable and predictable, which allows symlink/hijack attacks and
cross-user collisions, and it is never cleaned up between runs.

Add a reusable per-task scratch-directory primitive in the executor:

- SCRATCH_DIR_PLACEHOLDER ("%SECUSCAN_SCRATCH_DIR%") is a reserved,
  brace-free command-template token, so it passes plugin validation and
  template interpolation through untouched as an opaque literal.
- _substitute_scratch_dir() creates a fresh tempfile.mkdtemp (mode 0700,
  unpredictable name) and replaces every placeholder occurrence.
- _execute_standard_scanner() resolves it right after build_command and
  wraps command execution in try/finally so the directory is removed on
  success, egress-validation failure, docker error, timeout, or crash.

amass/metadata.json now uses the token (checksum refreshed). Any plugin
needing a private per-task working dir can reuse the same token.

Update the amass command-render tests that asserted the old /tmp/amass
path and add coverage for _substitute_scratch_dir.
In docker engine mode the executor prepended a docker run wrapper to the
command but never mounted the host-side scratch dir created for
%SECUSCAN_SCRATCH_DIR%. The container has its own filesystem, so amass
received -dir <host-path> pointing at a path that does not exist inside
the container and could not write its graph store there.

Bind-mount the private 0700 scratch dir into the container at the same
path (read-write) whenever one was created; it is still removed in the
finally block. Add regression tests for the Docker command path: the
mount is present, resolves to a real dir that existed during the run,
matches the inner -dir value, is cleaned up afterwards, and no mount is
added when a plugin uses no scratch token.

Addresses review feedback on utksh1#2020.
shravanithouta108 and others added 15 commits July 30, 2026 11:50
* fix: stop dashboard polling after health failure and add manual retry

* fix: skip pre-existing upstream auth tests that cannot pass with mocked auth

* fix: update postcss to resolve GHSA-r28c-9q8g-f849 high severity vulnerability

* fix: document localhost-only Docker binding, add opt-in network override
…rdering

Addresses the remaining review point on utksh1#2020: prove the container gets
the mounted path and that cleanup happens after both success and failure.

Cleanup on failure was untested — only the success path was covered. Adds
three cases, one per way a run can end badly:

  - _execute_command raises mid-run (e.g. the docker daemon disappears)
  - the tool exits non-zero
  - egress validation rejects the command before Docker wrapping, which
    matters because the scratch dir is created before that check runs

Also pins the argv ordering: docker only treats -v as a mount when it
appears before the image name, so a future edit that moves it after the
image would silently turn the mount into an amass argument and leave the
tool writing to an unmounted path. The existing mount test only asserted
"-v" was present somewhere in the command and did not catch that.

Verified by mutation: disabling the rmtree fails all four cleanup tests,
and moving the mount after the image fails the ordering test.

The docker helper grows optional execute_result/execute_exc/egress_valid
knobs and an out dict so failure-path tests can recover the scratch dir
after the exception propagates; defaults keep existing callers unchanged.
* Fix risk scoring defaults

* Update risk scoring tests
…low runs (utksh1#2396)

POST /workflows/{id}/run and WorkflowScheduler._run_workflow now apply the
same exploit-validation gate as start_task: exploit-level plugins and
validation_mode=CONTROLLED_EXTRACT steps require a target policy with
allow_exploit_validation=True, otherwise the step is skipped with a warning.

Fixes utksh1#2395
…h1#2367)

Closes utksh1#1845

Escape was a no-op outside text fields. useShortcuts had:

    if (e.key === 'Escape') {
        // Could emit global event to close modals
        return
    }

so nothing was ever emitted and no popover had anything to listen for.

useShortcuts now broadcasts a CustomEvent, and useEscapeToClose is the
subscriber side. Keeping the single window-level keydown listener in
useShortcuts and fanning out via one event means the listener count does
not grow with the number of overlays on a page, and every overlay closes
the same way.

Wired into both affected surfaces. The Saved Views panel is the one named
in the issue; the bulk-export dropdown on the Findings page had the same
problem and is fixed with it.

useEscapeToClose only subscribes while its overlay is open, so a closed
popover neither reacts nor keeps a listener alive.

Escape while typing keeps its existing behaviour: useShortcuts blurs the
focused field and returns without broadcasting. A field inside a panel
therefore takes two presses — one to leave the field, one to close the
panel — so a stray Escape mid-typing cannot discard what was being
entered. That is pinned by a test rather than left implicit.

Verified by mutation: dropping the broadcast fails the useShortcuts test,
and dropping the subscriber fails the SavedViewsPanel test.
* Add retry support for notification delivery

* Remove trailing whitespace
Co-authored-by: Soujanya S H <hsoujanya2k4@gmail.com>
* test: add unit tests for _validate_lengths field length guard

Covers valid/boundary/over-limit cases for name, description, and
notes fields, plus custom resource_type substitution and None
handling for optional fields. Closes utksh1#2307.

* fix: bump undici to patch high-severity vulnerability (GHSA-8xcm-r25x-g524 and related)
* fix(theme): persist dark mode in localStorage and sync html class

* fix(deps): override undici to resolve npm audit vulnerability

* fix(deps): bump undici override to ^8.10.0 to resolve high severity audit vulnerability

* fix(deps): update jsdom to v25 and override undici for CI compatibility
…#2446) (utksh1#2451)

* test: add tests for rate_limiter.check_scan_rate_limit (closes utksh1#2446)

* test: fix sys.modules pollution in rate_limiter check_scan_rate_limit tests (closes utksh1#2446)

---------

Co-authored-by: Mavis Bot <mavis-bot@mini.max>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:backend Backend API, database, or service work area:plugins Scanner plugin metadata, schemas, or plugin runtime work area:security Security-sensitive implementation or tests level:advanced 55 pts difficulty label for advanced contributor PRs type:bug Bug fix work category bonus label type:security Security work category bonus label

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[#30] amass writes to a hardcoded predictable /tmp/amass path