このPHPコードはログインシステムに安全ですか？

Question

これは一部ですが、最初に接続を行い、次にユーザー名が存在するかどうかを確認してから、データを表に挿入します。 私はPHPについてよく知らないので、私のことを知る必要はありません。ちょうどここで学ぶことを試みる、そして私が正しい道にいるかどうか疑問に思っている。代わりに、インライン変数、グローバル変数を使用して

停止の

require("constants.php"); 
try { 
    $DBH = new PDO("mysql:host=$host;dbname=$dbname", $dbconnect, $dbpass); 
    $DBH->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);  
} 
catch(PDOException $e) { 
    echo "sorry, something happened. try going back and try again."; 
    file_put_contents('PDOErrors.txt', $e->getMessage(), FILE_APPEND); 
} 

function checkName(){ 
$STH = $DBH->query('SELECT username FROM users WHERE username = $username'); 
$STH->setFetchMode(PDO::FETCH_OBJ); 
while($row = $STH->fetch()) { 
    if($username != $row->username){ 
    $check = 1; 
    } 
    else{ 
    $check = 0; 
    } 
    return $check; 
} 
function createSalt() 
{ 
    $string = md5(uniqid(rand(), true)); 
    return substr($string, 0, 3); 
} 
function register(){ 
$check = checkName(); 
if($check == 1){ 
$salt = createSalt(); 
$hash = sha1($salt . $hash); 
$data = array($username, $hash, $salt, $ip); 
$STH = $DBH->("INSERT INTO users (username, password, salt, ip) values (?, ?, ?)"); 
$STH->execute($data); 
} 
} 

Answer 1

いいえ、安全ではありません。レジスタグローバルが有効であると仮定し、関数引数の代わりにグローバル変数を使用すると、コードを異なるコンテキストに移植することが非常に困難になります。不適切な書式設定はさておき、コメントの完全な欠落はコードの保守を困難にします。良いコーディングスタイルは良いプログラミングの前提条件であり、したがってセキュリティです。

コード自体にも特定の問題があります。 $ usernameが引用されていると仮定すると、非効率的に実行されます。また、$ username文字列とデータベースから返される文字列を比較するので、明らかに適切にエスケープされていないため、コードがインジェクション攻撃に対してオープンであることを意味します。あなたがPDOを使用しているので、ソリューションは単純にプリペアドステートメント/変数バインディングを使用することです - これはINSERTで実際に行っています！

すべての一致するユーザー名を反復することは意味をなさないものであり、動作を大きく損なうものです。より良いアプローチ（ただし右1）は次のようになります。

/** 
* @param username string - a candidate username 
* @param DBH - connected PDO object referencing user database 
* @return bool - true if username does not exist already 
* 
* search the current list of users to see if the candidate username is available 
*/ 
function checkName($username, $DBH){  
    $STH = $DBH->prepare('SELECT username FROM users WHERE username = :username'); 
    $STH->execute(array(':username'=>$username)); 
    $STH->setFetchMode(PDO::FETCH_OBJ); 
    $row = $STH->fetch(); 
    if ($row === false) { 
    die('whoops!'); 
    } 
    return $username!==$row->username; 
} 

適切なソリューション：と仮定すると、あなたのユーザ名は一意である（そして、彼らは本当に、本当にする必要があります）、その後、ユーザ名が前に存在しているかどうかをチェックする気にしないでくださいINSERT - ユニークなインデックスを作成し、INSERT後に重複したキーの失敗を確認します。

次に、最初の接続でtry/catchを使用しますが、後続のクエリではエラーチェックを行いません。例外が発生したので、エラーを記録して報告しても、残りのコードが実行されないように、この時点で制御の流れに対処しているようには見えません。

申し訳ありません - 安全ではありませんが、良いコードではありません。

Answer 2

1. 使用prepared statements、。代わりに$_POST['name']を使用してください。
2. if($username != $row->username){何??！？！ユーザー名は一意であるため、正しい行（常に正しい）または0行のいずれかになります。

Answer 3

1. あなたは$ユーザ名を定義しないと、あなたは、単一引用符で囲まれた文字列でそれを使用するので、SELECTクエリはあなたが準備されたステートメントを使用してユーザー名をバインドする必要があります

2. を実行します方法はありません文字列にユーザー名を直接入力するのではなく、SELECTクエリのパラメータに設定する

3. すでに使用されているかどうかを確認するためにそのユーザー名を持つユーザーを選択する必要はなく、 0以外の場合

4. あなたのSELECTクエリが行を返さない場合にのみ、私はそれらのコメントが有用であることを望む

行をループ内を返すために、あなたのcheckName関数は値を返しません。