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

Status: NEW

(Bugzilla :: Attachments & Requests, enhancement)

Description

[Frédéric Buclin]

Attachment: 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.

Comment 1

[Max Kanat-Alexander]

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?

Comment 2

[Frédéric Buclin]

(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

[Max Kanat-Alexander]

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.

Comment 4

[Frédéric Buclin]

Attachment: 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.

Comment 5

[Max Kanat-Alexander]

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.

Comment 6

[Frédéric Buclin]

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.