このスクリプトを改善できますか？

Question

私はuser_login.phpに保存されているこのコードを改善できるか、間違っているのかどうか疑問に思っています。アプリケーションのスクリプトはすべて30-40行程度しかないので混乱しています。何かが欠落しているかどうか疑問に思っていました。

このスクリプトは、テンプレートファイルを除き、アプリケーション内の他のすべての人と同様に、ajax呼び出しで呼び出されます。

<?php 

# Ignore 
if (!defined('APP_ON')) { die('Silence...'); } 

# Gets the variables sent 
$user_name = post('user_name'); 
$user_password = extra_crypt(post('user_password')); 

# Check if the user exists 
if (!user::check($user_name, $user_password)) { template::bad($lang['incorrect_login']); } 

# Logging in 
$id = user::get_id($user_name, $user_password); 
$u = new user($id); 
$u->login(); 
template::good($lang['correct_login']); 

?> 

私はそれを説明するつもりです：

# Ignore 
if (!defined('APP_ON')) { die('Silence...'); } 

は、これは基本的にファイルが直接呼び出されていないことを確認してください。各URLはURL（es：www.mysite.com/user/view/id/1）を管理するindex.phpファイルにリダイレクトされ、適切なファイルが含まれます。このindex.phpファイルの最初の行には、define('APP_ON', true);があります。また、インデックスファイルは、一部の関数セットといくつかのクラスを呼び出すアプリケーションを初期化します。

# Gets the variables sent 
$user_name = post('user_name'); 
$user_password = extra_crypt(post('user_password')); 

機能post()は$ _POST [ 'USER_NAME']いくつかのチェックを行うの回収管理します。 機能extra_crypt()は、sha1とカスタムalghoritmを使用してパスワードを暗号化するだけです。 私はSQLクエリの準備文を使用していますので、post変数のエスケープについて心配しないでください。

# Check if the user exists 
if (!user::check($user_name, $user_password)) { template::bad($lang['incorrect_login']); 

userクラスは、静的メソッドと通常メソッドの両方を持ちます。静的なものは、idの開始を要求しません。通常のメソッドは開始します。たとえば、user::check();は、ユーザ名とパスワードがデータベースに存在するかどうかをチェックします。 templateクラスには、ヘッダーまたはフッターなしでユーザーに送信するための高速なダイアログボックスを管理する2つの静的メソッド（template::bad()およびtemplate::good()）があります。代わりに、あなたはクラス$t = new template('user_view')をインスタンス化した場合、テンプレートuser_view_body.phpが呼び出され、テンプレートに静的VARSを割り当てる$t->assign()と、そのページを管理することができ、または$t->loop()でループを開始することなど 最後に$langはでいくつかの共通の文字列を有するアレイでありますユーザーが設定した言語。

# Logging in 
$id = user::get_id($user_name, $user_password); 
$u = new user($id); 
$u->login(); 
template::good($lang['correct_login']); 

最後に実際のログインがあります。 userクラスがインスタンス化され、IDが復元されます。そのIDを持つユーザーがログインしていて、ユーザーにメッセージボックスを返します。 説明を明確にするために、上記のコメントを書いてください。

Answer 1

あなたが提供していない機能の背後にある非常に多くの機能を備えているため、何かを修正する必要があるかどうかは言い難いです。 post()で入力がきれいであることを確認していますか？データベースコールはuser::check()で安全ですか？ユーザーの設定/情報の保存を簡略化するためにセッションCookieを使用していますか？ templateクラスはよく書かれていますか？新しいユーザーを作成するときに、必要な情報（ログインの時刻、必要に応じてIPアドレスなど）をすべて記録していますか？ここで重要なのは、ユーザーが二重ログインしていないことを確認していますか？

あなたが書いたことについて私があなたに伝える唯一の具体的なことは、sha1 algorithm is completely brokenです。代わりに何かを使用する必要があります。以下のコメントを参考にしてください。

Answer 2

まず、sha1のようなメッセージダイジェスト関数は暗号化関数ではありません。したがって、混乱を避けるために、関数名からcryptを削除してください。さらに、パスワードを格納するための「カスタムアルゴリズム」というこのアイデアは、私を怖がらせます。 sha256はsha1よりも優れた選択ですが、sha1はまだNISTで承認されている機能のすべてが悪いというわけではありません。 md5とは異なり、誰もsha1の衝突を生成することはできませんでした（これは新しい数年で起こります）。あなたがsha1と一緒に行くならば、プレフィックス攻撃を妨げるように塩が接頭語であることを確認してください。

入力の検証は、必ず使用時に行う必要があります。 post（）関数はではなく、のいずれかである必要があります。入力の検証またはエスケープはです。これはセキュリティ・トレインが壊れたために削除されているmagic_quotes_gpcの化身に過ぎません。

PDOやADODBなどのパラメータライブラリは非常に優れているため、使用することをお勧めします。これは、使用時の消毒の素晴らしい例です。

テンプレートassign()を使用して、XSSのサニタイズを行うことができます。 Smartyには、これを行うhtmlspecialchars出力フィルタモジュールが付属しています。