Skip to content

Commit 191b247

Browse files
committed
Fix #18138: cleanup called twice when Post/Auxiliary modules fail
1 parent e60f77a commit 191b247

5 files changed

Lines changed: 379 additions & 19 deletions

File tree

lib/msf/base/simple/auxiliary.rb

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -84,8 +84,11 @@ def self.run_simple(omod, opts = {}, job_listener: Msf::Simple::NoopJobListener.
8484
omod.job_id = mod.job_id
8585
return [run_uuid, mod.job_id]
8686
else
87-
result = self.job_run_proc(ctx, &:run)
88-
self.job_cleanup_proc(ctx)
87+
begin
88+
result = self.job_run_proc(ctx, &:run)
89+
ensure
90+
self.job_cleanup_proc(ctx)
91+
end
8992

9093
return result
9194
end
@@ -146,10 +149,13 @@ def self.check_simple(mod, opts, job_listener: Msf::Simple::NoopJobListener.inst
146149
[run_uuid, mod.job_id]
147150
else
148151
# Run check if it exists
149-
result = self.job_run_proc(ctx) do |m|
150-
m.check
152+
begin
153+
result = self.job_run_proc(ctx) do |m|
154+
m.check
155+
end
156+
ensure
157+
self.job_cleanup_proc(ctx)
151158
end
152-
self.job_cleanup_proc(ctx)
153159

154160
result
155161
end
@@ -184,7 +190,6 @@ def self.job_run_proc(ctx, &block)
184190
raise
185191
end
186192
rescue Msf::Auxiliary::Complete
187-
mod.cleanup
188193
return
189194
rescue Msf::Auxiliary::Failed => e
190195
mod.error = e
@@ -196,21 +201,18 @@ def self.job_run_proc(ctx, &block)
196201
end
197202
mod.fail_detail ||= e.to_s
198203

199-
mod.cleanup
200204
return
201205
rescue ::Timeout::Error => e
202206
mod.error = e
203207
mod.fail_reason = Msf::Module::Failure::TimeoutExpired
204208
mod.fail_detail ||= e.to_s
205209
mod.print_error("Auxiliary triggered a timeout exception")
206-
mod.cleanup
207210
return
208211
rescue ::Interrupt => e
209212
mod.error = e
210213
mod.fail_reason = Msf::Module::Failure::UserInterrupt
211214
mod.fail_detail ||= e.to_s
212215
mod.print_error("Stopping running against current target...")
213-
mod.cleanup
214216
mod.print_status("Control-C again to force quit all targets.")
215217
begin
216218
Rex.sleep(0.5)
@@ -237,7 +239,6 @@ def self.job_run_proc(ctx, &block)
237239
end
238240

239241
elog('Auxiliary failed', error: e)
240-
mod.cleanup
241242

242243
end
243244
return result

lib/msf/base/simple/post.rb

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -79,8 +79,11 @@ def self.run_simple(omod, opts = {}, &block)
7979
omod.job_id = mod.job_id
8080
else
8181
ctx = [ mod ]
82-
self.job_run_proc(ctx)
83-
self.job_cleanup_proc(ctx)
82+
begin
83+
self.job_run_proc(ctx)
84+
ensure
85+
self.job_cleanup_proc(ctx)
86+
end
8487
end
8588
end
8689

@@ -112,26 +115,21 @@ def self.job_run_proc(ctx)
112115
mod.run
113116
else
114117
mod.print_error("Session not found")
115-
mod.cleanup
116118
return
117119
end
118120
rescue Msf::Post::Complete
119-
mod.cleanup
120121
return
121122
rescue Msf::Post::Failed => e
122123
mod.error = e
123124
mod.print_error("Post aborted due to failure: #{e.message}")
124-
mod.cleanup
125125
return
126126
rescue ::Timeout::Error => e
127127
mod.error = e
128128
mod.print_error("Post triggered a timeout exception")
129-
mod.cleanup
130129
return
131130
rescue ::Interrupt => e
132131
mod.error = e
133132
mod.print_error("Post interrupted by the console user")
134-
mod.cleanup
135133
return
136134
rescue ::Msf::OptionValidateError => e
137135
mod.error = e
@@ -148,7 +146,6 @@ def self.job_run_proc(ctx)
148146
end
149147

150148
elog('Post failed', error: e)
151-
mod.cleanup
152149

153150
return
154151
end

lib/msf/core/exploit_driver.rb

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -176,7 +176,8 @@ def run
176176
begin
177177
job_run_proc(ctx)
178178
rescue ::Interrupt
179-
job_cleanup_proc(ctx)
179+
# ensure skips cleanup when keep_handler is true, so handle it here
180+
job_cleanup_proc(ctx) if keep_handler
180181
raise $!
181182
ensure
182183
# For multi exploit targets.
Lines changed: 139 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,139 @@
1+
# -*- coding: binary -*-
2+
3+
require 'spec_helper'
4+
5+
RSpec.describe Msf::Simple::Auxiliary do
6+
let(:cleanup_calls) { [] }
7+
8+
let(:events_double) do
9+
double('events').tap do |e|
10+
allow(e).to receive(:on_module_run)
11+
allow(e).to receive(:on_module_complete)
12+
end
13+
end
14+
15+
let(:framework_double) do
16+
double('framework', events: events_double)
17+
end
18+
19+
let(:job_listener) { Msf::Simple::NoopJobListener.instance }
20+
21+
let(:mod) do
22+
calls_ref = cleanup_calls
23+
fw_double = framework_double
24+
25+
instance = Object.new
26+
instance.define_singleton_method(:framework) { fw_double }
27+
instance.define_singleton_method(:setup) {}
28+
instance.define_singleton_method(:print_error) { |_msg| }
29+
instance.define_singleton_method(:print_status) { |_msg| }
30+
instance.define_singleton_method(:elog) { |_msg, **_kw| }
31+
instance.define_singleton_method(:fail_reason) { Msf::Module::Failure::None }
32+
instance.define_singleton_method(:fail_reason=) { |_v| }
33+
instance.define_singleton_method(:fail_detail) { nil }
34+
instance.define_singleton_method(:fail_detail=) { |_v| }
35+
instance.define_singleton_method(:error=) { |_v| }
36+
instance.define_singleton_method(:report_failure) {}
37+
instance.define_singleton_method(:cleanup) { calls_ref << 1 }
38+
instance
39+
end
40+
41+
let(:run_uuid) { 'test-run-uuid-1234' }
42+
let(:ctx) { [mod, run_uuid, job_listener] }
43+
44+
def run_proc(ctx, &block)
45+
block ||= proc { |_m| }
46+
described_class.send(:job_run_proc, ctx, &block)
47+
end
48+
49+
def cleanup_proc(ctx)
50+
described_class.send(:job_cleanup_proc, ctx)
51+
end
52+
53+
describe '.job_run_proc' do
54+
context 'when the module raises Auxiliary::Failed' do
55+
it 'does not call cleanup' do
56+
run_proc(ctx) { raise Msf::Auxiliary::Failed, 'intentional failure' }
57+
expect(cleanup_calls.length).to eq(0)
58+
end
59+
end
60+
61+
context 'when the module raises Auxiliary::Complete' do
62+
it 'does not call cleanup' do
63+
run_proc(ctx) { raise Msf::Auxiliary::Complete }
64+
expect(cleanup_calls.length).to eq(0)
65+
end
66+
end
67+
68+
context 'when the module raises Timeout::Error' do
69+
it 'does not call cleanup' do
70+
run_proc(ctx) { raise ::Timeout::Error }
71+
expect(cleanup_calls.length).to eq(0)
72+
end
73+
end
74+
75+
context 'when the module raises a generic Exception' do
76+
it 'does not call cleanup' do
77+
run_proc(ctx) { raise 'unexpected error' }
78+
expect(cleanup_calls.length).to eq(0)
79+
end
80+
end
81+
82+
context 'when the module completes normally' do
83+
it 'does not call cleanup' do
84+
run_proc(ctx) { nil }
85+
expect(cleanup_calls.length).to eq(0)
86+
end
87+
end
88+
end
89+
90+
describe '.job_cleanup_proc' do
91+
it 'calls cleanup exactly once' do
92+
cleanup_proc(ctx)
93+
expect(cleanup_calls.length).to eq(1)
94+
end
95+
end
96+
97+
describe 'cleanup is called exactly once across job_run_proc + job_cleanup_proc' do
98+
def run_then_cleanup(ctx, &block)
99+
block ||= proc { |_m| }
100+
run_proc(ctx, &block)
101+
cleanup_proc(ctx)
102+
end
103+
104+
context 'when the module raises Auxiliary::Failed' do
105+
it 'calls cleanup exactly once total' do
106+
run_then_cleanup(ctx) { raise Msf::Auxiliary::Failed, 'intentional failure' }
107+
expect(cleanup_calls.length).to eq(1)
108+
end
109+
end
110+
111+
context 'when the module raises Auxiliary::Complete' do
112+
it 'calls cleanup exactly once total' do
113+
run_then_cleanup(ctx) { raise Msf::Auxiliary::Complete }
114+
expect(cleanup_calls.length).to eq(1)
115+
end
116+
end
117+
118+
context 'when the module raises Timeout::Error' do
119+
it 'calls cleanup exactly once total' do
120+
run_then_cleanup(ctx) { raise ::Timeout::Error }
121+
expect(cleanup_calls.length).to eq(1)
122+
end
123+
end
124+
125+
context 'when the module raises a generic Exception' do
126+
it 'calls cleanup exactly once total' do
127+
run_then_cleanup(ctx) { raise 'unexpected error' }
128+
expect(cleanup_calls.length).to eq(1)
129+
end
130+
end
131+
132+
context 'when the module completes normally' do
133+
it 'calls cleanup exactly once total' do
134+
run_then_cleanup(ctx) { nil }
135+
expect(cleanup_calls.length).to eq(1)
136+
end
137+
end
138+
end
139+
end

0 commit comments

Comments
 (0)