ZAPAnet総合情報局 > ZAPAブログ2.0 > MySQLとAjaxによる星型評価ボタンの脆弱性に注意

MySQLとAjaxによる星型評価ボタンの脆弱性に注意

2007年08月23日 プログラミングTIPS
MySQLとAjaxによる星型評価ボタンの設置方法*ホームページを作る人のネタ帳に、流行りのスターレイティングを表示するプログラムの設置方法が載っていました。
内容は、YvoSchaap.com - CSS: Star Rater Ajax Versionの日本語訳になっています。
このプログラムで気になった点、危険な点を挙げておきます。

3)PHPを作るに載っていたPHPのソースコードはかなり危険です。
if($_GET[’rating’] && $_GET[’imgId’]){
$dbh=mysql_connect ("localhost", “#######”, “"#######", “) or die (’I cannot connect to the database because: ‘ . mysql_error());
mysql_select_db (""#######", “);
$imgId = $_GET[’imgId’];
$text = addslashes(strip_tags($_GET[’rating’]));

//ads the rating to imgID in the database
$update = “update vote set voteNr = voteNr + 1, voteValue = voteValue + “.$_GET[’rating’].” WHERE imgId = ‘“.$imgId."‘“;
$result = mysql_query($update);
if(mysql_affected_rows() == 0){
//creates a new row if imgID has no own row yet
$insert = “insert into vote (voteNr,voteValue,imgId) values (’1’,’”.$_GET[’rating’]."’,’$imgId’)";
$result = mysql_query($insert);
}
}
まず、良くないのは1行目から。
if($_GET[’rating’] && $_GET[’imgId’]){
$_GET[’rating’]と$_GET[’imgId’]は定義されていない可能性があるので、isset関数を用いて変数がセットされているかどうかを検査した方が良いでしょう。(別に脆弱性の問題とは関係ありません)
例)
if( isset($_GET[’rating’], $_GET[’imgId’]) ){

そして、$_GET[’rating’]、$_GET[’imgId’]のどちらか、または両方に数値が入ることを期待しているのであれば(詳しくは見ていないので、数値かどうかは確認していません)、is_numeric関数を用いて、変数が数字または数値文字列であるかを調べた方が良いでしょう。(悪意あるコードが処理されるのを防ぐことにつながります)
例)
if( is_numeric($_GET[’imgId’]) ){

見るからに危険なのはこの一行。
$imgId = $_GET[’imgId’];
$_GET変数をそのまま信用して値を設定してしまっています。これは危険すぎます。
7)JavaScriptの呼び出しを忘れないのJavaScript内でも、
<li><a href="javascript:rateImg(1,’<? echo $imgId ?>’)" title=’1 star out of 5’ class=’one-star’>1</a></li>
$imgIdの内容を直接表示してしまっているので、imgIdに悪意のあるコードを書かれてしまった場合に危険なことが起こり得ます。

それから、
$text = addslashes(strip_tags($_GET[’rating’]));
の行で何か処理をしていますが、$textをどこで使うのかわかりません。

そして、一番危険なコードは、データベース部分です。
$update = “update vote set voteNr = voteNr + 1, voteValue = voteValue + “.$_GET[’rating’].” WHERE imgId = ‘“.$imgId."‘“;
$result = mysql_query($update);
$_GET[’rating’]を直接MySQLのSQL文として発行してしまっています。
これは、近年話題にもなっているSQLインジェクションにもつながります。
そもそも$imgIdを含め、$_GET変数をそのまま扱っていることに問題があります。
MySQLのエスケープ関数やプリペアードステートメントを利用することによって回避する必要があります。

6)PHPから現在の評価を取り出すコードにも危険なコードが含まれています。
<?
function getCurrenRating($imgId){

$sql= "select * from tblVote WHERE imgId= ‘“.addslashes($imgId).”’ LIMIT 0, 1”;
$result=@mysql_query($sql);
$rs=@mysql_fetch_array($result);

return @round($rs[voteValue] / $rs[voteNr],1);

}
?>
まず、PHPの始まりはphp.iniのshort_open_tagを無効にして、
<?php
〜〜〜
?>
を基本にしておいた方が安全です。

そして、この関数にもSQLインジェクションの危険性が存在します。
$sql= "select * from tblVote WHERE imgId= ‘“.addslashes($imgId).”’ LIMIT 0, 1”;
$result=@mysql_query($sql);
適切にエスケープ処理をしたつもりでも、addslashes($imgId)の使い方が間違っているケースがあります。
addslashes関数のページにも書いてある通り、
PHP ディレクティブ magic_quotes_gpc はデフォルトでは on で、 全ての GET、POST、COOKIE データについて基本的に addslashes() を実行します。 magic_quotes_gpc によってすでにエスケープされた文字列に対して addslashes() を実行しないでください。 さもないと、重複してエスケープされてしまいます。 関数 get_magic_quotes_gpc() はこれを確認するのに役立つかも知れません。
サーバーの設定に影響されるようなコードの書き方は良くありません。
また、文字コードによって問題が起きる場合もあります。(特にSJISに問題が多い)
ここでも、DB専用エスケープ関数やプリペアードステートメントを利用することによって危険を回避する必要性があります。


ざっとプログラムを眺めてみたところ、「MySQLとAjaxによる星型評価ボタン」プログラムにはいろいろと問題がありそうなことがわかりました。
海外のプログラムを翻訳して紹介するときは、そもそものプログラムに問題がある場合や日本語特有の問題が発生する場合があります。
元記事を詳しく読まない人も多いので、注意を促しておいた方が良いと思われます。