msg# 1
hxrr
登録日: 2007-10-21
投稿数: 26
バグというほどのものではないのでしょうか。
念のためご報告いたします。
ユーザグループに属していないログインユーザが、
http://MYSITE/modules/user/admin/index.phpにアクセスし、会員情報を変更できてしまうことを発見しました。
投票数:2
平均点:10.00
msg# 1.1
GIJOE
登録日: 2006-3-20
投稿数: 3708
hxrrさん、こんにちは。
引用：
もし本当にそうなら、バグというより脆弱性に近いレベルですが…
ありえないとは思いつつ、念のため確認してみたら、本当にそうでした
HDではないXCL2.1単独でも再現したので、コア側の問題みたいです。
ユーザグループに所属していないユーザというのがそもそも存在しない、という前提なのかもしれませんが、これって相当に気持ち悪いですね。なんらかの理由でリレーションが切れた程度でサイト管理者まるまる持って行かれる…
とりあえず追ってみますか。
投票数:1
平均点:10.00
msg# 1.1.1
GIJOE
登録日: 2006-3-20
投稿数: 3708
追ってみました。
なるほど、XoopsGroupPermHandler::checkRight() の$gperm_groupidに、空の配列を渡すと、単にグループによる絞り込み条件がなくなってしまうんですね。
SELECT COUNT(*) FROM xoops_group_permission WHERE (gpe
rm_modid = '1' AND gperm_name = 'module_admin' AND gperm_itemid = '3')
本来発行されるべきSQLはこんな感じ。
SELECT COUNT(*) FROM xoops_group_permission WHERE (gpe
rm_modid = '1' AND gperm_name = 'module_admin' AND gperm_itemid = '3' AND (gperm_groupid =
'5'))
とりあえず、checkRight()メソッドにパッチするのがいいですかね。
html/kernel/groupperm.php
function checkRight($gperm_name, $gperm_itemid, $gperm_groupid, $gperm_modid = 1, $bypass_admincheck = false)
{
if (empty($gperm_groupid)) {
return false;
}
if (($bypass_admincheck == false) &&
((is_array($gperm_groupid) && in_array(XOOPS_GROUP_ADMIN, $gperm_groupid))||
(XOOPS_GROUP_ADMIN == $gperm_groupid))) {
return true;
}
$criteria =& $this->getCriteria($gperm_name, $gperm_itemid, $gperm_groupid, $gperm_modid);
if ($this->getCount($criteria) > 0) {
return true;
}
return false;
}
こんな感じで。
投票数:1
平均点:10.00
msg# 1.1.1.1
これ、Legacyのユーザーモジュールではそういう「どのグループにも所属させない」っていう操作はできないようになってるんですね。
こういう時こそPreloadで対応とか
(でも、アップデートされたらこのPreloadの処理に困るんですけど。空ファイルにするとかかな？
)
html/preload/PatchXUGJQandA1249.class.php
<?php
!defined('XOOPS_ROOT_PATH') and exit;
require_once XOOPS_ROOT_PATH.'/kernel/object.php';
require_once XOOPS_ROOT_PATH.'/kernel/groupperm.php';
class PatchXUGJQandA1249 extends XCube_ActionFilter
{
function preFilter()
{
$this->mRoot->mDelegateManager->add('Legacy.Event.GetHandler',
array($this, 'patchNoneGroupUsers'));
}
function patchNoneGroupUsers(&$handler, $name, $optional)
{
if ($name == 'groupperm'){
$class = 'XoopsGroupPermHandlerPatch';
$handler = new $class($GLOBALS['xoopsDB']);
}
}
}
class XoopsGroupPermHandlerPatch extends XoopsGroupPermHandler
{
function checkRight($gperm_name, $gperm_itemid, $gperm_groupid, $gperm_modid = 1, $bypass_admincheck = false)
{
if (empty($gperm_groupid))
{
return false;
}
return parent::checkRight($gperm_name, $gperm_itemid, $gperm_groupid, $gperm_modid, $bypass_admincheck );
}
}
投票数:1
平均点:10.00
msg# 1.2
hxrr
登録日: 2007-10-21
投稿数: 26
こんにちは、GIJOEさま、tohokuaikiさま
GIJOEさま、
大変すばやい分析と解決、いつもながら脱帽です。kernelに手を入れなければならないほどの問題だったとは^^;
念のためご報告してよかったというか、これを掲示板で書くのはまずかったのかもしれないという風に思っています。できればメッセージでGIJOEさまあてに送るほうが無難だったのかなと。もしこの脆弱性を試して、のっとられてしまうサイトが出かねないと思うと、ぞっとします。
ご指南の通り、一応、応急処置として、GIJOEさまの仰ったようにパッチを宛てることとします。
tohokuaikiさま、
確かにLegacyのユーザーモジュールでは、そのようなことが出来ませんが、ホダ塾ディストリビューションだと、新規ユーザーの一括登録をCSVで出来てしまい、その場合は初期ではユーザーグループには所属していないのです。
HDは、この点を改善しないとまずいと思います。
投票数:1
平均点:10.00
msg# 1.2.1
引用：
hxrrさんは書きました：
確かにLegacyのユーザーモジュールでは、そのようなことが出来ませんが、ホダ塾ディストリビューションだと、新規ユーザーの一括登録をCSVで出来てしまい、その場合は初期ではユーザーグループには所属していないのです。
HDは、この点を改善しないとまずいと思います。
たしかに、これはマズイですね。
ちょっとHDの開発MLにも投げておきます。レポートありがとうございます。
投票数:0
平均点:0.00
msg# 1.1.1.1.1
suin
登録日: 2006-3-23
投稿数: 72
引用：
tohokuaikiさんは書きました：
これ、Legacyのユーザーモジュールではそういう「どのグループにも所属させない」っていう操作はできないようになってるんですね。
ユーザモジュールでも「ユーザグループ管理」で登録ユーザグループから解除すれば、どのグループにも所属していないユーザを作ることができました。CSVのインポートは試してません。
偶発的でレアなケースですが、非常に恐ろしい穴ですね。このケースで発生したユーザは管理者とほぼ同等であるのを確認しました^^;
引用：
こういう時こそPreloadで対応とか
(でも、アップデートされたらこのPreloadの処理に困るんですけど。空ファイルにするとかかな？ )
html/preload/PatchXUGJQandA1249.class.php
コアをいじれる人はGIJOEさんので対応して、とりあえずパッチ待ちの人は早急にプリロードで対応すれば大丈夫そうです。tohokuaikiさんのプリロードを入れたら穴がふさがったのを確認できました。
投票数:1
平均点:10.00
msg# 1.1.1.1.1.1
GIJOE
登録日: 2006-3-20
投稿数: 3708
suinさん、こんにちは。
引用：
ユーザモジュールでも「ユーザグループ管理」で登録ユーザグループから解除すれば、どのグループにも所属していないユーザを作ることができました。CSVのインポートは試してません。
ああ、出来るんですか…。
であれば、やはりXCL2.1の問題として扱うことになりますね。
X2ではどうだったのだろう、と確認してみたら、そのあたりのコードはまったくの別物でした。そういう意味でもXCL2.1だけの問題ですね。
引用：
偶発的でレアなケースですが、非常に恐ろしい穴ですね。このケースで発生したユーザは管理者とほぼ同等であるのを確認しました^^;
状況によっては、並の管理者を越える権限ですね。管理者権限を２つ以上に分けているケースでも、そのORがとられますから。
ただ、minahitoさんがsf.netフォーラムでも書いてますけど、攻撃者自身が能動的に使える穴ではないので、さほど緊急性はないと考えます。
そして、私のパッチですが、このファイルをざっとチェックしてみたところ、checkRight()だけでなく、getItemIds()もグループIDを空配列で受け取ると全権限を返しそうだったので、こっちにもパッチを当ててます。
# getCriteria() の方に当ててしまうと、今度は getGroupIds() が効かなくなるのでそっちはなし
===================================================================
--- groupperm.php (礫窿吾с 蓋54)
+++ groupperm.php (礫窿吾с 蓋56)
@@ -370,6 +370,10 @@
*/
function checkRight($gperm_name, $gperm_itemid, $gperm_groupid, $gperm_modid = 1, $bypass_admincheck = false)
{
+ if (empty($gperm_groupid)) {
+ return false;
+ }
+
if (($bypass_admincheck == false) &&
((is_array($gperm_groupid) && in_array(XOOPS_GROUP_ADMIN, $gperm_groupid))||
(XOOPS_GROUP_ADMIN == $gperm_groupid))) {
@@ -442,6 +446,10 @@
{
$ret = array();
+ if (empty($gperm_groupid)) {
+ return $ret;
+ }
+
$criteria =& $this->getCriteria($gperm_name, 0, $gperm_groupid, $gperm_modid);
$perms =& $this->getObjects($criteria, true);
このパッチをコミットして、HD-1.0.3aとしてリリースし直してます。
ただ、リリースしておいてナニですが、グループに所属していないユーザって、そもそもログインさせない、というのが正しいんじゃないかとふと思い当たりました。
「ユーザは最低１つ以上のグループに所属している」
これを大前提として書かれているコードは、コアにもモジュールにも結構ありそうです。
というわけで、「グループに所属していないユーザはログインできない」というpreloadもお願いします
投票数:0
平均点:0.00
msg# 1.1.1.1.1.1.1
おはようございます。
バグ報告＆パッチ＆プリロードありがとうございます m(__)m
引用：
GIJOEさんは書きました：
ただ、リリースしておいてナニですが、グループに所属していないユーザって、そもそもログインさせない、というのが正しいんじゃないかとふと思い当たりました。
僕もこのアプローチに賛成です。
できるだけ認証時にグループの割当を確認してログインの正否を決める処理を書くという一方で、認証方法を追加するモジュールや preload がすでにあるということを考えると……
ちょっと野暮ったいですが、 BASE 側としては、
全認証処理が終わった後、所属グループを確認して、グループに所属していなければログインを取り消すという処理を念のため入れておく……としておいたほうがいいでしょうか。
そうすれば昔フォーラムに書いて投げたような E メールログイン preload を使ってる場合でも、一応「グループに所属しているユーザーはログインに成功しない」という動作を担保できます。
（「ログインできない」じゃないですが^^;
投票数:0
平均点:0.00
msg# 1.1.1.1.1.1.2
引用：
GIJOEさんは書きました：
というわけで、「グループに所属していないユーザはログインできない」というpreloadもお願いします
この案は、分かりやすいですね。
html/preload/PatchXUGJQandA1249.class.php
<?php
!defined('XOOPS_ROOT_PATH') and exit;
class PatchXUGJQandA1249 extends XCube_ActionFilter
{
function preFilter()
{
if (!defined("LEGACY_MODULE_VERSION") ||
version_compare(LEGACY_MODULE_VERSION, "2.1.7", "<"))
{
$this->mRoot->mDelegateManager->add("Site.CheckLogin.Success",
array($this, "noGroupNoLogin"),
XCUBE_DELEGATE_PRIORITY_FIRST);
}
}
function noGroupNoLogin(&$xoopsUser)
{
$successFlag = false;
$groups = $xoopsUser->getGroups();
if (count($groups)==0){
$this->mRoot->mController->mLogout->call(new XCube_Ref($successFlag), $xoopsUser);
// $this->mRoot->mController->executeForward(XOOPS_URL);
$this->mRoot->mController->executeRedirect(XOOPS_URL, 5,
'This User is not belonging to any user groups.');
exit();
}
}
}
?>
executeForwardよりRedirectの方が良いかな？
hd_defaultだと、メッセージ出ないんですが・・・。
多分、ログアウト処理の時にSESSIONを消しちゃってるからかな・・・。
投票数:0
平均点:0.00
msg# 1.1.1.1.1.1.2.1
引用：
tohokuaikiさんは書きました：
hd_defaultだと、メッセージ出ないんですが・・・。 多分、ログアウト処理の時にSESSIONを消しちゃってるからかな・・・。
ズバリそうでしょう。
ログアウト時のメッセージ、hd_default だとでないんですヨ ^^;
投票数:0
平均点:0.00
msg# 1.1.1.1.1.1.2.2
GIJOE
登録日: 2006-3-20
投稿数: 3708
さっそくのPreload作成、ありがとうございました。＞tohokuaikiさん
grouppermハンドラをひっかけるより、こっちの方がつなぎのpreloadとしてはベターな気がしますね。
投票数:0
平均点:0.00
msg# 1.3
hxrr
登録日: 2007-10-21
投稿数: 26
こんにちは、GIJOEさま、tohokuaikiさま、suinさま、minahito さま、jidaikoboさま、錚々たる顔ぶれのみなさま、バグ修正、報告等、ありがとうございます。
よりXOOPSの安全性が増して、ますます、XOOPSの魅力が高まるので、うれしく思っています。
思いつきでいってしまって、大変恐縮なのですが、ユーザー編集画面のサブミットと、 ユーザーモジュール »» ユーザーグループ管理 »» メンバーの登録 での割り当て解除時で、ユーザーグループに所属していない場合は、エラーを表示するという仕様はいかかでしょうか。
また、ユーザーモジュールの管理権限を持っているユーザが管理画面に遷移した場合に、そのようなユーザがいないかどうかをチェックし、POROTECTORのようにユーザー情報などに異常がないかを一覧表示するという仕様とか。
もしくは、PROTECTORにユーザ情報の異常チェック機能を追加するか。
もしくは、白扇の改良版を、HDの推奨モジュールに追加して、ユーザー情報関連のチェック機能を担当させるとか。ついでに、ユーザ情報の設定でかゆい所に手が届きにくかった点を改善させられたらとも思っています。
投票数:0
平均点:0.00
msg# 1.3.1
GIJOE
登録日: 2006-3-20
投稿数: 3708
hxrrさん、こんにちは。
いろいろなアイデアを頂戴しましたが、それを自由に選択できるのがpreloadという仕組みです。
- grouppermハンドラを乗っ取る
- グループに所属しないユーザはログイン拒否
- グループに所属しないユーザがログインしてきたら強制的にgroupid=2に登録
という各種preloadが提案されていて、そのどれを選ぶのもサイトオーナーの自由なのですから、それが一番だと思いませんか？
もちろん、権限クエリがおかしくなる（全権限が返ってきてしまう）という部分は、セーフティネットとしてコアにパッチを入れるべきだとは思います。(HD-1.0.3aと同じもの）
引用：
もしくは、PROTECTORにユーザ情報の異常チェック機能を追加するか。
異常は異常ですけど、グループに所属しないユーザが権限上問題になるのは、XCL2.1だけですから、Protectorには実装しづらいですね。もしかしたら、本家系では、ユーザを無権限にするために、グループ所属なしにする、なんてのが一般的なやり方かもしれませんし。
投票数:1
平均点:10.00
msg# 1.3.1.1
hxrr
登録日: 2007-10-21
投稿数: 26
GIJOEさま
返信ありがとうございます。私も返信いたします。
引用：
いろいろなアイデアを頂戴しましたが、それを自由に選択できるのがpreloadという仕組みです。
なるほどです。プレロードになじみがないせいか、思いつきませんでした。プレロードをブラウザ上で指定できると、なじみやすくなるのかなと思ったりもします。
引用：
そのどれを選ぶのもサイトオーナーの自由なのですから、それが一番だと思いませんか？
選択の自由があるのは、確かにいいことだと思います。
引用：
XCL2.1だけですから、Protectorには実装しづらいですね。
なるほどです。
少しでも検討していただけでもありがたいです。
投票数:0
平均点:0.00
msg# 1.1.1.1.1.1.2.2.1
X2サイトを別サーバーのXCLに移転する作業中にここでぶつかりました。
X2サイトからbackbackでユーザー情報を取得。
XCLの一括登録のCSV形式に加工して、一括登録したところ、
すべての新規ユーザが所属グループなしとなってしまいました。
HD1.0.3aです。
html/kernel/groupperm.php
にはパッチは確かに当たっています。preloadは入れていません。
アクセスしたところたしかに、headerしか見えませんでした。
user.phpはアクセス出来るのですね。
ログアウトはできました。
X2用のgroupadminは登録グループがあらかじめ選択出来たかと思うのですが、XCLではこれらの新規ユーザを全員「登録ユーザ」とするためには、どうしたらよいでしょうか？
投票数:0
平均点:0.00
msg# 1.4
前の投稿
-
次の投稿
|
親投稿
-
子投稿なし
|
投稿日時 2009-4-12 21:16
すみません。とりあえず自己解決しました。
ユーザーモジュール »» ユーザーグループ管理 »» メンバーの登録
から登録できたのですね。不勉強でした。
ただ、400名ほどの登録だったので、少々大変でした。
やはり、新規登録時にグループ選択できる方法があるといいと思いました。
投票数:0
平均点:0.00
msg# 1.1.1.1.1.1.2.2.1.1
GIJOE
登録日: 2006-3-20
投稿数: 3708
いつの間にか解決済みになってますが、これ、本来はHDの不具合に近いですよね。
引用：
X2用のgroupadminは登録グループがあらかじめ選択出来たかと思うのですが、XCLではこれらの新規ユーザを全員「登録ユーザ」とするためには、どうしたらよいでしょうか？
ユーザー個別の対応は、X2もXCLもインターフェースとしては大差ありません。
ここで問題となっているのは、HD独自に実装した一括登録機能です。
一括登録で新規ユーザを登録するときに、グループ指定ができないのはいかにも片手落ち感は否めません。新規ユーザのグループをどこにするかの指定くらいつけた方が良さそうですね。さもなくば、機能ごと削る。
ついでに、`user_regdate` の処理がおかしい気がします。
INSERTの段階で、小さすぎる数値になってます。
strtotime()処理の問題？
投票数:0
平均点:0.00
登録日: 2006-5-16
投稿数: 389
登録日: 2006-3-20
投稿数: 656
