162588 - Some non-tier1 platforms may lack file truncation...

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect, P1)

Description

[Darin Fisher]

from the security review notes:

  "Some non-tier1 platforms have the problem that if we cache a file, then go
   see the site again and the file has become smaller, we don't erase the 
   difference. This could result in a situation (example) where I go to
   ecommerce site, view item on sale where default amount to buy is 10, come
   back a week later, page smaller (no longer possible to state how many to
   buy), I buy item, but accidentally by 10 items because the old page
   contained amount information."

we should either make adding PR_Truncate higher priority or simply add a
preprocessor #error message to prevent platforms which lack support for file
truncation from building.  this would help maintainers of the various "broken"
ports.

marking security sensitive... this is probably just dependent on some other bug(s).

Comment 1

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

This seems minor but we might as well fix it. CCing seawood for help. We should
probably take the path of least resistance and refuse to build if we don't have
a usable truncate().

Comment 2

[hacker formerly known as [e-mail]]

Is the problem that truncate() passes but doesn't actually truncate the file or
that truncate() always returns an error condition?  Right now, we appear to use
truncate() without any checks so platforms without any truncate() support would
fail anyway.  And do we have a workaround for those platforms with a broken
truncate()?

Comment 3

[Darin Fisher]

seawood: we call ftruncate for XP_UNIX builds.  any non-XP_UNIX builds have
special code to call ftruncate.  if some non-XP_UNIX build is not covered by our
#ifdef's then the files will not be truncated.  no function call, no truncation.

we need to make sure that for all platforms, there is a call to truncate the
file.  what about XP_BEOS?  i'm guessing it picks up the XP_UNIX #ifdef?  what
about other platforms?  thx!

Comment 4

[hacker formerly known as [e-mail]]

Oh.  I was looking at the truncate call at
http://lxr.mozilla.org/seamonkey/source/xpcom/io/nsFileSpecUnix.cpp#364 .  Since
it's the system's truncate, it should have the same behavior as ftruncate (I
think).  Assuming that truncate/ftruncate work correctly if they exist, we can
just add this one liner to configure.in to mandate these functions:

AC_CHECK_FUNCS(truncate ftruncate,,AC_MSG_ERROR([Your operating system does not
support truncate and/or ftruncate. Exiting.]))

BeOS is an anomaly.  It's not a unix variant but it contains a number of POSIX
compliant APIs which are usually associated with unix (eg, it does have
ftruncate & truncate).  BeOS does not set XP_UNIX; it uses the separate XP_BEOS
define.  All of the platforms, expect win32 (which doesn't do feature tests) &
mac classic, should be covered by adding the function check.

Comment 5

[Darin Fisher]

then BEOS definitely has this bug.

Comment 6

[gordon]

...but it sounds like it should be easy to fix.  Once I add the AC_CHECK_FUNCS
line, I should be able to add '| XP_BEOS' to the XP_UNIX implementation.  Right?

Comment 7

[hacker formerly known as [e-mail]]

Once you add the configure test, you should replace the XP_UNIX ifdef with a
HAVE_FTRUNCATE ifdef.  For the non-unix platforms (namely win32 & OS/2), should
you be using ftruncate, if it exists, or keep the existing truncate functions?

At this point, it only really sounds like BeOS was the only OS that isn't being
covered by the existing ifdefs so adding the || XP_BEOS would be the quick fix
to avoid adding the configure test.

Comment 8

[gordon]

What are the issues with adding a configure test?  It seems like that would be
the correct way to go.  Does it kick off a clobber build?

Comment 9

[hacker formerly known as [e-mail]]

There are no real issues with the configure test except that it may fail for
some non-unix platforms (namely OS/2) which you already cover with the other
ifdefs.   Adding the defines will cause the entire world to rebuild once (but
that's not really a problem).

Comment 10

[Mitchell Stoltz (not reading bugmail)]

Sounds like we've got a fix. Gordon, is it ready to be posted here and reviewed?

Comment 11

[hacker formerly known as [e-mail]]

Attachment: Quick fix

Comment 12

[Mitchell Stoltz (not reading bugmail)]

Let's get this reviewed and checked in for 1.3a.

Comment 13

[Darin Fisher]

Comment on attachment 105314 [details] [diff] [review]
Quick fix

sr=darin

Comment 14

[gordon]

Comment on attachment 105314 [details] [diff] [review]
Quick fix

r=gordon

Comment 15

[hacker formerly known as [e-mail]]

The quick fix has been checked in.

Comment 16

[gordon]

Okay, marking this fixed.  We have another bug to cover moving Truncate() into
NSPR. We can rework the config testing then.  See bug 88341.

Comment 17

[Mitchell Stoltz (not reading bugmail)]

adding [e-mail] to CC list

Comment 18

[Daniel Veditz [:dveditz]]

Bugs published on the Known-vulnerabilities page long ago, removing confidential
flag.