PHPの質問。動作させようとしている自サイトの検索機能があるのですが、どうもセキュリティに不安を感じています。どなたかチェックしていただけないでしょうか？

気がついた点を2つほど挙げます。

1. コマンドラインのidに文字列、正の整数以外の値、定義域を超えた値を代入されるとエラーが起こります。そのエラーにどう対処するかで、セキュリティレベルが変わってきます。
2. formのactionで$SCRIPT_NAMEを参照させていますが、なぜaタグでは$PHP_SELFを参照しているのでしょうか。$SCRIPT_NAMEに統一した方がセキュリティリスクは低減されます。

＞print "ディレクトリを開けませんでした\n";

エラーの内容がわかるのはまずいかと。

はじめまして。

1.の回答者様のいうように、入力値のチェックがもうちょっとあった方がよいかなぁという点と

PHP_SELFはXSSの脆弱性のために使わない方が良いという点がきになります。（でも参考書には多いですねPHP_SELF）

ただ、それよりも$PHP_SELFと書かれているのでregister_globalsがONになっているのでしょうか？

デメリット（リスク）を承知の上ならばもちろん問題はないと思いますが、そうでなければ$_SERVER['SCRIPT_NAME']など

スーパーグローバル変数をお使いになった方が良いと思います。

＞$filedir='/image';

推測されやすい文字列は危険です。

用意されている関数（php5で軽くテスト済みphp4には無い関数を使っています）を使ってコードを短くすることもバグ対策／セキュリティ対策に有効です

・ファイル一覧処理を簡略化

・５つずつ配列に分けていた部分を省略してメモリ節約／処理時間節約

・３項演算子で行数節約（３項演算子をネストしたりするとコードが見辛くなるので無理して短縮しないように注意）

・id指定ミスの自動修正

おまけ

・管理者用エラー番号を出力させることによって、管理者のみエラー箇所が判る仕組み追加

・１ページあたりの表示数指定を追加

$message = 'エラーが発生しました。時間をおいて再度お試しください。管理者用エラー番号：';
$page = 5;

$filedir = '/home/ユーザー名/public_html/'; #URLではなくサーバ上の位置指定が必要。これは一例
$pattern = '/key/i';
$dirlist = @scandir($filedir) or die($message . '1');
$fl_array = @array_values(preg_grep($pattern, $dirlist)) or die($message . '2');

$p = (isset($_GET['id']) ? min(max(0, $_GET['id'] - 1), floor((count($fl_array) - 1) / $page)) : 0);

for($i=0; $i < $page; $i++){
    echo (isset($fl_array[$page * $p + $i]) ? '<img src="/image/' . $fl_array[$page * $p + $i] . '">' : '');
}

function pager($id,$fl_count){
以下省略

@ はphpからの直接的なエラーメッセージを抑制するためのものです。最初から出ないサーバもありますのでワザとエラーが出るようにして@ありと@なしでエラーメッセージに変化があるかどうかを確認してみてください。

３項演算子部分は下記と同じ意味になっています。function pagerの中でも同様に使えばスッキリするかもしれません。

if (isset($_GET['id'])) {
    $p = min(max(0, $_GET['id'] - 1), floor((count($fl_array) - 1) / $page));
} else {
    $p = 0;
}

min(max(0, $_GET['id'] - 1), floor((count($fl_array) - 1) / $page)) は下記の要領で処理されます。

１．$_GET['id'] - 1 指示されたページの修正
２．上記で指示されたページがゼロ以下にならないよう調整(max)
３．floor((count($fl_array) - 1) / $page))　ファイル数からページ数計算
４．現実よりページ数が大きくなりすぎないよう調整（min）

調整不要ならば下記で以前通りですが「文字列の種類、長さなどを確認することでセキュリティを強化しました」とのことなので適宜対応してください。

$p = (isset($_GET['id']) ? $_GET['id'] - 1 : 0);