Bug if attachment contains a newline character in attachment.py

[Ian Clark]

Hi all,

I believe we've found an edge case which isn't correctly caught by the match_request() method in attachment.py:462.

If a filename contains a new line character (e.g. in our case a line feed), then it will never be picked up by the match_request method as the final match group (.*) doesn't account for new lines.

One suggested patch would be to add the re.S flag to the match, but I wanted to check if anyone knew of any issues (e.g. security) which might arise from this, particularly if any other places in Trac Core assume that a filename doesn't contain a new line character.

Kind regards,

Ian

[Steffen Hoffmann]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 21.11.2013 13:56, Ian Clark wrote:
> Hi all,
>
> I believe we've found an edge case which isn't correctly caught by the

> *match_request() *method in attachment.py:462
> <http://trac.edgewall.org/browser/trunk/trac/attachment.py#L462>.

>
> If a filename contains a new line character (e.g. in our case a line
> feed), then it will never be picked up by the match_request method as
> the final match group (.*) doesn't account for new lines.
>

> One suggested patch would be to add the *re.S* flag to the match, but I

> wanted to check if anyone knew of any issues (e.g. security) which might
> arise from this, particularly if any other places in Trac Core assume
> that a filename doesn't contain a new line character.

My personal feeling is to discourage such an insane filename (report it
in a warning?) in the first place. Neither have I encountered such a
wired filename before nor can I see a valid use case and consequently
the need to support it. Is this unrealistic thinking?

Steffen Hoffmann
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAlKODTMACgkQ31DJeiZFuHcDfwCg51NcUpd3/dLkKu73VYfhFbGm
lpgAmwbU61uUAjKsKLOok+YqUY0KDfhD
=F4z5
-----END PGP SIGNATURE-----

[Christopher Nelson]

>> I believe we've found an edge case which isn't correctly caught by the
>> *match_request() *method in attachment.py:462
>> <http://trac.edgewall.org/browser/trunk/trac/attachment.py#L462>.
>>
>> If a filename contains a new line character (e.g. in our case a line
>> feed), then it will never be picked up by the match_request method as
>> the final match group (.*) doesn't account for new lines.
>>
>> One suggested patch would be to add the *re.S* flag to the match, but I
>> wanted to check if anyone knew of any issues (e.g. security) which might
>> arise from this, particularly if any other places in Trac Core assume
>> that a filename doesn't contain a new line character.
>
> My personal feeling is to discourage such an insane filename (report it
> in a warning?) in the first place. Neither have I encountered such a
> wired filename before nor can I see a valid use case and consequently
> the need to support it. Is this unrealistic thinking?

I agree. Spaces in file names is one thing but vertical white space?
That's insane.

[RjOllos]

I'm in agreement on the insane aspect of it, but it seems to work just fine to create a file with a linefeed character on TracStandalone:

$ echo "Some text" > "myfile

"

The linefeed character is encoded as %0A: myfile%0A​

[Olemis Lang]

On Thu, Nov 21, 2013 at 5:42 PM, RjOllos <rjo...@gmail.com> wrote:

On Thursday, November 21, 2013 5:53:21 AM UTC-8, Chris Nelson wrote:

[...] 

> 
> My personal feeling is to discourage such an insane filename (report it
> in a warning?) in the first place. Neither have I encountered such a
> wired filename before nor can I see a valid use case and consequently
> the need to support it. Is this unrealistic thinking?

I agree.  Spaces in file names is one thing but vertical white space?
That's insane.

I'm in agreement on the insane aspect of it, but it seems to work just fine to create a file with a linefeed character on TracStandalone:

$ echo "Some text" > "myfile

"

The linefeed character is encoded as %0A: myfile%0A

IMO let's better filter such file upload requests and return an HTTP 400 Bad Request back to the caller with an informative message .

 

-- 
Regards,

Olemis - @olemislc

Apache™ Bloodhound contributor
http://issues.apache.org/bloodhound
http://blood-hound.net

Blog ES: http://simelo-es.blogspot.com/
Blog EN: http://simelo-en.blogspot.com/

Featured article:

[RjOllos]

On Friday, November 22, 2013 9:48:37 AM UTC-8, Olemis Lang wrote:

On Thu, Nov 21, 2013 at 5:42 PM, RjOllos <rjo...@gmail.com> wrote:

On Thursday, November 21, 2013 5:53:21 AM UTC-8, Chris Nelson wrote:

[...] 

> 
> My personal feeling is to discourage such an insane filename (report it
> in a warning?) in the first place. Neither have I encountered such a
> wired filename before nor can I see a valid use case and consequently
> the need to support it. Is this unrealistic thinking?

