回答編集履歴

追記

2016/06/20 02:20

投稿

mpyw

スコア5223

test CHANGED Viewed

@@ -2,332 +2,280 @@
+原因が判明しましたので，内容を変更します．ttyp03さんのご指摘通りです．
+ただ，依然としてこの部分はPHPの文字列として直接変数展開せずに，プリペアドステートメントのプレースホルダ「?」に当てはめるように書くべきなので，セキュリティ上まずい書き方であることには変わりません．
+## 修正すべき脆弱性
+- ユーザから入力された変数をそのままSQL文の中に展開しているので，**SQLインジェクション攻撃**に対する致命的な脆弱性がある．この脆弱性は1つもあってはいけないレベルで危険．そもそもPDOの使い方を間違っているので，正しい用法を覚える．
+- `htmlspecialchars`を通さずにHTML内にテキストデータを表示しようとしている場所がある．**XSS攻撃**に対する脆弱性になり得るので，常に通すように心がけるべき．
+## 誤作動やエラーの防止
+- 変数未定義のエラーを防ぐために，`$_POST`の存在確認を`isset`で行うべき．欲を言えば更に`is_string`で文字列かどうかの確認を行うべき．これらをまとめて行ってくれる，`filter_input`の利用が推奨される．
+- `SET NAMES utf8`はデータベース側の文字セットしか変更できないため，基本的には,PDOドライバ側も同時に変更できる，DSNでの`charset=utf8`指定を利用すべき．
+- エラーモードがデフォルトの`PDO::ERRMODE_SILENT`のままなので，`->prepare()`のコールに失敗した場合にPDOExceptionが発生せず，単にfalseが返されるだけになってしまっている．エラーモードを`PDO::ERRMODE_EXCEPTION`に変更しない限りは，いちいち返り値をチェックする必要がある．
-日付にどんな値が格納されているのか，また検索時にどんな値を送ったのか，不具合を再現できる具体値を出してください．
+- LIKE検索のためのエスケープを行っていないため，`%` `_` `\` といった特殊文字を含むキーワードを正しく取り扱えない．
+- SELECTの結果に対して`->rowCount()`は使えず，常にゼロになってしまうため，結果を配列として取得したあと配列に対して`count()`を実行する．
-また，日付に関する条件分岐にも違和感を覚えました．
+## 読みやすいコードにするためには必要
+- HTMLの表示内容はほとんど重複するにも関わらず，絞り込み検索を行うためだけにPHPファイルを複製している．このような場合は「絞り込み条件が送信されてきたか」で判定し，HTML表示部分は共通化するだけで十分．
+- PHPのテンプレートエンジンとしての特性を無視して，`print`でHTMLタグ全体を出力している部分がある．基本的には変数に依存する部分のみを `<?= ~ ?>` 記法で出力するだけで十分．
+- ロジックとHTMLを可能な限り分離すべき．**「あらかじめ必要な変数を作っておき，すべての変数が用意できたらHTMLを書き始める」**というやり方を徹底する．`<body>`まで書いてからデータベースに接続して…，というのは典型的にダメな書き方．また，HTMLを途中まで書いておいて最後まで書き終わる前に`die`で強制終了するのNG．
+## 改善済みのコード
-```php
+```html
-$stmt =$dbh->prepare($sql);
+<?php
-if ($vid != "") {
-    $sql .= " and id = $vid ";
+// HTML中にテキストを埋め込むときに利用する関数
+function h($str)
+{
+    return htmlspecialchars($str, ENT_QUOTES, 'UTF-8');
 }
+// $_POST['vid'], $_POST['nm'], $_POST['ndt'] を安全に文字列として受け取る
+// もし存在していない場合，空文字列になる
+$vid = (string)filter_input(INPUT_POST, 'vid');
+$nm = (string)filter_input(INPUT_POST, 'nm');
+$ndt = (string)filter_input(INPUT_POST, 'ndt');
+try {
+    // localhostのkadaiデータベースに文字コードutf8で接続
+    // SQL実行時のエラーもPDOExceptionに変換してスローさせる
+    $pdo = new PDO('mysql:dbname=kadai;host=localhost;charset=utf8', 'root', '', [
+        PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION,
+    ]);
+    // 共通のSQL文と空のパラメータ用配列を準備する
+    $sql = '
+        SELECT id, name, address, tel1, tel2, email, ndate, remarks
+        FROM mst_member
+        WHERE 1 = 1
+    ';
+    $params = [];
+    // $vid が入力されていたら，AND条件としてプレースホルダ「?」を追加し．
+    // そこに文字列として当てはめるための値を $params に追加する
+    if ($vid !== '') {
+        $sql .= ' AND id = ? ';
+        $params[] = $vid;
+    }
+    // $nm が入力されていたら，AND条件としてプレースホルダ「?」を追加し．
+    // そこに文字列として当てはめるための値を $params に追加する
+    // 名前に関してはLIKE検索を行うので，LIKE検索で使われる特殊文字を
+    // addcslashes関数を使ってエスケープしたあと，両端を%で囲んだ値を使用する
-if ($nm != "") {
+    if ($nm !== '') {
-    $sql .= " and name like '%$nm%' ";
+        $sql .= ' AND name like ? ';
+        $params[] = '%' . addcslashes($nm, '\_%') . '%';
+    }
+    // $hdt が入力されていたら，AND条件としてプレースホルダ「?」を追加し．
+    // そこに文字列として当てはめるための値を $params に追加する
+    if ($ndt !== '') {
+        $sql .= ' AND ndate >= ? ';
+        $params[] = $ndt;
+    }
+    // SQLを実行し，すべての結果を2次元の配列として一気に取得
+    $stmt = $pdo->prepare($sql);
+    $stmt->execute($params);
+    $rows = $stmt->fetchAll(PDO::FETCH_ASSOC);
+} catch (PDOException $e) {
+    // もしどこかでエラーが発生した場合，「500 Internal Server Error」として
+    // テキスト形式でエラーメッセージを表示して終了する
+    header('Content-Type: text/plain; charset=UTF-8', true, 500);
+    echo "ただいま障害により大変ご迷惑をお掛けしております。\n";
+    echo "Error: {$e->getMessage()}\n";
+    exit;
 }
+// エラーが起こらなければHTMLとして表示を開始する
+header('Content-Type: text/html; charset=UTF-8');
+?>
+<!DOCTYPE html>
+<title>名簿一覧</title>
+<h1>名簿一覧</h1>
+<form method="post" action="">
+    ID(半角) <input type="text" name="vid" value="<?=h($vid)?>"><br>
+    氏名 <input type="text" name="nm" value="<?=h($nm)?>"><br>
+    入会日 <input type="date" name="ndt" value="<?=($ndt)?>"><br>
+    <input type="submit" value="検索">
+</form>
+<?php if ($_SERVER['REQUEST_METHOD'] === 'POST'): /* POSTされたときだけ結果件数を示す */ ?>
+<p>検索結果は<?=h(count($rows))?>件です</p>
+<?php endif; ?>
-if ($ndt != "") {
+<table border="1">
-    $sql .= " and ndate >= $ndt ";
-} else {
-    $post[]="";
+    <tr>
-}
-$stmt = $dbh->prepare($sql);
+        <th>ID</th>
-$data[] = $vid;
-$data[] = $nm;
-$data[] = $ndt;
+        <th>名前</th>
+        <th>住所</th>
+        <th>固定電話番号</th>
+        <th>携帯電話番号</th>
+        <th>Eメールアドレス</th>
-$stmt->execute($data);
+        <th>入会日</th>
+        <th>備考</th>
+    </tr>
+<?php foreach ($rows as $row): ?>
+    <tr>
+        <td><?=h($row['id'])?></td>
+        <td><?=h($row['name'])?></td>
+        <td><?=h($row['address'])?></td>
+        <td><?=h($row['tel1'])?></td>
+        <td><?=h($row['tel2'])?></td>
+        <td><?=h($row['email'])?></td>
+        <td><?=h($row['ndate'])?></td>
+        <td><?=h($row['remarks'])?></td>
+    </tr>
+<?php endforeach; ?>
+</table>
 ```
