674529 - Set $CGI::POST_MAX to mitigate DoS attacks

Status: NEW

(Bugzilla :: Bugzilla-General, enhancement)

Description

[Frédéric Buclin]

$CGI::POST_MAX lets us limit the size of POST data passed to CGI scripts. If we exceed this limit, CGI.pm throws a "HTTP/1.1 413 Request entity too large" error. We could use it to prevent Bugzilla from being DoS'ed.

The biggest thing we can have in Bugzilla is attachments. If we include the comment which can be passed at the same time, I would say

  $CGI::POST_MAX = 2* max(Param('maxattachmentsize'), Param('maxlocalattachment'))

would be a reasonable limit.

Comment 1

[Max Kanat-Alexander]

Don't we already set POST_MAX? (In order to be able to upload attachments at all.)

Comment 2

[Frédéric Buclin]

(In reply to comment #1)
> Don't we already set POST_MAX? (In order to be able to upload attachments at
> all.)

We don't, no. Maybe you have MySQL's "max_allowed_packet" variable in mind?

Comment 3

[Max Kanat-Alexander]

(In reply to comment #2)
> We don't, no. Maybe you have MySQL's "max_allowed_packet" variable in mind?

  Huh. I think maybe we used to set it, back in the old days of like 2.18 or something. Maybe I realized it was unnecessary and removed it.

  I would be fine with 2 x max_attachment, that sounds like a good idea.

Comment 4

[Frédéric Buclin]

Attachment: patch, v1

Comment 5

[Max Kanat-Alexander]

Comment on attachment 575924 [details] [diff] [review]
patch, v1

Review of attachment 575924 [details] [diff] [review]:
-----------------------------------------------------------------

::: Bugzilla/CGI.pm
@@ +78,5 @@
>  
> +    # Limit the size of the POST data.
> +    $CGI::POST_MAX = 2 * max(1024 * Bugzilla->params->{maxattachmentsize},
> +                             1024 * 1024 * Bugzilla->params->{maxlocalattachment});
> +

Hmmm, there's no way to set this on $self instead, is there? This will affect other mod_perl apps on the same server, otherwise. Sometimes CGI.pm globals actually initialize some hidden variable in $self, so perhaps we could use that?

Comment 6

[Frédéric Buclin]

Comment on attachment 575924 [details] [diff] [review]
patch, v1

Pushing this patch out of our radar for now, but we may revive it in the future if we have evidence of such problems.