Skip to content

fix(security): log every scan attempt blocked before execution#1698

Open
anshul23102 wants to merge 1 commit into
utksh1:mainfrom
anshul23102:fix/1623-audit-log-blocked-attempts
Open

fix(security): log every scan attempt blocked before execution#1698
anshul23102 wants to merge 1 commit into
utksh1:mainfrom
anshul23102:fix/1623-audit-log-blocked-attempts

Conversation

@anshul23102

Copy link
Copy Markdown
Contributor

Description

Fixes #1623

Previously, the audit log only recorded scans that passed every gate and began executing, via executor.create_task's "task_created" event. Any attempt rejected earlier — missing consent, unknown plugin, invalid preset, unauthorised target/credential/session policy, invalid input, or a target blocked by safe mode — raised its HTTPException and exited the code path before ever reaching a log write. An administrator had no way to determine, post-incident, whether a user had attempted to scan an unauthorised target: the misuse simply left no trace.

Approach

Added _log_blocked_scan_attempt, a thin wrapper around the existing db.log_audit with severity="warning", and called it at every rejection point in POST /task/start before its raise:

  • task_blocked_invalid_payload — payload size/field-length guard
  • task_blocked_consent — consent not granted
  • task_blocked_plugin_not_found — unknown plugin_id
  • task_blocked_invalid_preset — preset not valid for the plugin
  • task_blocked_policy — target/credential/session policy missing or insufficiently permissive
  • task_blocked_invalid_input — timeout/max_scan_time out of bounds
  • task_blocked_safe_mode — target rejected or validation timed out under safe mode (the exact scenario the issue's reproduction steps describe)
  • task_blocked_rate_limit — per-client-per-plugin quota exceeded

Moved db = await get_db() to the top of start_task so it's available to every rejection branch, including ones that previously ran before the database handle was fetched.

Related Issues

Closes #1623

Type of Change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

New test file testing/backend/integration/test_task_start_audit_log.py (5 tests):

  • missing consent → task_blocked_consent audit entry, severity warning
  • unknown plugin → task_blocked_plugin_not_found entry
  • invalid preset → task_blocked_invalid_preset entry
  • safe-mode target rejection (public IP with no permissive target policy) → task_blocked_safe_mode entry with the target in context
  • regression guard: the existing successful-path task_created entry still fires exactly once, unaffected by the new instrumentation

Full suite run locally:

pytest testing/backend/unit -q -m "not benchmark"        # 2210 passed (6 pre-existing failures, confirmed identical on unmodified main — sandbox/subprocess tests, unrelated to this change)
pytest testing/backend/integration -q -m "not benchmark"  # 289 passed, 9 skipped (284 baseline + 5 new)
ruff check backend testing/backend                          # all checks passed
scripts/check-artifacts.sh origin/main                       # clean

Checklist

  • My code follows the code style of this project.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.

Additional Notes

I'm contributing this through GSSoC. While building the test for the safe-mode rejection path, I found a separate, more serious issue: start_task's safe-mode target check only looks at the literal "target" input key (if target := effective_inputs.get("target")), but plugins can declare their field under a different name — http_inspector's schema uses "url". For any plugin whose field isn't literally named "target", safe-mode validation is silently skipped entirely (confirmed: validate_target("http://8.8.8.8", True) correctly rejects it, but the request-level check never reaches it for http_inspector). I've flagged that separately rather than expanding this PR's scope, since it's a distinct bug from the audit-log gap this PR fixes.

Previously, the audit log only recorded scans that passed every gate and
began executing, via executor.create_task's "task_created" event. Any
attempt rejected earlier -- missing consent, unknown plugin, invalid
preset, unauthorised target/credential/session policy, invalid input,
or a target blocked by safe mode -- raised its HTTPException and exited
the code path before ever reaching a log write. An administrator had no
way to determine, post-incident, whether a user had attempted to scan an
unauthorised target: the misuse simply left no trace.

Fix: added _log_blocked_scan_attempt, a thin wrapper around the
existing db.log_audit with severity="warning", and called it at every
rejection point in POST /task/start before its raise:

- task_blocked_invalid_payload  (payload size/field-length guard)
- task_blocked_consent          (consent not granted)
- task_blocked_plugin_not_found (unknown plugin_id)
- task_blocked_invalid_preset   (preset not valid for the plugin)
- task_blocked_policy           (target/credential/session policy
                                  missing or insufficiently permissive)
- task_blocked_invalid_input    (timeout/max_scan_time out of bounds)
- task_blocked_safe_mode        (target rejected or validation timed
                                  out under safe mode -- the scenario
                                  the issue's repro steps describe)
- task_blocked_rate_limit       (per-client-per-plugin quota exceeded)

Moved db = await get_db() to the top of start_task so it's available
to every rejection branch, including ones that previously ran before
the database handle was fetched.

Testing:
- testing/backend/integration/test_task_start_audit_log.py: 5 new tests
  covering missing consent, unknown plugin, invalid preset, and safe-mode
  target rejection all producing an audit_log row with severity
  "warning", plus a regression guard confirming the existing successful-
  path 'task_created' entry is unaffected (exactly one entry, not zero
  or duplicated).
- pytest testing/backend/unit -q -m "not benchmark" -- 2210 passed (6
  pre-existing failures unrelated to this change, confirmed identical on
  unmodified main -- sandbox/subprocess tests failing in this sandbox)
- pytest testing/backend/integration -q -m "not benchmark" -- 289 passed,
  9 skipped (284 baseline + 5 new)
- ruff check backend testing/backend -- all checks passed
- scripts/check-artifacts.sh origin/main -- clean

Fixes utksh1#1623
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: audit log omits blocked and failed scan attempts, breaking forensics

1 participant