Project

General

Profile

Bug(バグ) #4012

メールアドレス設定から意図的に他のメンバーと同じメールアドレスを設定できる

Added by Youichi Kimura almost 3 years ago. Updated 7 months ago.

Status:
Fixed(完了)
Priority:
Normal(通常)
Target version:
Start date:
2016-09-23
Due date:
% Done:

100%

3.6 で発生するか:
Unknown (未調査)
3.8 で発生するか:
Unknown (未調査)

Description

Overview (概要)

再現手順:

  1. メンバーA でログインし、PCメールアドレス設定 (/member/config?category=pcAddress) を開く
  2. PCメールアドレスに hoge@example.com を入力して「送信」ボタンをクリック
    • hoge@example.com 宛に確認メールが届くが、この時点ではメールに記載されている URL を開かない
  3. メンバーB でログインし、PCメールアドレス設定 (/member/config?category=pcAddress) を開く
  4. PCメールアドレスに hoge@example.com を入力して「送信」ボタンをクリック
  5. メンバーA によって送られた「メールアドレス変更ページのお知らせ」のメールに記載されている URL を開く
  6. メンバーA のパスワードを入力して「送信」ボタンをクリック
  7. メンバーB によって送られた「メールアドレス変更ページのお知らせ」のメールに記載されている URL を開く
  8. メンバーB のパスワードを入力して「送信」ボタンをクリック
  9. メンバーA, メンバーB のPCメールアドレスが同じ hoge@example.com に設定された状態になる

Causes (原因)

lib/config/config/member_config.yml では pc_address および mobile_address に対して IsUnique: true が指定されており、実際に「PCメールアドレス設定」のフォームでは MemberConfigForm::isUnique() によって重複チェックが行われている。
しかし、メールアドレス変更については、変更後のメールアドレス宛に /member/configComplete?token=*** のURLをメールで送った上で、確認が完了するまでは pc_address を変更しないという例外的な対応を取っている。そのため、MemberConfigForm のバリデーション時には重複しなかったが /member/configComplete の時点で重複するような場合には対処できていなかった。

Way to fix (修正内容)

opMemberActions::executeConfigComplete() において、pc_address_pre または mobile_address_pre を pc_address または mobile_address に変更する直前にも重複チェックを行う。これにより、上記の再現手順でいう手順 8 の段階でエラーが発生するようになる。

なお、手順 4 の段階で pc_address_pre が重複するためこの時点でエラーを発生させるといった修正も考えられるが、これをエラーとしてしまうと悪意のあるメンバーが任意のメールアドレスを確認が未完了のまま放置することによって他のメンバーがそのメールアドレスを使用できない状態に出来てしまうため、pc_address_pre に対する重複チェックは行わない。

スクリーンショット_2016-09-26_13.16.55.png View (64.6 KB) Youichi Kimura, 2016-09-26 15:23


Subtasks

Backport(バックポート) #4017: メールアドレス設定から意図的に他のメンバーと同じメールアドレスを設定できるFixed(完了)Youichi Kimura

Backport(バックポート) #4018: メールアドレス設定から意図的に他のメンバーと同じメールアドレスを設定できるFixed(完了)Youichi Kimura

History

#2 Updated by Youichi Kimura almost 3 years ago

  • Status changed from New(新規) to Pending Review(レビュー待ち)
  • % Done changed from 0 to 50

下記の Pull Request にて修正しました (#3077, #4012 の修正を含んでいます)
https://github.com/openpne/OpenPNE3/pull/372

#3 Updated by Youichi Kimura almost 3 years ago

手順 8 の member/configComplete で発生させるエラーについて、単に設定値が重複している旨のエラーを表示するだけでは意図が伝わりにくいように見えたため、下記のように変更後の設定値を表示する行を追加しています。

#4 Updated by Youichi Kimura almost 3 years ago

#5 Updated by Youichi Kimura almost 3 years ago

#7 Updated by Shinichi Urabe over 2 years ago

  • Status changed from Pending Review(レビュー待ち) to Rejected(差し戻し)

https://github.com/openpne/OpenPNE3/pull/372#pullrequestreview-13092311 に直接コメントしてフィードバックしました
確認をお願いします

#8 Updated by kaoru n over 2 years ago

  • Target version changed from OpenPNE 3.9.0-old to OpenPNE 3.9.0

対象バージョン変更のため、修正内容の確認を行います。

#9 Updated by Youichi Kimura almost 2 years ago

#4012-7 のコメントを転記

lib/model/doctrine/MemberConfig.class.php:

@@ -117,6 +117,19 @@ public function getSetting()
     return $config[$this->getName()];
   }