I agree.  Spaces in file names is one thing but vertical white space?
That's insane.

I'm in agreement on the insane aspect of it, but it seems to work just fine to create a file with a linefeed character on TracStandalone:

$ echo "Some text" > "myfile

"

The linefeed character is encoded as %0A: myfile%0A

IMO let's better filter such file upload requests and return an HTTP 400 Bad Request back to the caller with an informative message .

What is your reasoning for throwing an error on the request? It seems that Trac handles the case without any issue and nothing breaks when uploading a file with a newline in the filename; it's such an odd scenario that I can't see the need to add any special handling for it in the codebase. I might feel differently if Trac couldn't handle the file.

[Olemis Lang]

On Fri, Nov 22, 2013 at 8:18 PM, RjOllos <rjo...@gmail.com> wrote:

On Friday, November 22, 2013 9:48:37 AM UTC-8, Olemis Lang wrote:

On Thu, Nov 21, 2013 at 5:42 PM, RjOllos <rjo...@gmail.com> wrote:

On Thursday, November 21, 2013 5:53:21 AM UTC-8, Chris Nelson wrote:

[...] 

> 
> My personal feeling is to discourage such an insane filename (report it
> in a warning?) in the first place. Neither have I encountered such a
> wired filename before nor can I see a valid use case and consequently
> the need to support it. Is this unrealistic thinking?

I agree.  Spaces in file names is one thing but vertical white space?
That's insane.

I'm in agreement on the insane aspect of it, but it seems to work just fine to create a file with a linefeed character on TracStandalone:

$ echo "Some text" > "myfile

"

The linefeed character is encoded as %0A: myfile%0A

IMO let's better filter such file upload requests and return an HTTP 400 Bad Request back to the caller with an informative message .

What is your reasoning for throwing an error on the request?

It seems there is consensus on the fact that new line chars should not be allowed . Considering this fact then we have (at least) three options :

  1. Do not allow uploading such attachments at all 

  2. Allow uploads and support new line chars in attachments web UI

  3. Keep things as they are now i.e. allow uploads and still fail to match attachment web UI requests

It seems to me that (1) is the best approach . 

 

It seems that Trac handles the case without any issue and nothing breaks when uploading a file with a newline in the filename;

I see no point in allowing file uploads that will not be reflected in web UI afterwards .

 

it's such an odd scenario that I can't see the need to add any special handling for it in the codebase.

... but maybe you're right and (3) is the best option considering ... performance ?

 

I might feel differently if Trac couldn't handle the file.

 

If Trac could not handle the filename (as should be e.g. the case when file with name containing backslash char \ is uploaded from linux to a windows server) then Trac environment state would be consistent : os error , no attachments in server FS and thereby no match in web UI .

[RjOllos]

On Monday, November 25, 2013 7:39:17 AM UTC-8, Olemis Lang wrote:

On Fri, Nov 22, 2013 at 8:18 PM, RjOllos <rjo...@gmail.com> wrote:

On Friday, November 22, 2013 9:48:37 AM UTC-8, Olemis Lang wrote:

On Thu, Nov 21, 2013 at 5:42 PM, RjOllos <rjo...@gmail.com> wrote:

On Thursday, November 21, 2013 5:53:21 AM UTC-8, Chris Nelson wrote:

[...] 

> 
> My personal feeling is to discourage such an insane filename (report it
> in a warning?) in the first place. Neither have I encountered such a
> wired filename before nor can I see a valid use case and consequently
> the need to support it. Is this unrealistic thinking?

I agree.  Spaces in file names is one thing but vertical white space?
That's insane.

I'm in agreement on the insane aspect of it, but it seems to work just fine to create a file with a linefeed character on TracStandalone:

$ echo "Some text" > "myfile

"

The linefeed character is encoded as %0A: myfile%0A

IMO let's better filter such file upload requests and return an HTTP 400 Bad Request back to the caller with an informative message .

What is your reasoning for throwing an error on the request?

It seems there is consensus on the fact that new line chars should not be allowed . Considering this fact then we have (at least) three options :

  1. Do not allow uploading such attachments at all 

  2. Allow uploads and support new line chars in attachments web UI

  3. Keep things as they are now i.e. allow uploads and still fail to match attachment web UI requests

It seems to me that (1) is the best approach . 

 

It seems that Trac handles the case without any issue and nothing breaks when uploading a file with a newline in the filename;

I see no point in allowing file uploads that will not be reflected in web UI afterwards .

Have you reproduced the issue? My point has been, when I tried to reproduce I found that the newline was encoded, and the files ARE viewable in the web ui. I found no issues when uploading a file that has a newline in the filename.

[Olemis Lang]

OS = Ubuntu 10.04

 

