From 24128ac28d782f8522ec4c72988f54dc45eea998 Mon Sep 17 00:00:00 2001 From: Regis Houssin Date: Wed, 16 Jan 2019 11:07:58 +0100 Subject: [PATCH 1/6] TODO security broken with Multicompany --- htdocs/user/card.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/htdocs/user/card.php b/htdocs/user/card.php index 6db383ed515..d84c52068cc 100644 --- a/htdocs/user/card.php +++ b/htdocs/user/card.php @@ -85,7 +85,7 @@ if ($user->societe_id > 0) $socid = $user->societe_id; $feature2='user'; if ($user->id == $id) { $feature2=''; $canreaduser=1; } // A user can always read its own card -if (! $canreaduser) { +if (! $canreaduser) { // TODO security broken with Multicompany $result = restrictedArea($user, 'user', $id, 'user&user', $feature2); } From 45a7e0356236705c516e8098112cca0c7a2566ca Mon Sep 17 00:00:00 2001 From: Regis Houssin Date: Wed, 16 Jan 2019 19:13:21 +0100 Subject: [PATCH 2/6] FIX a user can always read its own card --- htdocs/core/lib/security.lib.php | 4 +++- htdocs/user/card.php | 5 +---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/htdocs/core/lib/security.lib.php b/htdocs/core/lib/security.lib.php index d12ee339909..d8193f8067e 100644 --- a/htdocs/core/lib/security.lib.php +++ b/htdocs/core/lib/security.lib.php @@ -181,7 +181,7 @@ function dol_verifyHash($chain, $hash, $type='0') */ function restrictedArea($user, $features, $objectid=0, $tableandshare='', $feature2='', $dbt_keyfield='fk_soc', $dbt_select='rowid', $objcanvas=null, $isdraft=0) { - global $db, $conf; + global $db, $conf, $user; global $hookmanager; //dol_syslog("functions.lib:restrictedArea $feature, $objectid, $dbtablename,$feature2,$dbt_socfield,$dbt_select"); @@ -253,6 +253,7 @@ function restrictedArea($user, $features, $objectid=0, $tableandshare='', $featu $tmpreadok=1; foreach($feature2 as $subfeature) { + if ($subfeature == 'user' && $user->id == $objectid) continue; // A user can always read its own card if (! empty($subfeature) && empty($user->rights->$feature->$subfeature->lire) && empty($user->rights->$feature->$subfeature->read)) { $tmpreadok=0; } else if (empty($subfeature) && empty($user->rights->$feature->lire) && empty($user->rights->$feature->read)) { $tmpreadok=0; } else { $tmpreadok=1; break; } // Break is to bypass second test if the first is ok @@ -262,6 +263,7 @@ function restrictedArea($user, $features, $objectid=0, $tableandshare='', $featu $readok=0; // All tests are ko (we manage here the and, the or will be managed later using $nbko). $nbko++; } + var_dump($readok); } else if (! empty($feature) && ($feature!='user' && $feature!='usergroup')) // This is for old permissions { diff --git a/htdocs/user/card.php b/htdocs/user/card.php index d84c52068cc..19e85962038 100644 --- a/htdocs/user/card.php +++ b/htdocs/user/card.php @@ -83,11 +83,8 @@ if ($id) $socid=0; if ($user->societe_id > 0) $socid = $user->societe_id; $feature2='user'; -if ($user->id == $id) { $feature2=''; $canreaduser=1; } // A user can always read its own card -if (! $canreaduser) { // TODO security broken with Multicompany - $result = restrictedArea($user, 'user', $id, 'user&user', $feature2); -} +$result = restrictedArea($user, 'user', $id, 'user&user', $feature2); if ($user->id <> $id && ! $canreaduser) accessforbidden(); From c4b9bdd569ccb6eaaf564c2c978e12985f5ac48c Mon Sep 17 00:00:00 2001 From: Regis Houssin Date: Wed, 16 Jan 2019 19:15:02 +0100 Subject: [PATCH 3/6] FIX remove var_dump --- htdocs/core/lib/security.lib.php | 1 - 1 file changed, 1 deletion(-) diff --git a/htdocs/core/lib/security.lib.php b/htdocs/core/lib/security.lib.php index d8193f8067e..12d0e4d08d0 100644 --- a/htdocs/core/lib/security.lib.php +++ b/htdocs/core/lib/security.lib.php @@ -263,7 +263,6 @@ function restrictedArea($user, $features, $objectid=0, $tableandshare='', $featu $readok=0; // All tests are ko (we manage here the and, the or will be managed later using $nbko). $nbko++; } - var_dump($readok); } else if (! empty($feature) && ($feature!='user' && $feature!='usergroup')) // This is for old permissions { From ffeeb782b0b655d79766c460cafe7c11366ab0e0 Mon Sep 17 00:00:00 2001 From: Regis Houssin Date: Wed, 16 Jan 2019 19:25:19 +0100 Subject: [PATCH 4/6] FIX check is moved to restrictedArea() function --- htdocs/user/agenda_extsites.php | 5 +---- htdocs/user/clicktodial.php | 5 +---- htdocs/user/document.php | 7 +++---- htdocs/user/info.php | 5 +---- htdocs/user/ldap.php | 2 +- htdocs/user/note.php | 2 +- htdocs/user/param_ihm.php | 6 +----- htdocs/user/perms.php | 5 ----- 8 files changed, 9 insertions(+), 28 deletions(-) diff --git a/htdocs/user/agenda_extsites.php b/htdocs/user/agenda_extsites.php index 2d94711aae8..541cf455a03 100644 --- a/htdocs/user/agenda_extsites.php +++ b/htdocs/user/agenda_extsites.php @@ -57,10 +57,7 @@ $object->getrights(); $socid=0; if ($user->societe_id > 0) $socid = $user->societe_id; $feature2 = (($socid && $user->rights->user->self->creer)?'':'user'); -if ($user->id == $id) // A user can always read its own card -{ - $feature2=''; -} + $result = restrictedArea($user, 'user', $id, 'user&user', $feature2); // If user is not user that read and no permission to read other users, we stop diff --git a/htdocs/user/clicktodial.php b/htdocs/user/clicktodial.php index a6c43e2dc62..ce5491f5e20 100644 --- a/htdocs/user/clicktodial.php +++ b/htdocs/user/clicktodial.php @@ -35,10 +35,7 @@ $id=GETPOST('id','int'); $socid=0; if ($user->societe_id > 0) $socid = $user->societe_id; $feature2 = (($socid && $user->rights->user->self->creer)?'':'user'); -if ($user->id == $id) // A user can always read its own card -{ - $feature2=''; -} + $result = restrictedArea($user, 'user', $id, 'user&user', $feature2); // Initialize technical object to manage hooks of page. Note that conf->hooks_modules contains array of hook context diff --git a/htdocs/user/document.php b/htdocs/user/document.php index 2c41c1bf922..fd30172135d 100644 --- a/htdocs/user/document.php +++ b/htdocs/user/document.php @@ -66,10 +66,9 @@ if ($id) $socid=0; if ($user->societe_id > 0) $socid = $user->societe_id; $feature2='user'; -if ($user->id == $id) { $feature2=''; $canreaduser=1; } // A user can always read its own card -if (!$canreaduser) { - $result = restrictedArea($user, 'user', $id, 'user&user', $feature2); -} + +$result = restrictedArea($user, 'user', $id, 'user&user', $feature2); + if ($user->id <> $id && ! $canreaduser) accessforbidden(); // Get parameters diff --git a/htdocs/user/info.php b/htdocs/user/info.php index 3cecf94543b..c1db0455628 100644 --- a/htdocs/user/info.php +++ b/htdocs/user/info.php @@ -43,10 +43,7 @@ if ($id > 0 || ! empty($ref)) $socid=0; if ($user->societe_id > 0) $socid = $user->societe_id; $feature2 = (($socid && $user->rights->user->self->creer)?'':'user'); -if ($user->id == $id) // A user can always read its own card -{ - $feature2=''; -} + $result = restrictedArea($user, 'user', $id, 'user&user', $feature2); // If user is not user that read and no permission to read other users, we stop diff --git a/htdocs/user/ldap.php b/htdocs/user/ldap.php index 6b8d0b2502e..dcd47e5283f 100644 --- a/htdocs/user/ldap.php +++ b/htdocs/user/ldap.php @@ -37,7 +37,7 @@ $contextpage=GETPOST('contextpage','aZ')?GETPOST('contextpage','aZ'):'userldap'; $socid=0; if ($user->societe_id > 0) $socid = $user->societe_id; $feature2 = (($socid && $user->rights->user->self->creer)?'':'user'); -if ($user->id == $id) $feature2=''; // A user can always read its own card + $result = restrictedArea($user, 'user', $id, 'user&user', $feature2); $object = new User($db); diff --git a/htdocs/user/note.php b/htdocs/user/note.php index 87a6785615e..7ca91200734 100644 --- a/htdocs/user/note.php +++ b/htdocs/user/note.php @@ -45,7 +45,7 @@ if (($object->id != $user->id) && (! $user->rights->user->user->lire)) accessfor $socid=0; if ($user->societe_id > 0) $socid = $user->societe_id; $feature2 = (($socid && $user->rights->user->self->creer)?'':'user'); -if ($user->id == $id) $feature2=''; // A user can always read its own card + $result = restrictedArea($user, 'user', $id, 'user&user', $feature2); // Initialize technical object to manage hooks of page. Note that conf->hooks_modules contains array of hook context diff --git a/htdocs/user/param_ihm.php b/htdocs/user/param_ihm.php index 9e1ef7b9444..0d9e81b95e6 100644 --- a/htdocs/user/param_ihm.php +++ b/htdocs/user/param_ihm.php @@ -48,11 +48,7 @@ if ($id) $socid=0; if ($user->societe_id > 0) $socid = $user->societe_id; $feature2 = (($socid && $user->rights->user->self->creer)?'':'user'); -if ($user->id == $id) // A user can always read its own card -{ - $feature2=''; - $canreaduser=1; -} + $result = restrictedArea($user, 'user', $id, 'user&user', $feature2); if ($user->id <> $id && ! $canreaduser) accessforbidden(); diff --git a/htdocs/user/perms.php b/htdocs/user/perms.php index 574aa44f89d..c1b2e6da717 100644 --- a/htdocs/user/perms.php +++ b/htdocs/user/perms.php @@ -58,11 +58,6 @@ if (! empty($conf->global->MAIN_USE_ADVANCED_PERMS)) $socid=0; if (isset($user->societe_id) && $user->societe_id > 0) $socid = $user->societe_id; $feature2 = (($socid && $user->rights->user->self->creer)?'':'user'); -if ($user->id == $id && (empty($conf->global->MAIN_USE_ADVANCED_PERMS) || $user->rights->user->self_advance->readperms)) // A user can always read its own card if not advanced perms enabled, or if he has advanced perms -{ - $feature2=''; - $canreaduser=1; -} $result = restrictedArea($user, 'user', $id, 'user&user', $feature2); if ($user->id <> $id && ! $canreaduser) accessforbidden(); From f7703235c7a85f32a80f74739af94def9462ad3a Mon Sep 17 00:00:00 2001 From: Regis Houssin Date: Wed, 16 Jan 2019 19:39:57 +0100 Subject: [PATCH 5/6] FIX broken feature (better check) --- htdocs/user/perms.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/htdocs/user/perms.php b/htdocs/user/perms.php index c1b2e6da717..721d95cdec3 100644 --- a/htdocs/user/perms.php +++ b/htdocs/user/perms.php @@ -58,6 +58,10 @@ if (! empty($conf->global->MAIN_USE_ADVANCED_PERMS)) $socid=0; if (isset($user->societe_id) && $user->societe_id > 0) $socid = $user->societe_id; $feature2 = (($socid && $user->rights->user->self->creer)?'':'user'); +if ($user->id == $id && (! empty($conf->global->MAIN_USE_ADVANCED_PERMS) && empty($user->rights->user->self_advance->readperms))) // A user can always read its own card if not advanced perms enabled, or if he has advanced perms +{ + accessforbidden(); +} $result = restrictedArea($user, 'user', $id, 'user&user', $feature2); if ($user->id <> $id && ! $canreaduser) accessforbidden(); From d5b66eeffb285d6470d3a393df6fff529242c0df Mon Sep 17 00:00:00 2001 From: Regis Houssin Date: Sat, 19 Jan 2019 18:12:48 +0100 Subject: [PATCH 6/6] FIX $user is already in parameters --- htdocs/core/lib/security.lib.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/htdocs/core/lib/security.lib.php b/htdocs/core/lib/security.lib.php index 12d0e4d08d0..1641a0ff300 100644 --- a/htdocs/core/lib/security.lib.php +++ b/htdocs/core/lib/security.lib.php @@ -181,7 +181,7 @@ function dol_verifyHash($chain, $hash, $type='0') */ function restrictedArea($user, $features, $objectid=0, $tableandshare='', $feature2='', $dbt_keyfield='fk_soc', $dbt_select='rowid', $objcanvas=null, $isdraft=0) { - global $db, $conf, $user; + global $db, $conf; global $hookmanager; //dol_syslog("functions.lib:restrictedArea $feature, $objectid, $dbtablename,$feature2,$dbt_socfield,$dbt_select");