Validate before using `public_send` in Sidekiq workers
Replaced by these MRs:
- Stop PagesWorker from using `send` (!103781 - merged)
- Verify mount in BackgroundMoveWorker before usi... (!103782 - merged)
- Verify method in GitlabShellWorker before using... (!103783 - merged)
- Comment on safe `public_send` in DeactivateInte... (!103784 - merged)
- 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.
-
I have evaluated the MR acceptance checklist for this MR.