On Sat, 2004-01-31 at 20:57, Sebastien LITAIZE wrote:
> -is checking for wget & fetch enough.
It's just an extra check, it should be secure without that check. The
exploit wich was used to hack one of my servers used wget, so I made an
extra check.
> -should we check other variables as well.
Yes, but the album and pic vars are most important. Some parts of your
patch are perfectly suitable for this.
> The route I took in order to improve my phpix version quickly is
> attached. Basically, I tried to check each value read from remote and
> validate it.
That's exactly what every script and program should do.
> The main trouble is that there are assumptions regarding
> file names and such, which is OK here because files are all named on
> similar schemes, straight from digicams, but would be a pain for many
> users.
That's indeed a problem.
There are two limitations added by my patch, dirs can't have names with
wget or fetch in it.
> And anyway, I like your idea of checking is album is a dir, and if
> album/pic is a file.
I also like it because it are save assumptions.
> I don't know if there are other values read, appart from mode, album,
> pic, dispsize, start, degree and rotate...
Scripts should use $_REQUEST[''] $_POST[''] and $_SERVER[''] for
external vars. If they did it it whould be much easier to keep track of
unsafe data.
> Anyway, if this is of any help, please feel free to reuse.
Parts of it should get in the debian version of phpix.
> BTW, do you think I should send this reply to the same lists you sent
> you initial mail?
Yes.
> 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>"
+ if (!((ereg('^(album|view|home)$', $mode))||($mode == '')))
+ die('bad mode');
Good check. Shouldn't cause any trouble.
+ 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.
+ 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');
Looks good, some testing won't hurt.
+ if (!((ereg('^(0|90|180|270)$', $degree))||($degree == '')))
+ die('bad degree');
Good.
+ if (!((ereg('^1$', $rotate))||($rotate == '')))
+ die('bad rotate');
Good.
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?
--
Daniel van Eeden <[EMAIL PROTECTED]> http://compukid.no-ip.org/
jabber: [EMAIL PROTECTED] aim: Compukid128 icq: 36952189
