Bug if attachment contains a newline character in attachment.py

75 views
Skip to first unread message

Ian Clark

unread,
Nov 21, 2013, 7:56:23 AM11/21/13
to trac...@googlegroups.com
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

unread,
Nov 21, 2013, 8:40:07 AM11/21/13
to trac...@googlegroups.com
-----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

unread,
Nov 21, 2013, 8:53:21 AM11/21/13
to trac...@googlegroups.com
>> 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

unread,
Nov 21, 2013, 5:42:22 PM11/21/13
to trac...@googlegroups.com
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

unread,
Nov 22, 2013, 12:48:37 PM11/22/13
to trac-dev
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

unread,
Nov 22, 2013, 8:18:21 PM11/22/13
to trac...@googlegroups.com
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

unread,
Nov 25, 2013, 10:39:17 AM11/25/13
to trac-dev
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

unread,
Nov 25, 2013, 10:54:03 AM11/25/13
to trac...@googlegroups.com


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

unread,
Nov 25, 2013, 11:44:26 AM11/25/13
to trac-dev
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

unread,
Nov 25, 2013, 11:47:44 AM11/25/13
to trac...@googlegroups.com
>>>>> 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> (大前 潤)
attachment-ctrl-codes.diff

Olemis Lang

unread,
Nov 25, 2013, 11:55:43 AM11/25/13
to trac-dev

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

unread,
Nov 26, 2013, 9:27:32 PM11/26/13
to trac...@googlegroups.com
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

unread,
Dec 12, 2013, 12:02:23 PM12/12/13
to trac...@googlegroups.com
I made a ticket where we can test and discuss Jun's proposed change:

RjOllos

unread,
Dec 13, 2013, 3:13:05 PM12/13/13
to trac...@googlegroups.com
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

unread,
Feb 12, 2014, 7:22:20 AM2/12/14
to trac...@googlegroups.com
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

unread,
Jan 3, 2015, 3:26:22 AM1/3/15
to trac...@googlegroups.com

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
Reply all
Reply to author
Forward
0 new messages