Set $CGI::DISABLE_UPLOADS = 1 by default to mitigate DoS attacks

NEW
Unassigned

Status

()

Bugzilla
Attachments & Requests
--
enhancement
7 years ago
5 years ago

People

(Reporter: Frédéric Buclin, Unassigned)

Tracking

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

1.02 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

7 years ago
Created attachment 548776 [details] [diff] [review]
patch, v1

$CGI::DISABLE_UPLOADS = 1 makes CGI.pm to ignore fields of type 'file'. This avoids making it process unnecessary large data when they are unexpected. Only attachment.cgi and post_bug.cgi are supposed to get attachments.
Attachment #548776 - Flags: review?(mkanat)

Updated

7 years ago
Target Milestone: --- → Bugzilla 5.0

Comment 1

7 years ago
Comment on attachment 548776 [details] [diff] [review]
patch, v1

Hmm, interesting thought. When does DISABLE_UPLOADS take effect? If it's very late, perhaps we could simply set it inside of Bugzilla::Attachment->create? Or refactor Bugzilla so that we only have to set it there or something?
(Reporter)

Comment 2

7 years ago
(In reply to comment #1)
> Hmm, interesting thought. When does DISABLE_UPLOADS take effect? If it's
> very late

This happens very early, when we call Bugzilla->cgi for the very first time: internally, this happens in CGI->new() which calls init() which calls read_multipart() which checks this parameter to know what to do with uploads (if it must skip them or store them in a temporary file).

Comment 3

7 years ago
Comment on attachment 548776 [details] [diff] [review]
patch, v1

Okay. In that case we need some hookable constant named ALLOW_UPLOADS or something, perhaps even as a variable in Bugzilla::CGI. Then we should check $0 in Bugzilla::CGI->new against scripts listed in ALLOW_UPLOADS and set DISABLE_UPLOADS appropriately.
Attachment #548776 - Flags: review?(mkanat) → review-
(Reporter)

Comment 4

7 years ago
Created attachment 575914 [details] [diff] [review]
patch, v2

I list both attachment.cgi and post_bug.cgi in a constant within CGI.pm. I don't think we need a hook for now unless someone can show evidence that this would cause any trouble.
Attachment #548776 - Attachment is obsolete: true
Attachment #575914 - Flags: review?(mkanat)

Comment 5

7 years ago
Okay, so the patch looks okay, but I'm concerned about breaking extensions. In general, I'm actually not quite sure that this code is needed--I'm not familiar with any attacks that have been done against Bugzilla that this would protect us against, and I think it would make life more complex for customizers and extension writers.
(Reporter)

Comment 6

6 years ago
Comment on attachment 575914 [details] [diff] [review]
patch, v2

Pushing this patch out of our radar for now, but we may revive it in the future if we have evidence of such problems.
Attachment #575914 - Flags: review?(mkanat)
(Reporter)

Updated

6 years ago
Assignee: LpSolit → attach-and-request
Status: ASSIGNED → NEW
Target Milestone: Bugzilla 4.4 → ---
You need to log in before you can comment on or make changes to this bug.