+  public function checkUniqueness()

メソッド名からはユニーク性をチェックするように見えますが、内部の処理では設定値からユニーク判定が不要な場合も true と返されますので、メソッド名と処理に差異があるように感じます。

lib/model/doctrine/MemberConfig.class.php:

+    $duplicate = $this->getTable()->retrieveByNameAndValue($this->name, $this->value);

(既に重複しているデータがある前提となりますが) 取得したレコードが複数存在した場合で且つ、
opDoctrineQuery::fetchOne() で取得したレコードがたまたま、ユーザ自身のレコードであった場合、重複判定が回避されます。

lib/action/opMemberAction.class.php:

@@ -309,6 +309,13 @@ public function executeConfigComplete(opWebRequest $request)
         }
         $config->setValue($pre->getValue());

+        if (!$config->checkUniqueness())
+        {
+          $this->getUser()->setFlash('error', 'The inputted value is already exist.');

エラーメッセージが親切であるのは良いことだと思いますが、該当メールアドレスがすでに SNS 上の誰かに紐付いて存在するという情報の暴露には繋がらないでしょうか。
変更しようとして受信したメールアドレスの持ち主自身での操作になると思いますので、難しいところですが。

#10 Updated by Youichi Kimura over 1 year ago

#4012-9 の指摘について:

lib/model/doctrine/MemberConfig.class.php:

@@ -117,6 +117,19 @@ public function getSetting()
     return $config[$this->getName()];
   }

+  public function checkUniqueness()

メソッド名からはユニーク性をチェックするように見えますが、内部の処理では設定値からユニーク判定が不要な場合も true と返されますので、メソッド名と処理に差異があるように感じます。

修正後のメソッド名として validateUniqueness で考えています。
メソッド名以外は変更なしで、設定値にエラーが無いか重複チェックが不要な場合は true、設定値の重複によるエラーが発生した場合は false が返ります。

lib/model/doctrine/MemberConfig.class.php:

+    $duplicate = $this->getTable()->retrieveByNameAndValue($this->name, $this->value);

(既に重複しているデータがある前提となりますが) 取得したレコードが複数存在した場合で且つ、
opDoctrineQuery::fetchOne() で取得したレコードがたまたま、ユーザ自身のレコードであった場合、重複判定が回避されます。

当チケットの修正前は他のメンバーと重複した pc_address, mobile_address が設定可能な状態であったため、この指摘について考慮が必要となります。

id member_id name value
1001 1 pc_address
1002 2 pc_address

例えば sns@example.com がPCメールアドレスとして重複登録されている場合、現在の修正通りに retrieveByNameAndValue メソッドで name = 'pc_address' AND value = 'sns@example.com' (実際は name_value_hash の値で比較される) に合致するレコードを 1 件だけ抽出すると、上記の表でいう id = 1001 または id = 1002 のどちらのレコードが返るか不定になります。

これを踏まえて「取得したレコードがたまたま、ユーザ自身のレコードであった場合、重複判定が回避されます」というのは、

  • member_id = 1 のメンバーがPCメールアドレスを sns@example.com から sns@example.com に変更
  • sns@example.com が他のメンバー (member_id = 2) と重複して登録されている
  • 重複チェック時に $duplicate に取得されたレコードが id = 1001 であった

の条件が揃った場合になります。
あまり現実味の無い状況ではありますが、以下のように member_id の条件を fetchOne で絞り込むより前に移動することで対処することが可能です。

--- a/lib/model/doctrine/MemberConfig.class.php
+++ b/lib/model/doctrine/MemberConfig.class.php
@@ -125,9 +125,12 @@
       return true;
     }

