178841 - file name field must *not* include full windows paths

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, defect)

Description

[timeless]

C:\WINNT\Profiles\pschwartau\Desktop\169707_TALKBACK.txt
Attachment Type Created Flags Actions
Talkback stack traces (2) text/plain 2002-09-20 14:37:32 none Edit

------- Additional Comment #6 From Phil Schwartau 2002-09-20 14:41 -------

The stack traces both show crashes in the MRJ plug-in.
Reassigning to Patrick and OJI component -

sorry phil, it was a random attachment number (generated by zeroing)

Comment 1

[Joel Peshkin]

And this cannot be fixed by just doing...

    $filename =~ s/^.*[\/\\]//;        

as this does not handle escaped spaces in filenames and tricks tainting.

Comment 2

[Joel Peshkin]

use File::Spec->splitpath instead

Comment 3

[Gervase Markham [:gerv]]

Another cryptic timeless bug report :-) What exactly is the issue here? That
people can type anything in the attachment filename field? If so, the solution
is obviously a regexp-enforced whitelist of characters. Isn't it?

Gerv

Comment 4

[timeless]

the problem is that installed databases will have full paths because browsers do
that.

there's nothing wrong with people entering full paths *after* attaching a file.
but prepopulated databases where people's local paths were leaked (by n4, moz,
ie, ...) should not be exposed.

i should not have been able to find out that phil has a user 'pschwartau' on an
nt4 (or migrated) box and that he uses and has a c: drive.

The fix should be to sanitize all paths *once* during the migration for this new
feature which allows showing and changing file names.

Comment 5

[Myk Melez [:myk] [@mykmelez]]

At least in b.m.o's database there are no escaped spaces:

mysql> select filename from attachments where INSTR(filename, '\\ ') limit 10;
Empty set (2 min 9.94 sec)

mysql> select filename from attachments where INSTR(filename, ' ') limit 10;
+-------------------------------------------------------------------+
| filename                                                          |
+-------------------------------------------------------------------+
| ***\test accounts\account.txt |
| C:\My Documents\***                                    |

(anonymizing asterisks added by me)

Comment 6

[Myk Melez [:myk] [@mykmelez]]

attachment.cgi line 335 already sanitizes file names:

http://lxr.mozilla.org/mozilla/source/webtools/bugzilla/attachment.cgi#335

This doesn't look like a bug to me.

Comment 7

[Myk Melez [:myk] [@mykmelez]]

Ok, I now see how this is a bug--it's on the "edit attachment" page.  I've
masked the problem on my test install using the same code on the line I just
quoted, but we need a better solution.

Comment 8

[Bradley Baetz (:bbaetz)]

Why do we need a better solution? I realise that this doesn't handle stuff like
the mac's ':' separated stuff, but isn't this the browser's job?

joel; We can't use splitpath, because we don't know the os of the browser, which
is what is the issue here.

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Well, for one thing, if someone edits a new attachment with a full path filename
that's been munged for display, and then submits changes to that attachment,
they'll remove the path from the filename, and that change will show up in bug
history.

Second of all, we don't want to have to munge/unmunge file names every time we
display them, we should instead munge them once when we receive them if the
browser hasn't done its job for us.

Comment 10

[timeless]

i really think this can be solved in checksetup

i think attachment.cgi's create action prevents new attachments from leaking the
info, and if someone manually adds a full path later, that's ok.

so checksetup just needs to convert people as part of the 2.17 process.

Comment 11

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v2: does the job

This patch strips paths in filenames when attachments are created and removes
paths from the filenames of existing attachments.  I needed a way to
distinguish between installations with and without this fix, and fortunately
the filename column has a ridiculous type (mediumtext, or millions of
characters), so I convert it to varchar(100) and use the column type to
indicate whether or not existing filenames have had their paths removed yet.

Comment 12

[Bradley Baetz (:bbaetz)]

Comment on attachment 105530 [details] [diff] [review]
patch v2: does the job

-  my $filename = SqlQuote($cgi->param('data'));
+  $filename = SqlQuote($cgi->param('data'));

should be

$filename = SqlQuote($filename)

You need to change the tabel defintiona at the top of checksetup.

