[pmwiki-users] Bug: mkdirp crashes

[Joachim Durchholz]

Patrick R. Michaud wrote:

> On Sat, Apr 09, 2005 at 10:58:42PM +0200, Joachim Durchholz wrote:
> 
>>Hi all,
>>
>>there's a bug in mkdirp that will make it recurse endlessly trying to 
>>create directory '.', resulting in the error message:
>>[...]
>>Fix:
>>
>>In function "mkdirp", change line 365 to read
>>  if ($dir == '.' || file_exists($dir)) return;
>>(the check for '.' is new).
> 
> Hmm, that's bizarre.  The only way that file_exists('.') could be
> false is if the current process somehow doesn't have appropriate
> permissions to the current directory (in which case, how did we
> get to the directory in the first place?) . 
> 
> I played with directory permissions on my server and couldn't
> duplicate the '.' bug (linux).

It seems to be related to ownership issues, in connection with safe mode.

Here's what I did:

On that machine, PHP is running as an Apache module, with safe mode 
switched on.
PmWiki had been running successfully for a while.
I did an upgrade by wgetting the tarball and unpacking it; then I did a 
cp -a of the unpacked sources over the PmWiki installation.
What I had overlooked was that I was running as root at that time, so 
all the files were created with the wrong owner. I think this also 
clobbered the directory ownerships, so that safe mode prevented any 
writes even though the Unix filesystem would have permitted that.

> Still, that doesn't mean it doesn't exist for some environments, so
> I'll probably go ahead and add the check.  I should probably check
> for '..' while I'm at it.

I don't think that's needed. That '.' is the result of dirname()ing a 
directory name without embedded slashes, which will give you '.' (or, on 
really old PHPs before 4.3.0, and empty string).

>> I also noted a few minor bugs and nits to pick in mkdirp. I don't
>> know whether any of these are relevant, so I simply list all of
>> them.
>> 
>> 1) $safemode is always initialised, but not used unless an error
>> occurs.
> 
> Ok.
> 
>> 2) In case of problems, $perms is calculated, but not used unless
>> PHP is running in safe mode.
> 
> That is *really* a minor nit.  :-)  I think the code reads better without
> the braces on the if ($safemode).

Agreed on that.

> Also, when writing it, I didn't know if I would need the $perms value
> for the non-safemode text.

Consider moving the $perms calculation immediately before the line where 
$perms used. Should improve readability anyway. (Not that it's even 
remotely important.)

>>3) Directories are created with mode 0777 (rwxrwxrwx). This opens a 
>>small window of vulnerability until the permissions are fixed. 
> 
> No, directories are created with 0775 (rwxrwxr-x) permissions, because
> PmWiki sets the umask to 002 earlier in the program.

Ah, I didn't see that.

> Creating the directory with mode 000 doesn't buy any additional
> security.  Defaulting to 0755 or 0700 permissions might be useful
> (and fixperms can then adjust them),

Agreed then.

 > but I think I'll leave this to a wiki admin to control via a
> custom umask setting rather than hardcode one of these values into
> mkdirp().
> 
>     umask(022);   # create directories and files as 0755
>     umask(077);   # create directories and files as 0700
> 
> 
>> 4) I don't know why mkdirp tries to create '.'. Must be some
>> borderline behavior of dirname, mkdir, or fixperms.
> 
> It would have to be because mkdirp is being sent a strange directory 
> name, coming from a strange configuration variable setting.  PmWiki 
> doesn't expect '.' to show up as a directory (but shouldn't care if
> it does).

That riddle is now solved: it's the result of

   dirname ('wiki.d')

> Out of curiosity, what version of PHP was running the system where
> you found this error?

PHP 4.3.4 on SuSE 9.2.

>> 5) If mkdirp fails due to owner mismatch problems, it shouldn't
>> output a message telling the administrator that he can fix the
>> problem with a chmod. No message would be better, pointing the
>> admin to chown (resp. "rm -rf" followed by "su" or "sudo") would be
>> best.
> 
> Not everyone has su or sudo, or even shell access, or even Unix.  The
>  current system works for the vast majority of cases--I don't want to
>  exclude 50% of potential wiki admins in order to "fix" an error 
> message for 2%.  (These numbers are conservative.)
Shell access is irrelevant - the error message that comes out already 
assumes shell access.
I just suggest suppressing that error message in those cases where it 
gives misleading advice, that's all.

Regards,
Jo