Skip to content

Commit 10981c8

Browse files
Maffoochclaude
andcommitted
security: remove pickle from forms and Celery serializer
Pickle deserialization on attacker-controllable bytes is arbitrary code execution. Two sites used pickle: - The survey choice-question form pickled/unpickled a list of strings through MultiExampleField.compress / MultiWidgetBasic.decompress and pickle.loads in survey/views.py. Switched to json — the data is just a list of up to 6 strings. - Celery defaulted to the pickle serializer with CELERY_ACCEPT_CONTENT including pickle/yaml/msgpack so dispatch sites could pass Django model instances and a Dojo_User on the wire. Made every task argument JSON-serializable: async_delete_task takes (model_label, pk) and refetches; SLA recompute takes (sla_config_id, product_ids); per-channel notification sends run inline inside the surrounding async_create_notification task instead of dispatching an inner Celery task with model instances; user context is injected as async_user_id and resolved in the worker. Flipped DD_CELERY_TASK_SERIALIZER default to json, tightened CELERY_ACCEPT_CONTENT to ["json"], and added CELERY_RESULT_SERIALIZER = "json". Added unittests/test_no_pickle.py as a regression net (asserts no pickle imports in dojo/ and that the Celery serializer settings stay JSON-only) and unittests/test_survey_forms.py for the widget round-trip. Operator note: workers running this version reject in-flight pickled messages with ContentDisallowed. Drain the broker (\`celery -A dojo purge -f\`) or scale workers to zero before deploy. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 9f4d6c2 commit 10981c8

13 files changed

Lines changed: 140 additions & 55 deletions

File tree

dojo/celery.py

Lines changed: 14 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -28,23 +28,26 @@ def __call__(self, *args, **kwargs):
2828
"""
2929
Restore user context in the celery worker via crum.impersonate.
3030
31-
The apply_async method injects ``async_user`` into kwargs when a task
32-
is dispatched. Here we pop it and set it as the current user in
33-
thread-local storage so that all downstream code — including nested
34-
dojo_dispatch_task calls — sees the correct user via
35-
get_current_user().
31+
The apply_async method injects ``async_user_id`` into kwargs when a task
32+
is dispatched. Here we pop it, resolve to a user instance, and set it
33+
as the current user in thread-local storage so that all downstream
34+
code — including nested dojo_dispatch_task calls — sees the correct
35+
user via get_current_user().
3636
37-
When a task is called directly (not via apply_async), async_user is
37+
When a task is called directly (not via apply_async), async_user_id is
3838
not in kwargs. In that case we leave the existing crum context
3939
intact so that callers who already set a user (e.g. via
4040
crum.impersonate in tests or request middleware) are not disrupted.
4141
"""
42-
if "async_user" not in kwargs:
42+
if "async_user_id" not in kwargs:
4343
return super().__call__(*args, **kwargs)
4444

4545
import crum # noqa: PLC0415
4646

47-
user = kwargs.pop("async_user")
47+
from dojo.models import Dojo_User # noqa: PLC0415 circular import
48+
49+
user_id = kwargs.pop("async_user_id")
50+
user = Dojo_User.objects.filter(pk=user_id).first() if user_id else None
4851
with crum.impersonate(user):
4952
return super().__call__(*args, **kwargs)
5053

@@ -59,8 +62,9 @@ def apply_async(self, args=None, kwargs=None, **options):
5962
# Inject user context for Dojo tasks only. Celery built-in tasks (e.g.
6063
# celery.backend_cleanup) do not accept custom kwargs.
6164
task_name = self.name or ""
62-
if not task_name.startswith("celery.") and "async_user" not in kwargs:
63-
kwargs["async_user"] = get_current_user()
65+
if not task_name.startswith("celery.") and "async_user_id" not in kwargs:
66+
user = get_current_user()
67+
kwargs["async_user_id"] = user.id if user else None
6468

6569
# Control flag used for sync/async decision; never pass into the task itself
6670
kwargs.pop("sync", None)
@@ -135,8 +139,6 @@ def __call__(self, *args, **kwargs):
135139

136140
app = Celery("dojo", task_cls=PgHistoryTask)
137141

138-
# Using a string here means the worker will not have to
139-
# pickle the object when using Windows.
140142
app.config_from_object("django.conf:settings", namespace="CELERY")
141143

142144
app.autodiscover_tasks(lambda: settings.INSTALLED_APPS)

dojo/celery_dispatch.py

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,10 +18,11 @@ def apply_async(self, args: Any | None = None, kwargs: Any | None = None, **opti
1818

1919
def _inject_async_user(kwargs: Mapping[str, Any] | None) -> dict[str, Any]:
2020
result: dict[str, Any] = dict(kwargs or {})
21-
if "async_user" not in result:
21+
if "async_user_id" not in result:
2222
from dojo.utils import get_current_user # noqa: PLC0415 circular import
2323

24-
result["async_user"] = get_current_user()
24+
user = get_current_user()
25+
result["async_user_id"] = user.id if user else None
2526
return result
2627

2728

@@ -58,7 +59,7 @@ def dojo_dispatch_task(task_or_sig: _SupportsSi | _SupportsApplyAsync | Signatur
5859
"""
5960
Dispatch a task/signature using DefectDojo semantics.
6061
61-
- Inject `async_user` if missing.
62+
- Inject `async_user_id` if missing.
6263
- Capture and inject pghistory context if available.
6364
- Respect `sync=True` (foreground execution) and user `block_execution`.
6465
- Support `countdown=<seconds>` for async dispatch.

dojo/decorators.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ def __wrapper__(*args, **kwargs):
8888
from dojo.utils import get_current_user # noqa: PLC0415 circular import
8989

9090
user = get_current_user()
91-
kwargs["async_user"] = user
91+
kwargs["async_user_id"] = user.id if user else None
9292

9393
# Capture pghistory context to pass to Celery worker
9494
# The PgHistoryTask base class will apply this context in the worker

dojo/forms.py

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
1+
import json
12
import logging
2-
import pickle
33
import re
44
import warnings
55
from datetime import date, datetime
@@ -3522,7 +3522,7 @@ def __init__(self, attrs=None):
35223522

35233523
def decompress(self, value):
35243524
if value:
3525-
return pickle.loads(value)
3525+
return json.loads(value)
35263526
return [None, None, None, None, None, None]
35273527

35283528
def format_output(self, rendered_widgets):
@@ -3542,7 +3542,7 @@ def __init__(self, *args, **kwargs):
35423542
super().__init__(list_fields, *args, **kwargs)
35433543

35443544
def compress(self, values):
3545-
return pickle.dumps(values)
3545+
return json.dumps(values)
35463546

35473547

35483548
class CreateChoiceQuestionForm(forms.Form):

dojo/models.py

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1094,7 +1094,12 @@ def save(self, *args, **kwargs):
10941094
from dojo.sla_config.helpers import async_update_sla_expiration_dates_sla_config_sync # noqa: I001, PLC0415 circular import
10951095
from dojo.celery_dispatch import dojo_dispatch_task # noqa: PLC0415 circular import
10961096

1097-
dojo_dispatch_task(async_update_sla_expiration_dates_sla_config_sync, self, products, severities=severities)
1097+
dojo_dispatch_task(
1098+
async_update_sla_expiration_dates_sla_config_sync,
1099+
self.id,
1100+
list(products.values_list("id", flat=True)),
1101+
severities=severities,
1102+
)
10981103

10991104
def clean(self):
11001105
sla_days = [self.critical, self.high, self.medium, self.low]
@@ -1256,7 +1261,11 @@ def save(self, *args, **kwargs):
12561261
from dojo.sla_config.helpers import async_update_sla_expiration_dates_sla_config_sync # noqa: I001, PLC0415 circular import
12571262
from dojo.celery_dispatch import dojo_dispatch_task # noqa: PLC0415 circular import
12581263

1259-
dojo_dispatch_task(async_update_sla_expiration_dates_sla_config_sync, sla_config, Product.objects.filter(id=self.id))
1264+
dojo_dispatch_task(
1265+
async_update_sla_expiration_dates_sla_config_sync,
1266+
sla_config.id,
1267+
[self.id],
1268+
)
12601269

12611270
def get_absolute_url(self):
12621271
return reverse("view_product", args=[str(self.id)])

dojo/notifications/helper.py

Lines changed: 9 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@
1818
from dojo import __version__ as dd_version
1919
from dojo.authorization.roles_permissions import Permissions
2020
from dojo.celery import app
21-
from dojo.celery_dispatch import dojo_dispatch_task
2221
from dojo.decorators import we_want_async
2322
from dojo.labels import get_labels
2423
from dojo.models import (
@@ -833,58 +832,43 @@ def _process_notifications(
833832

834833
# Some errors should not be pushed to all channels, only to alerts.
835834
# For example reasons why JIRA Issues: https://github.com/DefectDojo/django-DefectDojo/issues/11575
835+
# Per-channel sends run synchronously inside the surrounding async_create_notification
836+
# task body. Dispatching inner Celery tasks would require JSON-serializable kwargs, but
837+
# callers pass model instances (finding/test/engagement/product/...) and refetching every
838+
# one of them per channel would multiply DB queries; running synchronously avoids both.
836839
if not alert_only:
840+
user_id = getattr(notifications.user, "id", None)
837841
if self.system_settings.enable_slack_notifications and "slack" in getattr(
838842
notifications,
839843
event,
840844
notifications.other,
841845
):
842846
logger.debug("Sending Slack Notification")
843-
dojo_dispatch_task(
844-
send_slack_notification,
845-
event,
846-
user_id=getattr(notifications.user, "id", None),
847-
**kwargs,
848-
)
847+
send_slack_notification.run(event, user_id=user_id, **kwargs)
849848

850849
if self.system_settings.enable_msteams_notifications and "msteams" in getattr(
851850
notifications,
852851
event,
853852
notifications.other,
854853
):
855854
logger.debug("Sending MSTeams Notification")
856-
dojo_dispatch_task(
857-
send_msteams_notification,
858-
event,
859-
user_id=getattr(notifications.user, "id", None),
860-
**kwargs,
861-
)
855+
send_msteams_notification.run(event, user_id=user_id, **kwargs)
862856

863857
if self.system_settings.enable_mail_notifications and "mail" in getattr(
864858
notifications,
865859
event,
866860
notifications.other,
867861
):
868862
logger.debug("Sending Mail Notification")
869-
dojo_dispatch_task(
870-
send_mail_notification,
871-
event,
872-
user_id=getattr(notifications.user, "id", None),
873-
**kwargs,
874-
)
863+
send_mail_notification.run(event, user_id=user_id, **kwargs)
875864

876865
if self.system_settings.enable_webhooks_notifications and "webhooks" in getattr(
877866
notifications,
878867
event,
879868
notifications.other,
880869
):
881870
logger.debug("Sending Webhooks Notification")
882-
dojo_dispatch_task(
883-
send_webhooks_notification,
884-
event,
885-
user_id=getattr(notifications.user, "id", None),
886-
**kwargs,
887-
)
871+
send_webhooks_notification.run(event, user_id=user_id, **kwargs)
888872

889873

890874
@app.task

dojo/settings/settings.dist.py

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@
8888
DD_CELERY_RESULT_BACKEND=(str, "django-db"),
8989
DD_CELERY_RESULT_EXPIRES=(int, 86400),
9090
DD_CELERY_BEAT_SCHEDULE_FILENAME=(str, root("dojo.celery.beat.db")),
91-
DD_CELERY_TASK_SERIALIZER=(str, "pickle"),
91+
DD_CELERY_TASK_SERIALIZER=(str, "json"),
9292
DD_CELERY_LOG_LEVEL=(str, "INFO"),
9393
# Hard ceiling on task runtime. When reached, the worker process is sent SIGKILL — no cleanup
9494
# code runs. Always set higher than DD_CELERY_TASK_SOFT_TIME_LIMIT. (0 = disabled, no limit)
@@ -1267,8 +1267,9 @@ def saml2_attrib_map_format(din):
12671267
CELERY_TIMEZONE = TIME_ZONE
12681268
CELERY_RESULT_EXPIRES = env("DD_CELERY_RESULT_EXPIRES")
12691269
CELERY_BEAT_SCHEDULE_FILENAME = env("DD_CELERY_BEAT_SCHEDULE_FILENAME")
1270-
CELERY_ACCEPT_CONTENT = ["pickle", "json", "msgpack", "yaml"]
1270+
CELERY_ACCEPT_CONTENT = ["json"]
12711271
CELERY_TASK_SERIALIZER = env("DD_CELERY_TASK_SERIALIZER")
1272+
CELERY_RESULT_SERIALIZER = "json"
12721273
CELERY_LOG_LEVEL = env("DD_CELERY_LOG_LEVEL")
12731274

12741275
if env("DD_CELERY_TASK_TIME_LIMIT") > 0:
@@ -1292,7 +1293,6 @@ def saml2_attrib_map_format(din):
12921293
"add-alerts": {
12931294
"task": "dojo.tasks.add_alerts",
12941295
"schedule": timedelta(hours=1),
1295-
"args": [timedelta(hours=1)],
12961296
"options": {
12971297
"expires": int(60 * 60 * 1 * 1.2), # If a task is not executed within 72 minutes, it should be dropped from the queue. Two more tasks should be scheduled in the meantime.
12981298
},

dojo/sla_config/helpers.py

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,12 @@
88

99

1010
@app.task
11-
def async_update_sla_expiration_dates_sla_config_sync(sla_config: SLA_Configuration, products: list[Product], *args, severities: list[str] | None = None, **kwargs):
11+
def async_update_sla_expiration_dates_sla_config_sync(sla_config_id: int, product_ids: list[int], *args, severities: list[str] | None = None, **kwargs):
12+
sla_config = SLA_Configuration.objects.filter(pk=sla_config_id).first()
13+
if sla_config is None:
14+
logger.info("SLA_Configuration with id %s no longer exists, skipping update", sla_config_id)
15+
return
16+
products = Product.objects.filter(pk__in=product_ids)
1217
if method := get_custom_method("FINDING_SLA_EXPIRATION_CALCULATION_METHOD"):
1318
method(sla_config, products, severities=severities)
1419
else:

dojo/survey/views.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import pickle
1+
import json
22
from datetime import date, timedelta
33

44
from django.contrib import messages
@@ -490,7 +490,7 @@ def create_question(request):
490490
order=form.cleaned_data["order"],
491491
text=form.cleaned_data["text"],
492492
multichoice=choiceQuestionFrom.cleaned_data["multichoice"])
493-
choices_to_process = pickle.loads(choiceQuestionFrom.cleaned_data["answer_choices"])
493+
choices_to_process = json.loads(choiceQuestionFrom.cleaned_data["answer_choices"])
494494

495495
for c in choices_to_process:
496496
if c is not None and len(c) > 0:

dojo/tasks.py

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,9 @@ def log_generic_alert(source, title, description):
3232

3333

3434
@app.task(bind=True)
35-
def add_alerts(self, runinterval, *args, **kwargs):
35+
def add_alerts(self, *args, **kwargs):
36+
# Run interval matches the beat schedule for this task (see CELERY_BEAT_SCHEDULE).
37+
runinterval = timedelta(hours=1)
3638
now = timezone.now()
3739

3840
upcoming_engagements = Engagement.objects.filter(target_start__gt=now + timedelta(days=3), target_start__lt=now + timedelta(days=3) + runinterval).order_by("target_start")

0 commit comments

Comments
 (0)