119071 - Can not ftp files from / on Cobalt web server

Status: RESOLVED FIXED

(Core Graveyard :: Networking: FTP, defect)

Description

[David McCormack]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:0.9.7+)
Gecko/20020108
BuildID:    2002010803


My web site is located on a Cobalt rackmounted server.

To use ftp to access the log files I have to use the following instructions;

  ****************** Site Administrator FTP instructions ******************
 
    This session begins in your user home directory at /users/dmccormack
    Your personal web directory is /users/dmccormack/web, accessible at the URL
    http:///~dmccormack/
 
    As a site administrator, you can access the site home directory at /
    The site web directory is /web, accessible at the URL
    http:///
 
    To upload to your personal web directory, type:
    "cd web", then "put ", where  is the full file name of the
    document on your local computer.  
 
    To upload to the site web directory, type:
    "cd /web", then "put " where  is the full file name of the 
    document on your local computer.
 
  *************************************************************************

Mozilla does not allow me to move higher than the initial starting folder though.

Reproducible: Always


Steps to Reproduce:

Entering the address ftp://dmccormack:<password>@ftp.zamang.co.uk/../../logs in
the address text box.


Actual Results:

I get the error message '/users/dmccormack/logs: No such file or directory'.


Expected Results:

I would have expected this to login (once I've entered the correct password), to
have the current folder as /users/dmccormack then cd ../ twice up to /. It
should then list the main folders in the root folder.

It appears that Mozilla is ignoring the ../../ and preventing me from using
Mozilla to access any of the admistrator files.

I've tried this address in MSIE 6 and it is accepted. Though it might be broken
behaviour on IE's part.

Comment 1

[Bradley Baetz (:bbaetz)]

andreas - is this related to yuor fix for bug 84242?

Comment 2

[Andreas Otte]

Hm ... I don't think so. It's more a problem that we normalize the url while
parsing it and part of that is to get rid of ../.. and similar stuff. One
special thing: We don't go above the starting point, anything doing that will
get dropped and that seems to happen here. What we do is okay for http, but
maybe wrong for ftp.

David: What url is displayed in the adress bar when you start with 
ftp://dmccormack:<password>@ftp.zamang.co.uk/../../logs after the error message ?

Comment 3

[David McCormack]

Andreas:
After the error message is displayed the address stays the same as 
entered, 'ftp://dmccormack:<password>@ftp.zamang.co.uk/../../logs'.

Comment 4

[Andreas Otte]

Okay, I was able to reproduce it myself. The url in the adressbar seems not to
be updated. I'm now pretty sure the problem is as I described above. The problem
really seems to be that we normalize absolute as well as relative urls. For
relative urls RFC 2396 says:

         If the resulting buffer string still begins with one or more

         complete path segments of "..", then the reference is

         considered to be in error.  Implementations may handle this

         error by retaining these components in the resolved path (i.e.,

         treating them as part of the final URI), by removing them from

         the resolved path (i.e., discarding relative levels above the

         root), or by avoiding traversal of the reference.


and also:

    Parsers must be careful in handling the case where there are more
    relative path ".." segments than there are hierarchical levels in the
    base URI's path.  Note that the ".." syntax cannot be used to change
    the authority component of a URI.

    ...

    In practice, some implementations strip leading relative symbolic
    elements (".", "..") after applying a relative URI calculation, based
    on the theory that compensating for obvious author errors is better
    than allowing the request to fail.  Thus, the above two references
    will be interpreted as "http://a/g" by some implementations.

Mozilla discards relative levels above the root. However we may be in error to
also subject absolute URLs to this kind of normalisation. I could not find
anything in RFC 2396 that says that absolute urls are also subject to this part
of the algorithm to resolve relative urls.

ccing darin on this.

Comment 5

[Darin Fisher]

David: as a quick workaround you can probably set up a soft link to / in your
home dir... that way you can still use mozilla to access files outside your home
directory.

andreas: seems to me that "ftp://user:[e-mail]/../../bar" should be valid. 
only the FTP server knows otherwise.  our coalescing logic needs to be smarter
to not collapse ../'s when it would change the resource being referenced.  a
change like this would probably be ok for http, though i can't imagine it being
useful in practice.  another thing is to consider security issues... are there
any we need to consider here?  nothing comes to mind, but we should be careful.

Comment 6

[Andreas Otte]

A security problem with http might arise when the webserver would allow ../../
GETs to move to a level above the root of the webserver. But that would be a bug
with the webserver ... 

Comment 7

[Andreas Otte]

Attachment: patch to fix the problem

This patch to CoalesceDirs does not drop to much /.. instead it leaves them in
the path. Darin could you take a look? This works now with the travesal
variable which counts the hierachies.

Comment 8

[Darin Fisher]

Comment on attachment 64667 [details] [diff] [review]
patch to fix the problem

sr=darin

Comment 9

[Andreas Otte]

Bradley, want to give a r=, already have sr= from darin

Comment 10

[Bradley Baetz (:bbaetz)]

Comment on attachment 64667 [details] [diff] [review]
patch to fix the problem

This looks fine. PRUint32 instead of int, and use --traversal intsead of
traversal--, (and ++foo intead of foo += 1).

r=bbaetz if you've tested.

Comment 11

[Andreas Otte]

incorporated bbaetz suggestions, tested again with standard urltest tests and
some special cases. fix checked in. 

Comment 12

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

I think the fix for this bug broke the relative links (the dates) on the right
side of http://tinderbox.mozilla.org/showbuilds.cgi?tree=SeaMonkey .  I'd be
surprised if it didn't break other sites as well.  Could it be limited to FTP? 
What do the URI and URL RFCs say?

Comment 13

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

...or, now that I read the bug more carefully, perhaps it could be limited to
absolute URLs only?

Comment 14

[Darin Fisher]

ok, i backed out the patch.  we need to figure out how relative paths are
getting into CoallesceDirs and determine if they either need to be stopped from
doing so, or if CoallesceDirs needs to also know how to deal with them :-(

Comment 15

[Andreas Otte]

Hmmm ... sorry I had to sleep for three hours ... it should be okay for both
absolute and relative urls. The problems seems to be if the absolute url does
not end with an / but the last part of the path is *meant* as part of the
directory (but parsed as filname instead). Did not catch this case with urltest
and also not with the other tests I did and I couldn't because it's correct what
happens. My guess is we had this problem in the old version of CoalesceDirs too
but dropping the ../ saved it. I could make tinderbox a case for tech
evangelism. ;-)

Comment 16

[Andreas Otte]

Further looking into this ... it's not exactly what I thought first, the html
generated by the tinderbox script seems to be bogus, please correct me if I'm
wrong here. The base-url is:

http://tinderbox.mozilla.org/showbuilds.cgi?tree=SeaMonkey

The directory part is /. The generated html contains then:

<tt>
<a HREF='showlog.cgi?log=SeaMonkey/1011167160.20268.gz' onclick="return
log(event,0,'1011167160.20268.gz');">L</a>
<a
href=../bonsai/cvsquery.cgi?module=MozillaTinderboxAll&date=explicit&mindate=1011165060&maxdate=1011167159>C</a></tt>

or

<td> <a
href='../registry/who.cgi?email=law%25netscape.com&d=MozillaTinderboxAll|HEAD|/cvsroot|1011162660|1011162840'
onclick="return who(event);">law</a>
</td>


Both hrefs are not correct since they reference stuff above the root level of
the http server. Simply dropping the /.. in CoalesceDirs fixed the html in this
page only by accident.

If nobody objects I will file a tech evangelism bug against mozilla.org shortly,
I always wanted to do that ... ;-)

