Skip to content

Fixes de-registering datastore options validation#21529

Draft
cgranleese-r7 wants to merge 1 commit into
rapid7:masterfrom
cgranleese-r7:improve-deregistered-datastore-options-validation
Draft

Fixes de-registering datastore options validation#21529
cgranleese-r7 wants to merge 1 commit into
rapid7:masterfrom
cgranleese-r7:improve-deregistered-datastore-options-validation

Conversation

@cgranleese-r7
Copy link
Copy Markdown
Contributor

This PR addresses #21319.

The goal here was to address:

When a module deregisters an option, the validation process should ensure that it is not present when the module runs. It datastore['DEREGISTERED_OPTION'] should probably be nil but at the very least it should be #blank?.
Allowing the caller to pass datastore options that the module explicitly deregistered can break the module authors assumption of how library methods will behave.

This comment calls out some example modules that replicates the issue (thanks for writing those up @zeroSteiner, they were super helpful).

Before

image

After

image

Notes

Specs were added so we if the implementation is changed in the future we will know if options start leaking back in. I also added a spec to cover the shell to Meterpreter workflows.

TODO LIST

  • Shell to Meterpreter workflows still needs tested manually
  • Run Pros test suite

Verification

  • Manually test the shell to Meterpreter workflows
  • Pro test suite goes green
  • Test modules work as expected
  • Code changes are sane

@jenkins-eks-metasploit
Copy link
Copy Markdown

Additional test pipeline started ⌛
Note: build results only accessible to maintainers.

@jenkins-eks-metasploit
Copy link
Copy Markdown

Pipeline results available

Slice summary:

No test slices found.

Note: build results only accessible to maintainers.

@cgranleese-r7 cgranleese-r7 force-pushed the improve-deregistered-datastore-options-validation branch from 7b85389 to 4dab3fc Compare June 5, 2026 09:49
@cgranleese-r7 cgranleese-r7 requested a review from Copilot June 5, 2026 10:03
@jenkins-eks-metasploit
Copy link
Copy Markdown

Additional test pipeline started ⌛
Note: build results only accessible to maintainers.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR addresses metasploit-framework issue #21319 by preventing options that a module has explicitly deregistered from being written into or read back out of a module’s datastore, and adds RSpec coverage to guard against regressions in common workflows (including shell-to-meterpreter handler/payload datastore sharing).

Changes:

  • Add tracking of deregistered option keys inside Msf::ModuleDataStore and filter reads/writes for those keys.
  • Ensure deregistration state is copied alongside datastore state (copy_state) and cleared when a key is re-registered (import_options).
  • Add a new spec suite covering direct assignment, _import_extra_options, framework datastore fallback reads, re-registering previously-deregistered options, and share_datastore shell-to-meterpreter patterns.

Impact Analysis:

  • Blast radius: high — Msf::ModuleDataStore is core infrastructure used broadly across module execution and inter-module workflows.
  • Data and contract effects: changes datastore semantics for deregistered keys (they should no longer be observable); interactions with datastore merging/sharing/copying are particularly sensitive.
  • Rollback and test focus: focus on module invocation from other modules, handler/payload datastore workflows, and any code paths that merge/copy datastores (e.g., console handler flows); rollback should be straightforward (single core file + specs) but correctness should be validated via the full test suite and targeted manual runs mentioned in the PR description.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
lib/msf/core/module_data_store.rb Adds deregistered-key tracking and filtering hooks to prevent deregistered options from being set or resolved (including via framework datastore fallback).
spec/lib/msf/core/module_data_store_filter_spec.rb Adds regression specs for deregistered option filtering across assignment/import/fallback/copy/share and re-registering scenarios.

Comment thread lib/msf/core/module_data_store.rb
Comment thread lib/msf/core/module_data_store.rb
Comment thread lib/msf/core/module_data_store.rb Outdated
Comment thread spec/lib/msf/core/module_data_store_filter_spec.rb
@jenkins-eks-metasploit
Copy link
Copy Markdown

Pipeline results available

Slice summary:

  • Test slice 1 - 🔴
  • Test slice 3 - 🔴
  • Test slice 4 - 🔴

Note: build results only accessible to maintainers.

@cgranleese-r7 cgranleese-r7 force-pushed the improve-deregistered-datastore-options-validation branch from 4dab3fc to dfb1ebb Compare June 5, 2026 12:17
@cgranleese-r7 cgranleese-r7 requested a review from Copilot June 5, 2026 12:17
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

Comment thread lib/msf/core/module_data_store.rb
Comment thread lib/msf/core/module_data_store.rb
@cgranleese-r7 cgranleese-r7 force-pushed the improve-deregistered-datastore-options-validation branch from dfb1ebb to 184334b Compare June 5, 2026 12:24
@cgranleese-r7 cgranleese-r7 force-pushed the improve-deregistered-datastore-options-validation branch from 184334b to 96b43d5 Compare June 5, 2026 12:28
@jenkins-eks-metasploit
Copy link
Copy Markdown

Additional test pipeline started ⌛
Note: build results only accessible to maintainers.

@jenkins-eks-metasploit
Copy link
Copy Markdown

Pipeline results available

Slice summary:

  • Test slice 1 - 🔴
  • Test slice 2 - 🟢
  • Test slice 4 - 🔴

Note: build results only accessible to maintainers.

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

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

3 participants