harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Mark Hindess" <mark.hind...@googlemail.com>
Subject [classlib] File.createNewFile(...)
Date Fri, 21 Sep 2007 07:35:05 GMT

The security section of the excellent Linux Weekly News (http://lwn.net)
this week has an article about "Exploiting symlinks and tmpfiles".
After reading it, I thought I'd take a look at the Harmony natives.  I
am beginning to wish I hadn't ...

It turns out it took less than 10 minutes to find a problem (but the
rest of the day to figure out how to fix it and the related issues).
The method in the subject is supposed to atomically create a file only
if it doesn't exists.  The current (unix) implementation in harmony [0]
does a non-atomic "stat then open".

This is wrong for two reasons.  First stat will report that the file
doesn't exist even if the file is an existing dangling symlink (e.g. one
create by a hacker to cause a new file to be created with the privileges
of the java process).  This should be an lstat call.

Second and more importantly, it should be implemented using a single
open call with the O_EXCL flag.  So I tried to do the harmony equivalent
of this with hyfile_open.  It is then necessary in the error case to
check the portlib last error to see if it was a "file exists" error
since the method returns false if the file exists and an exception on
other errors.

Unfortunately, this doesn't work because hyfile_open does a stat to
check that it isn't being asked to open a directory[1] - and it doesn't
set the portlib last error flag.  At this point I thought things were
getting quite involved but I didn't feel like modifying hyfile_open in
case anything was relying on this stat call.  So I decided to modify
hyfile_open to set the last error to report file exists with the message
"Is a directory"[2].

I begin to feel like I'm making progress so I run the luni tests.  It
turns out there are two failures.  The test cases call createNewFile
with "/tmp" and "/.." and both expect an IOException and now the code
just returns false for these calls.  This seemed incorrect since if the
file exists (and both of these do) then the method should not throw an
exception but return false.  So I test on the RI.  It turns out the
RI returns false for "/tmp" so this test case is invalid.  But rather
surprisingly the other test case, "/..", does throw an exception on the
RI.

At this point I was a bit confused and assumed that perhaps the
different behaviour was because I had write permission on "/tmp" but not
"/.." (which I assumed was the same as "/"). So I tried "/usr" instead
but got the same (returns false) behaviour.

It turns out that the behaviour is caused by the fact that the syscalls:

  open("/tmp", O_RDWR|O_CREAT|O_EXCL, 0666) and
  open("/..", O_RDWR|O_CREAT|O_EXCL, 0666)

return EEXIST and EISDIR respectively even though the former also
exists and the latter is also a directory.  (This behaviour is very
confusing[4].)

Since stat returns the same for both of these then it will be necessary
to remove the stat call from this code path in order to achieve the
correct behaviour.

It might be possible to leave the stat call (with a set last error call
to make this path less confusing to debug) but only for the readonly
case which might otherwise unintentionally pass the open test.  But
right now I'm running the tests with the stat call from hyfile_open
completely removed to see what breaks since it might just be easier to
get rid of it.

I have a patch for the unix now and I don't think it is as complicated
on windows but I'm just getting someone to check the patch before 
committing it.

Regards,
 Mark.

[0] Java_java_io_File_newFileImpl in
    modules/luni/src/main/native/luni/shared/file.c line 323

[1] It's probably not that useful but on unix you can use the file open
    syscall to open a directory provided you are using read only flags.

[2] Previously when this error path returned without setting the 
    portlib last error value if we tested the last error value we'd
    actually be getting the second last error value so this change
    will certainly make things a little less confusing.

[3] I'd love someone to explain why '/' (and any path ending with . or 
    ..) seems to be treated as a special case:

  open("/tmp", O_RDWR|O_CREAT|O_EXCL|O_LARGEFILE, 0666) = -1 EEXIST
  open("/..", O_RDWR|O_CREAT|O_EXCL|O_LARGEFILE, 0666) = -1 EISDIR
  open("/usr", O_RDWR|O_CREAT|O_EXCL|O_LARGEFILE, 0666) = -1 EEXIST
  open("/", O_RDWR|O_CREAT|O_EXCL|O_LARGEFILE, 0666) = -1 EISDIR
  open("/usr/..", O_RDWR|O_CREAT|O_EXCL|O_LARGEFILE, 0666) = -1 EISDIR
  open("/usr/.", O_RDWR|O_CREAT|O_EXCL|O_LARGEFILE, 0666) = -1 EISDIR

    I honestly can't believe that "/usr" and "/usr/." are treated 
    differently but they are.  Linux/2.6.22.6



Mime
View raw message