Comment 17

[Bradley Baetz (:bbaetz)]

showbuilds.cgi?tree=SeaMonkey is the same as
http://tinderbox.mozilla.org/Seamonkey/, I think, or something similar.

Doesn't the HTTP spec say something about these forms of urls?

Comment 18

[Andreas Otte]

That looks very much like the first URL I saw at home:

http://tinderbox.mozilla.org/Seamonkey

Note, it is without the / at the end, so my first comment after the backout
would apply with the same result: wrong HTML.

See comment 4 for info what the spec says. We simply changed from dropping to
leaving. 

Comment 19

[Andreas Otte]

The problem seems to be that there are different access paths to tinderbox
information:

http://tinderbox.mozilla.org/showbuilds.cgi?tree=SeaMonkey 

and

http://tinderbox.mozilla.org/SeaMonkey/

The second one would work fine with the patch since it puts the base url at the
right level. The first one seems to be the problem, the generated relative urls
do not fit the base url. Previously we silently ignored this error ...

Comment 20

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

But wouldn't you expect that some other sites have the same problem, since it
seems that all browsers handle such URLs the same way?

Comment 21

[Andreas Otte]

I don't expect it to see this in very much cases, since this clearly is a very
visible error, base url and relative urls simply do not fit together. All this
sites are cases for tech evangelism. I would like to do the following:

