はじめに
実務未経験でWebエンジニアとして入社してからの2カ月で受けた社内研修についての振り返りです。
今回は「Webアプリケーション基礎」の課題について、 セキュリティ に関するレビュー内容に焦点を当てて、整理します（レビューで色々と指摘をいただいた、かつ、重要と感じたため）。
結論としては、 SQLインジェクション対策 や、 クロスサイトスクリプティング（XSS）対策 、 バリデーション がうまくできていなかった、という初歩的な内容です。
タイトルは、こちらの徳丸本を意識しています。（最近読了しましたが、今回の記事では直接的に触れていません）
記事の目的
同時期に実務未経験でWebエンジニアになって交流している方々（他社の）が、
「他の人が初期にどのような注意を受けているか聞いてみたい」
と言っていたため、私は自分が書いたコードをレビューしてもらった機会はまだ研修のみですが、どのような点をレビューで教えてもらったかについて、共有したいと思います。
振り返りを通して、私自身現在忘れてしまわずに身になっているか確認していきます。
想定する読者
想定する読者は、実務未経験の初学者の方です。（過去の自分を想定して書いてます）
どんな社内研修？
社内研修は、Web開発全般についての基礎的な研修で、以下のカリキュラムでした。
- PHPの基礎（PHP、オブジェクト指向）
- データベースの基礎（SQL、MySQL）
- Webアプリケーション基礎（HTML、CSS、CRUD、Cookie、セッション）
これらの内容についての課題を解いていき、 GitLab でマージリクエストして、社内のエンジニアの方にレビューをいただく、という流れで実施しました。
これまでに PHP と SQL については、コードレビューを題材にして記事を書きました。
Webアプリケーション基礎の課題は例えば下記のようなものでした。
「HTML, CSS, PHPを利用して、以下の画像と似たような画面を実装してください」
※ このスクショは当時の自分の回答
このような問題が数ステップあり、OKもらえるまで修正しました。
今回の内容は私がもらったリアルなレビューなので、初心者が見落としがちなポイントなのかなと思っています。
※ 記事にする都合上、レビューの文面は意味が変わらない程度に編集しています。
目次
1. SQLインジェクション対策
まずは、Webアプリケーションのセキュリティに関して、SQLインジェクション対策ができていなかったことに対するレビューです。
1.1. 「ユーザーの入力値をそのままSQLに入れないでください」
/**
* リクエストされた検索条件に従ってproductテーブルからデータを取得する
* - 商品名は部分一致での検索とする。
* - 価格は入力値以上の範囲検索とする。
* - 表示件数の選択肢は10件,20件,30件とする。
* - 作成日時の降順、IDの昇順で取得・表示とする。
*
* @param array $searchValues (バリデーションエラーがなかった)検索条件
* @return array $products リクエストされた検索条件で、productsテーブルからデータ取得された商品
*/
function searchProducts(array $searchValues):array
{
// データベースに接続する
$link = dbConnect();
// productsテーブルからリクエストされた検索条件に従って商品名、色、商品単価を取得するSQL文
$sql = <<<SQL
SELECT
product_name,
color,
price
FROM
products
WHERE
product_name LIKE '%{$searchValues['productName']}%'
コードレビューの内容
入力値をそのままSQLに入れることは絶対にしてはいけません。
SQLインジェクション によって不正にデータベースを操作されてしまいます。(例：商品名の入力を'; {任意のSQL文}とすると、任意のSQL文が実行できてしまいます)
対策として プリペアドステートメント とバインド変数を用いる方法があり、PDOでも利用可能です。
function searchProducts(array $searchValues):array
{
// データベースに接続する
$link = dbConnect();
// productsテーブルからリクエストされた検索条件に従って商品名、色、商品単価を取得するSQL文
$sql = <<<SQL
SELECT
product_name,
color,
price
FROM
products
WHERE
product_name LIKE :productName
AND
price >= :price
ORDER BY
created_at DESC,
product_id ASC
LIMIT
:limit
;
SQL;
$stmt = $link->prepare($sql);
// SQLインジェクションの対策として、プリペアドステートメントとバインド変数を用いる
// - LIKE検索のワイルドカードについて、エスケープする
$stmt->bindValue(':productName', '%'.addcslashes($searchValues['productName'], '\_%').'%', PDO::PARAM_STR);
$stmt->bindValue(':price', $searchValues['price'], PDO::PARAM_INT);
$stmt->bindValue(':limit', $searchValues['limit'], PDO::PARAM_INT);
// SQLを実行する
$stmt->execute();
// データを取得する
$products = $stmt->fetchAll(PDO::FETCH_ASSOC);
// データベースの接続を解除する
dbDisconnect($link);
return $products;
}
絶対にしてはいけないことをしてしまいました。
独学の頃にも学んだことはありましたが、重要性を理解できていませんでした！…
2. クロスサイトスクリプティング（XSS）対策
続いて、Webアプリケーションのセキュリティに関して、クロスサイトスクリプティング（XSS）ができていなかったことに対するレビューです。
2.1. 「ユーザーの入力値を表示する際のエスケープを必ず行ってください」
<main>
<div class="mx-4 my-2">
<h1>Cookie</h1>
<p><?php echo $name ?>さん、ようこそ！</p>
コードレビューの内容
ユーザーの入力値を表示する際のエスケープは、必ず行ってください。
※ レビュー後に調べたこと
- HTML や JavaScript を注入・変形される XSS（Cross-Site Scripting）脆弱性 が生じるため、 エスケープ する必要がある
- エスケープ とは、メタ文字の持つHTMLの文法上特別な意味を打ち消すこと
<main>
<div class="mx-4 my-2">
<h1>Cookie</h1>
<p><?php echo escape($name) ?>さん、ようこそ！</p>
<?php
/**
* XSS(クロスサイト・スクリプティング)対策として、特殊文字をエスケープする
*/
function escape($string)
{
return htmlspecialchars($string, ENT_QUOTES, 'UTF-8');
}
XSS 対策は課題の中で部分的に対策はしていたのですが、対策の抜け漏れがあり、自身のセキュリティへの認識の甘さを感じました。
研修でのレビューを通して、これらのセキュリティはWebアプリケーションを作成する上で当然わかっていないといけない、そしてそれを理解できていない、ということが浮き彫りになりました。
転職のためのポートフォリオとして作成したWebアプリケーションに、ザルすぎるセキュリティホールがなかったか今更ながら不安になりました…（今後、ソースコードを振り返る機会を作りたいです）
3. バリデーション
続いて、 バリデーション について、適切に入力値の検証ができていないことに対するレビューです。
バリデーション（入力値検証） は、ユーザーが入力した値が不正な値ではないかチェックすることです。
3.1. 「『パラメータが入力されているか』のみの検証はempty() ではなく、 isset() を使ってください」
<?= $_GET['productName'] === '0' || !empty($_GET['productName']) ? escape($_GET['productName']) : '';?>
コードレビューの内容
今回は「パラメータが入力されているか」を検証する必要があります。動作としては問題ないと思いますが、そもそもempty()を使って判定する必要はありません。
PHPマニュアル には以下の通り記載があります。
empty()は本質的に!isset($var) || $var == falseと同じことを簡潔に記述しているだけです。
「パラメータが入力されているか」（!isset($var)）の判定は必要 ですが、「入力されたパラメータの値が何か」（$var == false）の判定は不要 です。
なので、今回のケースではisset()を用いるのが適切です（空入力を判定したい場合は、$var === ''の判定を加える）。
※ レビュー後に調べたこと（isset()が何を返すかを var_dump で確認）
// アクセスした(GETパラメータがない)とき
var_dump($_GET['price']); // null
var_dump(isset($_GET['price'])); // boolean false
// '0'を入力して検索ボタンを押したとき
var_dump($_GET['price']); // string '0' (length=1)
var_dump(isset($_GET['price'])); // boolean true
// 未入力で検索ボタンを押したとき
var_dump($_GET['price']); // string '' (length=0)
var_dump(isset($_GET['price'])); // boolean true
<?= isset($_GET['productName']) ? escape($_GET['productName']) : '';?>
3.2. 「バリデーションでの整数の判定方法は正規表現を使ってください」
// 価格が10000000000以上、もしくは整数ではない場合、エラーメッセージ「価格は9999999999以下の整数で入力してください。」を表示する
if ((!ctype_digit($searchValues['price']) && $searchValues['price'] !== '')) {
$errors['price'] = '価格は正の整数で入力してください。';
} elseif ((int)$searchValues['price'] >= 10000000000) {
$errors['price'] = '価格は9999999999以下で入力してください。';
}
このページを参考に考えた結果、なぞなぞ不正解でした…
コードレビューの内容
バリデーションでの整数の判定方法は、 正規表現 を使ってください。
preg_match('/^-?[0-9]+$/', $a)
// 価格が10000000000以上、もしくは整数ではない場合、エラーメッセージ「価格は9999999999以下の整数で入力してください。」を代入する
if (((int)$searchValues['price'] >= 10000000000) || !preg_match('/^-?[0-9]+$/', $searchValues['price'])) {
$errors['price'] = '価格は9999999999以下の整数で入力してください。';
}
正規表現、奥が深そう…。
おわりに
「セキュリティについて体系的に勉強しておいてください」
研修を終えて、「セキュリティは重要なので、現状のような指摘されてから対処するという場あたり的な状態から抜け出すために体系的に勉強しておいてほしい。まずはこれを読んでおいてほしい」と言われたものが以下の参考資料です。
通読したのですが、代表的な脆弱性と、その脅威、対応策がまとめられていました。代表的な攻撃手段を一通り把握できていると、ここにはセキュリティの対策が必要ではないかと開発時に思い当たるようになると思うので、とても勉強になりました。（巻末にチェックシートがついてるので、対策に漏れがないかのチェックにも使えそうです）
また、「はじめに」でも表紙を貼りましたが、徳丸浩先生の著書『安全なWebアプリケーションの作り方』（通称、徳丸本）についても、最近通読しました！が、まだ理解が浅いです。（688ページありました！…）
ちなみに、徳丸本読んだ後はこれがおすすめ！という取り組みがもしあれば教えていただけますと幸いです。
一つ教えていただいた TryHackMe というのが今は気になっています。
今回の記事は以上です！ありがとうございました。