Don't do the UPDATE if the filenames are the same, to save time (although the
db may do that optimisation for you, I'm not sure.

You may want to only SELECT those entries containing \ or /.

Also, is NOT NULL correct? What if the browser doesnt' send the filename? Do we
storte the emtpy string in, the/

Comment 13

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v3: review fixes

>should be
>
>$filename = SqlQuote($filename)

Fixed.	I made a change here at the last minute after making sure it worked. 
Serves me right for not testing again.


>You need to change the tabel defintiona at the top of checksetup.

Done.


>Don't do the UPDATE if the filenames are the same, to save time (although the
>db may do that optimisation for you, I'm not sure.
>
>You may want to only SELECT those entries containing \ or /.

Seems like these are mutually exclusive.  I went for selecting only the entries
with a slash.


>Also, is NOT NULL correct? What if the browser doesnt' send the filename? Do
we
>storte the emtpy string in, the/

I don't know if it's correct, but that's the way the column is defined now, so
I didn't change it.

Comment 14

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v4: truncates correctly

This version truncates the filename at 100 characters to protect those
databases that wig out when they get too much data.

Comment 15

[Bradley Baetz (:bbaetz)]

Comment on attachment 105535 [details] [diff] [review]
patch v4: truncates correctly

Joy, another 2G table copy for bmo. (I don't suppose that you can combine these
somehow?)

I'm not sure if we want the status updates,since we don't know how many entries
there will be, because of the WHERE. I also don't think its worth trying to do
the / removal in sql

r=bbaetz, but please get another person to sanity test this.

Comment 16

[Gervase Markham [:gerv]]

Comment on attachment 105535 [details] [diff] [review]
patch v4: truncates correctly

>-    filename mediumtext not null,
>+    filename varchar(100) not null,

>+# 2002 November, [e-mail], bug 178841:
>+#
>+# Convert the "attachments.filename" column from a ridiculously large
>+# "mediumtext" to a much more sensible "tinytext" (255 characters). Also take 

This comment doesn't match the code above or below.

>+# the opportunity to remove paths from existing filenames, since they shouldn't
>+# be there for security.  Buggy browsers include them, and attachment.cgi 
>+# now takes them out, but old ones need converting.
>+#
>+# Note that this may not work on Mac file names and could mess up file names 
>+# with slashes in them, but them's the breaks.  We only use them as a hint 
>+# to users downloading attachments anyway, so it's not a big deal if we munge
>+# a file name incorrectly occasionally.

You probably don't need to duplicate this information here. As you say, it's
not a big issue.

Other than that, this looks very sane - but I have to go to work so I can't
test it. I'll try and do so tonight if no-one else has by then, but that's
cutting it a bit fine.

Gerv

Comment 17

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v5: comment fixes

I'll test this on a test copy of the b.m.o data, and I'll see if Dave can also
test it, if you haven't gotten to it yet Gerv.

Comment 18

[Gervase Markham [:gerv]]

Comment on attachment 105613 [details] [diff] [review]
patch v5: comment fixes

r=gerv. Seems to work for me. :-)

Gerv

Comment 19

[Dave Miller [:justdave]]

a= justdave

Comment 20

[Myk Melez [:myk] [@mykmelez]]

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.203; previous revision: 1.202
done
Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.29; previous revision: 1.28
done

Comment 21

[Myk Melez [:myk] [@mykmelez]]

Note: this was checked in with a newline added to the following line:
(per conversation with bbaetz on IRC)

+        print "Removing paths from filenames in attachments table... ";

So that it looks like this:

+        print "Removing paths from filenames in attachments table...\n";

Without this, Perl buffers the line and doesn't display it in time.

Comment 22

[Bradley Baetz (:bbaetz)]

Can we open this up? It was never in a shipped release, or even on bmo. Or maybe
we shoudl wait for 2.17.1

Comment 23

[Bradley Baetz (:bbaetz)]

Opening up fixed security bug.

This bug affected cvs updates from:

2002/09/21 16:57:07 to 2002/11/09 01:23:07 US/Pacific time

(+/- 15 minutes each side for the cvs mirroring)

where files were uploaded using browsers which leaked this information.