-この`$post`ってなんですかね？使ってない変数に見えるんですが．
-## 修正すべき脆弱性
-- ユーザから入力された変数をそのままSQL文の中に展開しているので，**SQLインジェクション攻撃**に対する致命的な脆弱性がある．この脆弱性は1つもあってはいけないレベルで危険．そもそもPDOの使い方を間違っているので，正しい用法を覚える．
-- `htmlspecialchars`を通さずにHTML内にテキストデータを表示しようとしている場所がある．**XSS攻撃**に対する脆弱性になり得るので，常に通すように心がけるべき．
-## 誤作動やエラーの防止
-- 変数未定義のエラーを防ぐために，`$_POST`の存在確認を`isset`で行うべき．欲を言えば更に`is_string`で文字列かどうかの確認を行うべき．これらをまとめて行ってくれる，`filter_input`の利用が推奨される．
-- `SET NAMES utf8`はデータベース側の文字セットしか変更できないため，基本的には,PDOドライバ側も同時に変更できる，DSNでの`charset=utf8`指定を利用すべき．
-- エラーモードがデフォルトの`PDO::ERRMODE_SILENT`のままなので，`->prepare()`のコールに失敗した場合にPDOExceptionが発生せず，単にfalseが返されるだけになってしまっている．エラーモードを`PDO::ERRMODE_EXCEPTION`に変更しない限りは，いちいち返り値をチェックする必要がある．
-- LIKE検索のためのエスケープを行っていないため，`%` `_` `\` といった特殊文字を含むキーワードを正しく取り扱えない．
-- SELECTの結果に対して`->rowCount()`は使えず，常にゼロになってしまうため，結果を配列として取得したあと配列に対して`count()`を実行する．
-## 読みやすいコードにするためには必要
-- HTMLの表示内容はほとんど重複するにも関わらず，絞り込み検索を行うためだけにPHPファイルを複製している．このような場合は「絞り込み条件が送信されてきたか」で判定し，HTML表示部分は共通化するだけで十分．
-- PHPのテンプレートエンジンとしての特性を無視して，`print`でHTMLタグ全体を出力している部分がある．基本的には変数に依存する部分のみを `<?= ~ ?>` 記法で出力するだけで十分．
-- ロジックとHTMLを可能な限り分離すべき．**「あらかじめ必要な変数を作っておき，すべての変数が用意できたらHTMLを書き始める」**というやり方を徹底する．`<body>`まで書いてからデータベースに接続して…，というのは典型的にダメな書き方．また，HTMLを途中まで書いておいて最後まで書き終わる前に`die`で強制終了するのNG．
-## 改善済みのコード
-```html
-<?php
-// HTML中にテキストを埋め込むときに利用する関数
-function h($str)
-{
-    return htmlspecialchars($str, ENT_QUOTES, 'UTF-8');
-}
-// $_POST['vid'], $_POST['nm'], $_POST['ndt'] を安全に文字列として受け取る
-// もし存在していない場合，空文字列になる
-$vid = (string)filter_input(INPUT_POST, 'vid');
-$nm = (string)filter_input(INPUT_POST, 'nm');
-$ndt = (string)filter_input(INPUT_POST, 'ndt');
-try {
-    // localhostのkadaiデータベースに文字コードutf8で接続
-    // SQL実行時のエラーもPDOExceptionに変換してスローさせる
-    $pdo = new PDO('mysql:dbname=kadai;host=localhost;charset=utf8', 'root', '', [
-        PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION,
-    ]);
-    // 共通のSQL文と空のパラメータ用配列を準備する
-    $sql = '
-        SELECT id, name, address, tel1, tel2, email, ndate, remarks
-        FROM mst_member
-        WHERE 1 = 1
-    ';
-    $params = [];
-    // $vid が入力されていたら，AND条件としてプレースホルダ「?」を追加し．
-    // そこに文字列として当てはめるための値を $params に追加する
-    if ($vid !== '') {
-        $sql .= ' AND id = ? ';
-        $params[] = $vid;
-    }
-    // $nm が入力されていたら，AND条件としてプレースホルダ「?」を追加し．
-    // そこに文字列として当てはめるための値を $params に追加する
-    // 名前に関してはLIKE検索を行うので，LIKE検索で使われる特殊文字を
-    // addcslashes関数を使ってエスケープしたあと，両端を%で囲んだ値を使用する
-    if ($nm !== '') {
-        $sql .= ' AND name like ? ';
-        $params[] = '%' . addcslashes($nm, '\_%') . '%';
-    }
-    // $hdt が入力されていたら，AND条件としてプレースホルダ「?」を追加し．
-    // そこに文字列として当てはめるための値を $params に追加する
-    if ($ndt !== '') {
-        $sql .= ' AND ndate >= ? ';
-        $params[] = $ndt;
-    }
-    // SQLを実行し，すべての結果を2次元の配列として一気に取得
-    $stmt = $pdo->prepare($sql);
-    $stmt->execute($params);
-    $rows = $stmt->fetchAll(PDO::FETCH_ASSOC);
-} catch (PDOException $e) {
-    // もしどこかでエラーが発生した場合，「500 Internal Server Error」として
-    // テキスト形式でエラーメッセージを表示して終了する
-    header('Content-Type: text/plain; charset=UTF-8', true, 500);
-    echo "ただいま障害により大変ご迷惑をお掛けしております。\n";
-    echo "Error: {$e->getMessage()}\n";
-    exit;
-}
-// エラーが起こらなければHTMLとして表示を開始する
-header('Content-Type: text/html; charset=UTF-8');
-?>
-<!DOCTYPE html>
-<title>名簿一覧</title>
-<h1>名簿一覧</h1>
-<form method="post" action="">
-    ID(半角) <input type="text" name="vid" value="<?=h($vid)?>"><br>
-    氏名 <input type="text" name="nm" value="<?=h($nm)?>"><br>
-    入会日 <input type="date" name="ndt" value="<?=($ndt)?>"><br>
-    <input type="submit" value="検索">
-</form>
-<?php if ($_SERVER['REQUEST_METHOD'] === 'POST'): /* POSTされたときだけ結果件数を示す */ ?>
-<p>検索結果は<?=h(count($rows))?>件です</p>
-<?php endif; ?>
-<table border="1">
-    <tr>
-        <th>ID</th>
-        <th>名前</th>
-        <th>住所</th>
-        <th>固定電話番号</th>
-        <th>携帯電話番号</th>
-        <th>Eメールアドレス</th>
-        <th>入会日</th>
-        <th>備考</th>
-    </tr>
-<?php foreach ($rows as $row): ?>
-    <tr>
-        <td><?=h($row['id'])?></td>
-        <td><?=h($row['name'])?></td>
-        <td><?=h($row['address'])?></td>
-        <td><?=h($row['tel1'])?></td>
-        <td><?=h($row['tel2'])?></td>
-        <td><?=h($row['email'])?></td>
-        <td><?=h($row['ndate'])?></td>
-        <td><?=h($row['remarks'])?></td>
-    </tr>
-<?php endforeach; ?>
-</table>
-```
 ## 参考リンク