Re: phpix bug 229794 security patch

[Sebastien LITAIZE <slitaize@free.fr>]

Daniel wrote, answering an email I sent him:
> On Sat, 2004-01-31 at 20:57, Sebastien LITAIZE wrote:
> > BTW, do you think I should send this reply to the same lists you sent
> > you initial mail?
> Yes.

Well it's useless now, doesn't matter.

> > PS: I'm no expert either in php or in diff... thus if the file is not
> > useable, please tell me so.
> It looks a little strange to me, but it's very usable. Personaly I
> always use "diff -u <orig_file> <new_file>"

Oups... I think I did a -c. Sorry about that (could do a -u but only
monday). Anyway, file attached to this mail again, sorry Daniel you
already got it, but others didn't.

> + if (!((ereg('^[A-Z][a-zA-Z0-9-]*$', $album))||($album == '')))
> +       die('bad album');
> +
> + if (!((ereg('^[a-zA-Z][a-zA-Z0-9_-]*(\.[a-z]+)?\.jpg$', $pic))||($pic
> == '')))
> +       die('bad pic');
> This won't work for everybody. Should be optional.

Indeed. But I had to write something "to do the job" asap... Maybe there
is a function or a regex already written that would help to positively
check that the file / dir name is valid? Or maybe just the "is_file"
test you did is enough? What's the likelyhood of some type encoding of
evil chars to slip through?

> + if (!((ereg('^(Original|[1-9][0-9]{2,3})$', $dispsize))||($dispsize ==
> '')))
> +       die('bad size');

This makes allowed sizes 100-9999, but as while testing I discovered
that phpix does not check if the requested size is part of the array
setup in config.inc, so I also added a check further on ("in_array") to
solve this.

> + if (!((ereg('^(0|[1-9][0-9]{0,3})$', $start))||($start == '')))
> +       die('bad start');
> Looks good, some testing won't hurt.

Would cause trouble with albums with more than 9999 pics, but it's easy
enough to increase if needed.

> I don't like the die() cals which are made by this patch and by my
> patch. Anybody a nice solution for this? (
> Maybe a header("Location: security_error.php") kind of thing?

Or maybe some http error (4xx or 5xx I don't know)? Might be usefull
while parsing logs.

Regards,

Sebastien

*** index.php.old	Thu Jan 29 16:33:12 2004
--- index.php.new	Thu Jan 29 16:38:51 2004
***************
*** 1,4 ****
--- 1,25 ----
  <?
+ if (!((ereg('^(album|view|home)$', $mode))||($mode == '')))
+ 	die('bad mode');
+ 
+ if (!((ereg('^[A-Z][a-zA-Z0-9-]*$', $album))||($album == '')))
+ 	die('bad album');
+ 
+ if (!((ereg('^[a-zA-Z][a-zA-Z0-9_-]*(\.[a-z]+)?\.jpg$', $pic))||($pic == '')))
+ 	die('bad pic');
+ 
+ if (!((ereg('^(Original|[1-9][0-9]{2,3})$', $dispsize))||($dispsize == '')))
+ 	die('bad size');
+ 
+ if (!((ereg('^(0|[1-9][0-9]{0,3})$', $start))||($start == '')))
+ 	die('bad start');
+ 
+ if (!((ereg('^(0|90|180|270)$', $degree))||($degree == '')))
+ 	die('bad degree');
+ 
+ if (!((ereg('^1$', $rotate))||($rotate == '')))
+ 	die('bad rotate');
+ 
  /* read system wide defaults */
  include("/etc/phpix/config.inc");
  
***************
*** 36,41 ****
--- 57,66 ----
  ($imgbase == '/') and $imgbase = '';
  
  if ($dispsize == '') $dispsize = $default_size;
+ 
+ if (!in_array($dispsize, $viewsizes))
+ 	die('undefined size');
+ 
  if ($start == '') $start = 0;
  if (preg_match("/\.\./",$pic) or preg_match("/\.\./",$album)) {
    print "Please refrain from trying to access unauthorized files in this manner.<br>";