Re: [PHPLIB] Am I doing right? From: Kristian Koehntopp (kris <email protected>)
Date: 11/14/99

On Sat, Nov 13, 1999 at 04:48:11PM +0100, Björn Schotte wrote:
> page_open(array("sess" => "erinner_dich_session", "auth" => "Example_Auth"));
> if (!isset($zeit)) { $zeit=time(); }
> if (!isset($txt)) { $txt="bla"; }
> if (!isset($eintrag)) {
> $sess->register("zeit");
> $sess->register("txt");
> }

It would be more secure, if you'd set 'var $auto_init =
"setup.inc"' in your class errinner_dich_session. You'd then
have setup.inc in your php include directory, looking like this:

<?php
  global $zeit;
  $zeit = time();
  $sess->register("zeit");

  global $txt;
  $txt = "bla";
  $sess->register("txt");
?>

Not only is this more secure, but it is also easier to manager,
as you do not have to repeat this on every page which may have
been bookmarked by a user and is being used as an entry page in
your application.

You could also add a redirect in setup.inc, using a "Location:"
header. That would force the user to start a session on a
predefined start page and protect you from deep linking.

> ?>
> <html>
> <body bgcolor="#ffffff">
> <?
> if (!isset($eintrag)) {
> if (isset($tid)) {
> $q = new DB_Example;
> $q->query("select * from termine where tid=$tid and uname='".$auth->auth["uname"]."'");
> $q->next_record();
> $txt=$q->f("txt");
> }

The variables $eintrag and $tid are crossing the trust boundary
here. They are coming in from from the hostile internet and are
being used internally in statements that have the power to
change trusted data. No validation is done on either variable.

$eintrag is only being used as a trigger, checking only for the
existence of that variable. This is safe.

$tid is being used as part of an SQL statement. It is implicitly
assumed that $tid is an identifier, that is, that
ereg("^[0-9]+$", $tid) is true. This is never checked. In a
database sufficiently powerful, $tid could be a subquery, a
stored procedure call or any function call to a
database-internal function. The results of such a statement are
unpredictable and can have desastrous results, depending on the
database structure. You'd do good to check for this case and
deal with it, as early as possible in your program.

It would be even better not to export $tid into the browser. Why
don't you make it a session variable so that you do not need to
export it beyond the trust boundary and back?

> echo "Self-URL: ".$sess->self_url()."<br><br>\n";
> echo "<form action=\"".$sess->self_url()."\" method=\"post\">\n";
> echo "<input type=hidden name=\"eintrag\" value=\"yes\">\n";
> if (isset($tid)) {
> echo "<input type=hidden name=\"tid\" value=\"$tid\">\n";
> }
> echo "Zeit: <input type=text size=12 name=\"zeit\" value=\"".time()."\"><br>\n";
> echo "Text: <textarea name=\"txt\" cols=30 rows=10>$txt</textarea><br>\n";
> echo "<input type=submit value=\"Eintragen\"></form><br>\n";

You could drop the <input type=hidden name=eintrag> and use
<input type=submit value="Eintragen" name="eintrag"> instead.
This has the additional benefit of enabling you to use multiple
different actions for form such as

<input type=submit value="Eintragen" name="eintrag">
<input type=submit value="Löschen" name="delete">

> $sess->reimport_post_vars();

Reimporting variables is a bad idea just for the security reasons
given above. Instead insecure variables and trusted variables
should have different names and data should flow from insecure
variables to trusted variables only after validation.

In your code you are using SQL like

insert into termine values(0,$zeit,'".addslashes($auth->auth[uname])."','".addslashes($txt)."')";

This is a bad idea, because such SQL is neither invariant
against column order nor is it maintainable in the case of
adding columns. You'd do better specifying all column names.

Kristian

-
PHP3 Base Library Mailing List. Send messages to <phplib <email protected>>.
To unsubscribe, send "unsubscribe" to <phplib-request <email protected>> in
the body, not the subject, of your message.