267828 - nsLocalFileWin::IsExecutable needs to trim trailing dots

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This comes from investigation of bug 267122.

It seems that when saving a file in Windows, trailing dots are trimmed from the
filename.  Therefore a file saved as "foo.exe." will be saved as "foo.exe", and
an attempt to read a file "foo.bar..." will read the file "foo.bar".

This means that nsLocalFile::IsExecutable in nsLocalFileWin needs to be fixed to
trim trailing dots before comparison.

(Perhaps normalization needs to happen at an earlier stage?  Are there any other
functions in nsLocalFile that are security-sensitive?)

Do we need to trim any other characters as well?

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

note bug 246280

Comment 2

[chris hofmann]

sounds like this is fixed for seamonkey and just needed for firefox.

Comment 3

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

<roc> what I want to know is:
<roc> -- Does Windows strip trailing dots when you do a ShellExecute on a file?
<roc> -- Does Windows strip trailing dots when you create a file?
<roc> (i.e. via CreateFile API)

I put together a test app to find out:

  ShellExecute(GetDesktopWindow(), "open", "test.txt...", NULL,
               ".", SW_RESTORE);
This opened "test.txt".  I could not find a way to create a file with trailing
dots (I tried GUI apps, commandline, and cygwin).

  CreateFile("test2.txt...", FILE_ALL_ACCESS, FILE_SHARE_READ, NULL,
             CREATE_ALWAYS, 0, NULL);
This created a file called "test2.txt".

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

thanks Chris. Do you want to try making a fix for this bug?

Comment 5

[Doug Turner (:dougt)]

Attachment: Patch v.1

This patch does two things.  First is normalizing the path in IsExecutable(). 
Secondly, and probably more importantly, we call IsExecutable() in Launch(). 
This  will prevent us from executing processes that we have filtered out in
IsExecute.

Comment 6

[Doug Turner (:dougt)]

Attachment: test cases

dbaron, I am not sure if you had a particular test case that you wanted me to
try out.  attached are a few that I was playing with to ensure nothing got
broken.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 173992 [details] [diff] [review]
Patch v.1

It doesn't look like the normalize function performs the normalization needed
to fix this bug.

(Do we need to do this for all components of the path, or just file names?)

Comment 8

[Doug Turner (:dougt)]

Attachment: Patch v.2

this corrects normalization as you suggested.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Is Launch supposed to work on files that aren't executable (i.e., launch the
program that handles that type)?

Comment 10

[Doug Turner (:dougt)]

Attachment: Patch v.3

fix the bug, and nothing but the bug.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Launch() needs to work on any file type the OS knows how to launch ("equivalent
of doucle-clickin on the icon", like the documentation says).

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 173996 [details] [diff] [review]
Patch v.2

r- per comment 11

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 173998 [details] [diff] [review]
Patch v.3

So I did some quick tests and found that Windows seems to strip all trailing
dots and spaces off the last component of the path, but only a single trailing
dot off of preceding components.  This patch thus doesn't seem to do quite
enough normalization.  If you don't want to fix that here, could you please
file a followup bug on fixing that issue?  There may be other dangers in doing
too little normalization (there's certainly danger in doing too much).

(Also, is there a system function we could call to normalize the path?	Does
normalization differ across versions of Windows?)

Comment 14

[Doug Turner (:dougt)]

i do not see any danger in leaving a trailing dot in a non terminal component of
a file path.  I will check this in as is, and follow up with a new bug to review
and possibly clean up the windows normalization code.  I do remember a Win32
function called *FixUp* that normalized.  It might have been defined by windows
or maybe defined in another source base.

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 173998 [details] [diff] [review]
Patch v.3

a=dveditz for 1.7 and aviary1.0.1 branches

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 173998 [details] [diff] [review]
Patch v.3

a=dveditz for 1.8b too

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'm actually wondering now whether IsExecutable should really be changing the
object by calling Normalize.  We should probably be consistent across platforms
about this sort of thing.

Also, should we pull bug 187957 onto the branches?  Does it have security
implications?

Comment 18

[Doug Turner (:dougt)]

dbaron and I agreed that IsExecutable should not mutate the object.  The patch
that landed was slightly modifed -- we are not calling Normalize in
IsExecutable, but simply remove the trailing dots and spaces from the path
before we compare.

This patch landed on the trunk, 171, and avairy branches.

I created bug 281951 to track normalization cleanup, if any.

I am not sure if we need to back port normalize.  As I remember, it was added to
fixup problem when creating nsLocalFile's using bad paths from javascript.  

Comment 19

[Jay Patel [:jay]]

Doug:  I am failing the testcases you posted.  Not sure if I setup the
environment wrong or if this bug is not fixed.  I put notepad.exe in the right
location and failed the last 5 tests:

    success &= TestExecute("C:\\windows\\notepad.exe", true);
    success &= TestExecute("C:\\windows\\notepad.exe .", false);
    success &= TestExecute("C:\\windows\\notepad.exe . ", false);
    success &= TestExecute("C:\\windows\\notepad.exe.", false);
    success &= TestExecute("C:\\windows\\notepad.exe..", false);
    success &= TestExecute("C:\\windows\\notepad.exe...", false);

I could not create any files with trailing dots or spaces, so not sure if I need
those files to exist.  I'm guessing not, since the js code seems to handle all
that stuff.  What am I doing wrong?  And if nothing, should we reopen this bug?

I already reopened bug 275441 since it didn't look like the fix here worked for
those cases.  I could be wrong, so please let me know what I can do to retest. 
Thanks.

Comment 20

[Boris Zbarsky [:bzbarsky]]

>    success &= TestExecute("C:\\windows\\notepad.exe .", false);

isExecutable should be returning "true" for this case... so it looks like the
test is wrong?

Comment 21

[Asa Dotzler [:asa]]

reopening based on Jay's comment.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Er.. as far as I can tell, Jay's comment is based on incorrectly testing the fix. 

Comment 23

[Doug Turner (:dougt)]

bz, the test was written when I thought that we shouldn't say a file is
executable if it ended in trailing spaces or dots. 

Comment 24

[Doug Turner (:dougt)]

sorry jay -- I should have be more clear about the usage of this testcase. 

Comment 25

[Jay Patel [:jay]]

Doug:  So should all of those cases be using "true" as the param for
shouldExecute?  We are trying to trim everything so that windows sees the file
eventually as just "notepad.exe", right?  If that is the case, I already tested
with everything set to "true" and all the tests pass.  If I'm still way off, let
me know how to properly test this.  Thanks.  

I'm guessin you guys might have already tested this again, but I still want to
figure out how this works for my own sanity.  :-)  Feel free to mark this fixed
again if you have verified it yourselves.

Comment 26

[Daniel Veditz [:dveditz]]

Attachment: updated testcase

Updated testcase to pass correct param, and to eliminate the annoying multiple
security requests that encourage granting permanent god privileges to any
bugzilla attachment that comes along (which would be a really, really, bad
thing to do).

Comment 27

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #26)
> Updated testcase to pass correct param, and to eliminate the annoying multiple
> security requests 

of course with default settings, no security request at all appears, and the
testcase just fails to work

Comment 28

[Jay Patel [:jay]]

The new testcase worked fine for me and passed all the tests.

Comment 29

[Daniel Veditz [:dveditz]]

This one does appear fixed, it just didn't fix bug 275441 as expected.

Comment 30

[Daniel Veditz [:dveditz]]

This bug is fixed, comment 21 was a mistake.