-    $duplicate = $this->getTable()->retrieveByNameAndValue($this->name, $this->value);
-
-    return !$duplicate || $duplicate->member_id === $this->member_id;
+    $duplicate = $this->getTable()->createQuery('mc')
+      ->andWhere('mc.member_id <> ?', $this->member_id)
+      ->andWhere('mc.name_value_hash = ?', $this->getTable()->generateNameValueHash($this->name, $this->value))
+      ->fetchOne();
+
+    return !$duplicate;
   }

   public function generateRoleId(Member $member)

lib/action/opMemberAction.class.php:

@@ -309,6 +309,13 @@ public function executeConfigComplete(opWebRequest $request)
         }
         $config->setValue($pre->getValue());

+        if (!$config->checkUniqueness())
+        {
+          $this->getUser()->setFlash('error', 'The inputted value is already exist.');

エラーメッセージが親切であるのは良いことだと思いますが、該当メールアドレスがすでに SNS 上の誰かに紐付いて存在するという情報の暴露には繋がらないでしょうか。
変更しようとして受信したメールアドレスの持ち主自身での操作になると思いますので、難しいところですが。

この箇所については、変更後のメールアドレスの所有者自身でなければこのエラーメッセージに辿り着くことは無いため、現状の修正のままで問題は無いと考えています。

#11 Updated by isao sano over 1 year ago

  • Status changed from Rejected(差し戻し) to Accepted(着手)
  • % Done changed from 50 to 0

#12 Updated by isao sano over 1 year ago

  • Status changed from Accepted(着手) to Pending Review(レビュー待ち)
  • % Done changed from 0 to 50

#4012-9 の方針で修正し
https://github.com/openpne/OpenPNE3/pull/503 にてプルリクエストしました。

#13 Updated by Rimpei Ogawa 12 months ago

  • Status changed from Pending Review(レビュー待ち) to Rejected(差し戻し)

#14 Updated by isao sano 11 months ago

  • Status changed from Rejected(差し戻し) to Accepted(着手)
  • % Done changed from 50 to 0

#15 Updated by isao sano 11 months ago

  • Status changed from Accepted(着手) to Pending Review(レビュー待ち)
  • % Done changed from 0 to 50

#16 Updated by Rimpei Ogawa 9 months ago

  • Status changed from Pending Review(レビュー待ち) to Pending Testing(テスト待ち)
  • % Done changed from 50 to 70

#17 Updated by kaoru n 8 months ago

  • Target version changed from OpenPNE 3.9.0 to OpenPNE-3.9.4

#18 Updated by Chiharu Nakajima 8 months ago

  • Status changed from Pending Testing(テスト待ち) to Rejected(差し戻し)
  • % Done changed from 70 to 50

修正を取り込んだ状態でメールアドレス変更を行うと、メールアドレス変更時のパスワード入力画面で正しいパスワードを入力して「送信」を選択した場合、以下の500エラーが発生します(他のユーザと同じメールアドレスに変更しようとしているかどうかにかかわらず)。

500 | Internal Server Error | Doctrine_Record_UnknownPropertyException
Unknown method MemberConfig::validateUniqueness
stack trace
at ()
in SF_ROOT_DIR/lib/vendor/symfony/lib/plugins/sfDoctrinePlugin/lib/vendor/doctrine/lib/Doctrine/Record.php line 2660 ...
            }

        }

        throw new Doctrine_Record_UnknownPropertyException(sprintf('Unknown method %s::%s', get_class($this), $method));

    }

    /**

#19 Updated by Chiharu Nakajima 7 months ago

  • Status changed from Rejected(差し戻し) to Pending Merge(マージ待ち)
  • % Done changed from 50 to 80

動作確認OKです

修正を取り込んだ状態でメールアドレス変更を行うと、メールアドレス変更時のパスワード入力画面で正しいパスワードを入力して「送信」を選択した場合、以下の500エラーが発生します(他のユーザと同じメールアドレスに変更しようとしているかどうかにかかわらず)。

#3077 の方の修正で試験をしていたことが原因でした( #4012 で差し戻し修正されていたため、 #4012 の修正を取り込まなければ行けなかった)。

#20 Updated by kaoru n 7 months ago

  • Status changed from Pending Merge(マージ待ち) to Fixed(完了)
  • % Done changed from 80 to 100

Also available in: Atom PDF