Skip to content

Validate before using `public_send` in Sidekiq workers

Sean McGivern requested to merge remove-send-from-sidekiq-workers into master

Replaced by these MRs:

  1. Stop PagesWorker from using `send` (!103781 - merged)
  2. Verify mount in BackgroundMoveWorker before usi... (!103782 - merged)
  3. Verify method in GitlabShellWorker before using... (!103783 - merged)
  4. Comment on safe `public_send` in DeactivateInte... (!103784 - merged)
  5. Verify method in MailScheduler::NotificationSer... (!103785 - merged)

What does this MR do and why?

A Sidekiq job class that takes a method name as an argument and calls that method (with public_send on an object) is a security risk. It's not typically directly exploitable, but it can be used to escalate from Redis injection to arbitrary code execution (see #371098 (closed) for an example).

We should remove public_send from Sidekiq workers where possible, and where not possible, validate the method name we're passing carefully. I found these workers that disable the Security/PublicSend cop:

PagesWorker

It only had one possible method to call, so let's hard-code that.

GitlabShellWorker

There are only a few valid methods that this could be calling on Gitlab::Shell, so we explicitly list them and check in that list before calling public_send.

This has a feature flag, disabled by default, to make failing that check raise ArgumentError.

ObjectStorage::BackgroundMoveWorker

This is mostly safe anyway, as it doesn't pass any arguments, but we should still check that the given field_field (mount point) is actually an uploader of the correct type before we call it.

This has a feature flag, disabled by default, to make failing that check raise ArgumentError.

DeactivateIntegrationWorker

This was already safe, just added a comment.

MailScheduler::NotificationServiceWorker

This worker is very generic, and it can call any method on NotificationService (including those defined in EE and JH). This change ensures that the method is defined only on those classes, not on a superclass like Object.

This has a feature flag, disabled by default, to make failing that check raise ArgumentError.

For #371470 (closed).

How to set up and validate locally

Mostly just specs.

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Sean McGivern

Merge request reports