Skip to content

Commit

Permalink
Validate days parameter to avoid possible DoS in Web UI
Browse files Browse the repository at this point in the history
Thank you to Sergey Shpakov of http://tutum.space for reporting.
  • Loading branch information
mperham committed Jan 20, 2022
1 parent 0a4de94 commit 7785ac1
Show file tree
Hide file tree
Showing 4 changed files with 17 additions and 2 deletions.
2 changes: 2 additions & 0 deletions lib/sidekiq/api.rb
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,8 @@ def lengths

class History
def initialize(days_previous, start_date = nil)
# we only store five years of data in Redis
raise ArgumentError if days_previous < 1 || days_previous > (5 * 365)
@days_previous = days_previous
@start_date = start_date || Time.now.utc.to_date
end
Expand Down
5 changes: 4 additions & 1 deletion lib/sidekiq/web/application.rb
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,10 @@ def self.set(key, val)

get "/" do
@redis_info = redis_info.select { |k, v| REDIS_KEYS.include? k }
stats_history = Sidekiq::Stats::History.new((params["days"] || 30).to_i)
days = (params["days"] || 30).to_i
return halt(401) if days < 1 || days > 180

stats_history = Sidekiq::Stats::History.new(days)
@processed_history = stats_history.processed
@failed_history = stats_history.failed

Expand Down
9 changes: 9 additions & 0 deletions test/test_api.rb
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,15 @@
Time::DATE_FORMATS[:default] = @before
end

describe "history" do
it "does not allow invalid input" do
assert_raises(ArgumentError) { Sidekiq::Stats::History.new(-1) }
assert_raises(ArgumentError) { Sidekiq::Stats::History.new(0) }
assert_raises(ArgumentError) { Sidekiq::Stats::History.new(2000) }
assert Sidekiq::Stats::History.new(200)
end
end

describe "processed" do
it 'retrieves hash of dates' do
Sidekiq.redis do |c|
Expand Down
3 changes: 2 additions & 1 deletion test/test_web.rb
Original file line number Diff line number Diff line change
Expand Up @@ -748,8 +748,9 @@ def app
basic_authorize 'a', 'b'

get '/'

assert_equal 200, last_response.status
get '/?days=1000000'
assert_equal 401, last_response.status
end
end

Expand Down

2 comments on commit 7785ac1

@nfedyashev
Copy link

Choose a reason for hiding this comment

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

Hi @mperham

I have a question regarding this commit. Is it possible to backport it to 6.2/6.3 version branches as well?

Name: sidekiq
Version: 6.2.2
CVE: CVE-2022-23837
GHSA: GHSA-jrfj-98qg-qjgv
Criticality: High
URL: 7785ac1
Title: Denial of service in sidekiq
Solution: upgrade to >= 6.4.0, ~> 5.2.10

2 major versions upgrade just to get the CVE patch makes it a bit inconvenient.

@mperham
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's two minor versions. I don't backport fixes for minor versions. Why can't you upgrade from 6.2 to 6.4?

Please sign in to comment.