677187 - If the attachment filename contains a newline, an error is thrown when trying to download the attachment

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, defect)

Description

[Mateusz Goik]

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:5.0.1) Gecko/20100101 Firefox/5.0.1
Build ID: 20110707182747

Steps to reproduce:

1. Upload file (mime type = application/octet-stream)
2. Click Attachment 1999 [details] Details for Bug 3284 and SUBMIT
3. Use TemperData Add-ons and change filename to a%22%0d%0aLocation:a.pl


Actual results:

Software error:

Invalid header value contains a newline not followed by whitespace: application/octet-stream; name="a\"

Location:a.pl" at (eval 817) line 25.

For help, please send mail to the webmaster ([e-mail]), giving this error message and the time and date of the error.


Expected results:

Download FIle. -> https://bug3284.landfill.bugzilla.org/p9m4BZJNdO/attachment.cgi?id=1999

Tested on https://landfill.bugzilla.org/p9m4BZJNdO/show_bug.cgi?id=3284 but the error is also on bugzilla.mozilla.org 
Sorry for my English

Comment 1

[Frédéric Buclin]

I can reproduce in 4.1.3 (the reporter tried with 4.0.2). The error occurs when a user tries to view the attachment. The problem appears in view() in attachment.cgi:

 print $cgi->header(-type=>"$contenttype; name=\"$filename\"",
                    -content_disposition=> "$disposition; filename=\"$filename\"",
                    -content_length => $attachment->datasize,
                    -x_content_type_options => "nosniff");

If one of these values contains a quote, it seems to break the CGI headers.
 $attachment->datasize is safe as it's the length of a string.
 $disposition is safe as this string is hardcoded.
 $contenttype I would have to check how the regexp behaves with newlines, but quotes are fine.
 $filename doesn't forbid newlines nor quotes.

Comment 2

[Frédéric Buclin]

(In reply to Frédéric Buclin from comment #1)
>  $filename doesn't forbid newlines nor quotes.

... but quotes are escaped:

  $filename =~ s/"/\\"/g; # escape quotes

Comment 3

[Frédéric Buclin]

The error comes from CGI::header() (in CGI 3.50 and above):

  # From RFC 822:
  # Unfolding  is  accomplished  by regarding   CRLF   immediately
  # followed  by  a  LWSP-char  as equivalent to the LWSP-char.
  $header =~ s/$CRLF(\s)/$1/g;

  # All other uses of newlines are invalid input. 
  if ($header =~ m/$CRLF|\015|\012/) {
      # shorten very long values in the diagnostic
      $header = substr($header,0,72).'...' if (length $header > 72);
      die "Invalid header value contains a newline not followed by whitespace: $header";
  }

So there is no way to abuse CGI by injecting newlines. Note that this check exists since CGI 3.50, per our request in bug 591165. As Bugzilla requires CGI 3.51 or newer, we are protected by this kind of injections.

To me, this is not a security bug, but what we could do in 4.0, 4.2 and 4.3 is to call clean_text() in the validators instead of trim() to correctly remove newlines and prevent this kind of messages.

Comment 4

[Frédéric Buclin]

Attachment: patch, v1

I don't think checksetup.pl needs to scan the attachments table at each run to look for illegal MIME types or filenames (this would only make sense if checksetup.pl had a "run only once" mode). As there is no security implication (from what I can see), I think it's enough to validate attributes from now. If someone gets an error due to an existing illegal filename, all he has to do is to go edit the filename himself.

Comment 5

[Max Kanat-Alexander]

Okay. Since we're sure that newlines cannot be injected, this is not a security issue, as far as I can see. If we agree, feel free to remove the security flag.

Comment 6

[Max Kanat-Alexander]

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

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

Okay, looks good. I'm definitely in support of more thorough validation in any case. Are we sure nothing else valid can show up in Content-Type headers, though? (Other ;something= things?)

Comment 7

[Max Kanat-Alexander]

I'm slightly tentative about taking this on a stable branch, since despite RFCs, I'm not sure if somebody will be putting something valid into the Content-Type header that wouldn't pass our new regex. (particularly something in UTF-8)

Comment 8

[Raymond Forbes[:rforbes]]

Looking at the check inside of CGI:header and how the error message is being presented I am comfortable saying this is not a security vuln and having the security flag removed.

Comment 9

[Max Kanat-Alexander]

Okay. Removing security flag.

Comment 10

[Frédéric Buclin]

(In reply to Max Kanat-Alexander from comment #6)
> any case. Are we sure nothing else valid can show up in Content-Type
> headers, though? (Other ;something= things?)

None that we officially support. For 4.0.3, I will use a less restrictive regexp, just in case:

 /^($legal_types)\/[a-z0-9_\-\+\.]+(;.+)?$/i

Comment 11

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified Bugzilla/Attachment.pm
modified template/en/default/global/user-error.html.tmpl
Committed revision 7915.

Committing to: bzr+ssh://[e-mail]/bugzilla/4.2/
modified Bugzilla/Attachment.pm
modified template/en/default/global/user-error.html.tmpl
Committed revision 7897.

Committing to: bzr+ssh://[e-mail]/bugzilla/4.0/
modified Bugzilla/Attachment.pm
modified template/en/default/global/user-error.html.tmpl
Committed revision 7640.