Have you reproduced the issue?

In Opera (Version=12.02 , Build=1578 , Platform=Linux , System=x86_64, 2.6.32-40-generic) the file selection input control renders a red warning message saying "specified one or more files that could not be found" so the file upload is not even possible .

In Firefox 11.0 the new line char is replaced by a single whitespace (i.e. %20) char before upload so there's no actual error

Finally ... when using trac-admin to upload "x\ny.txt" file I can actually see /attachment/ticket/14/x%0Ay.txt link in attachments list . On click I get an error message : 

{{{

Error: Invalid Attachment

Attachment 'ticket:14: x' does not exist.

}}}

JFTR: I tried both Trac (/trunk) + Bloodhound 0.8-dev ;)

My point has been, when I tried to reproduce I found that the newline was encoded, and the files ARE viewable in the web ui.

I found no issues when uploading a file that has a newline in the filename.

Now I understand your previous statement ...

[Jun Omae]

>>>>> I'm in agreement on the insane aspect of it, but it seems to work just
>>>>> fine to create a file with a linefeed character on TracStandalone:
>>>>>
>>>>> $ echo "Some text" > "myfile
>>>>> "
>>>>>
>>>>> The linefeed character is encoded as %0A: myfile%0A

This behavior depends on browser implementation. Firefox replaces
linefeed characters with spaces. I guess that you're using Google
Chrome.

Firefox:
http://hg.mozilla.org/mozilla-central/file/757c2011df5b/content/html/content/src/nsFormSubmission.cpp#l445
http://hg.mozilla.org/mozilla-central/file/757c2011df5b/content/html/content/src/nsFormSubmission.cpp#l521
http://hg.mozilla.org/mozilla-central/file/757c2011df5b/content/html/content/src/nsFormSubmission.cpp#l705

Webkit:
http://trac.webkit.org/browser/trunk/Source/WebCore/platform/network/FormDataBuilder.cpp?rev=159750#L163
http://trac.webkit.org/browser/trunk/Source/WebCore/platform/network/FormDataBuilder.cpp?rev=159750#L56

Also, Trac currently strips whitespaces from filename. Then, it would
be strip the linefeed character after "myfile".

http://trac.edgewall.org/browser/tags/trac-1.0.1/trac/attachment.py?marks=715#L711

>> 1. Do not allow uploading such attachments at all
>> 2. Allow uploads and support new line chars in attachments web UI
>> 3. Keep things as they are now i.e. allow uploads and still fail to
>> match attachment web UI requests
>>
>> It seems to me that (1) is the best approach .

4. Replace unicode control codes with spaces.

See attachment-ctrl-codes.diff.

--
Jun Omae <jun...@gmail.com> (大前 潤)

[Olemis Lang]

On Mon, Nov 25, 2013 at 11:47 AM, Jun Omae <jun...@gmail.com> wrote:

[...] 

 
>>   1. Do not allow uploading such attachments at all
>>   2. Allow uploads and support new line chars in attachments web UI
>>   3. Keep things as they are now i.e. allow uploads and still fail to
>> match attachment web UI requests
>>
>> It seems to me that (1) is the best approach .

4. Replace unicode control codes with spaces.

I'm not very fond of this approach but (like I just said) it seems others are taking the lead on doing so ...

;)

 

See attachment-ctrl-codes.diff.
 

You'll need something similar in attachments admin component, and everywhere an attachment might be created so I guess it's better to add this check in Attachement (model) class itself ?

[RjOllos]

Either (4) or URL-encoding the control codes seems like a good approach. I think users will find it less frustrating than having their upload rejected, and I haven't heard any good cause for needing to reject the files when instead the filenames can just be fixed-up. 

Nice to see WebKit being hosted on Trac! 

[RjOllos]

I made a ticket where we can test and discuss Jun's proposed change:

http://trac.edgewall.org/ticket/11395 

[RjOllos]

Ian, Do you actually have a file in your Trac system that has an embedded newline? If so, how did it get there (e.g. upload through browser, trac-admin add, XmlRpc attachment add, ...)? We should make sure to cover that case in the patch for #11395.

[Ian Clark]

Hi RjOllos,

Sorry for the late reply. Yes we do, my understand is that it was uploaded as an attachment to a milestone.

Ian

[RjOllos]

On Wednesday, February 12, 2014 4:22:20 AM UTC-8, Ian Clark wrote:

Hi RjOllos,

Sorry for the late reply. Yes we do, my understand is that it was uploaded as an attachment to a milestone.

Ian

Changes were finally committed to trunk for inclusion in release 1.1.3. Please follow-up if you find any issues with the changes.

http://trac.edgewall.org/changeset/13603