1. Get tinderbox fixed
2. Check in the patch again shortly after the 0.9.8 branch
3. See which (if any!) sites else have problems. What we do is clearly within
the bounderies of RFC 2396:

  If the resulting buffer string still begins with one or more complete path    
  segments of "..", then the reference is considered to be in error.   
  Implementations may handle this error by retaining these components in the 
  resolved path (i.e., treating them as part of the final URI), by removing them 
  from the resolved path (i.e., discarding relative levels above the root), or by 
  avoiding traversal of the reference.

Comment 22

[Andreas Otte]

I looked again and both versions are off by one level ... time to file another bug.

Comment 23

[Andreas Otte]

As a quick solution we could add CoalesceDirsAbs to nsURLHelper which works like
the CoalesceDirs in the patch. We would call that version while normalizing
absoulte urls and call the original CoalesceDirs while resolving relative urls.

Comment 24

[Andreas Otte]

Attachment: another patch as described above, working around the wrong urls in tinderbox

Comment 25

[Darin Fisher]

i agree... this sounds like a good plan to me.  i do wish the code could be
shared a bit more, but perhaps that would just result in something less efficient??

andreas: your comment is that this might be a good quick solution... what do you
have in mind long term?

sr=darin

Comment 26

[Andreas Otte]

The right solution in my opinion is of course the first patch. There is nothing
wrong with it, it just found a big bug in the pages generated by tinderbox,
because we are no longer so forgiving about ill formed relative urls. 

The second patch is only forgiving when doing resolutions of relative urls.

My intention of course is to get in the first patch *after* tinderbox is fixed.  

Comment 27

[Andreas Otte]

bbaetz, want to r= again?

Comment 28

[Andreas Otte]

Moving to 0.9.9

Comment 29

[Darin Fisher]

andreas: actually, if tinderbox is/was abusing this then i suspect other web
sites probably do as well.  so, i'm not sure it is worth it to try to land the
original patch.  what is wrong with the latest patch?

Comment 30

[Andreas Otte]

There is nothing wrong with it. I just think it is doing uri-fixup (by accident)
that it should not do. Embedded links in a html page (as opposed to something
the user types into the urlbar) should follow a certain minimal standard, and
such (relative) urls are simply wrong. Doing this kind of "fixup" violates in my
opinion all that mozilla stands for.

Comment 31

[Andreas Otte]

Doug, want to r= the second patch?

Comment 32

[Bradley Baetz (:bbaetz)]

well, try this argument:

~$ cd /home
/home$ cd /
/$ cd ../../../../home
/home$ cd /../../../home
/home$ 

The filesystem does it, so why shouldn't uris?

Anyway, the second patch is OK, I think. I'm a bit concerned about the #ifdef
XP_WIN stuff, though - does that handle all cases? I don't know anything about
windows charset handling.

Comment 33

[Andreas Otte]

I did not change the #ifdef XP_WIN stuff. So it should be okay if it is okay in
the old CoalesceDirs.

RFC 2396 clearly states that such an URI is wrong. Allowing it (and silently
modifying it to something that *may be* correct) would just promote lazyness. I
simply don't like it.

For now I will go with the second patch and if it is checked in I will close
this bug, but I will open a new one on the issue, ask around in the netlib
newsgroup, ....

Comment 34

[Doug Turner (:dougt)]

let bradley review the second since he reviewed the first.

Comment 35

[Bradley Baetz (:bbaetz)]

Comment on attachment 65454 [details] [diff] [review]
another patch as described above, working around the wrong urls in tinderbox

OK. r=bbaetz

Comment 36

[Andreas Otte]

fix checked in