View Issue Details

IDProjectCategoryView StatusLast Update
0017356phplist applicationBounce Managementpublic20-10-14 11:22
Reporterivilata 
PrioritynormalSeverityminorReproducibilityalways
Status closedResolutionfixed 
PlatformOSDebian WheezyOS Version7.5
Product Version3.0.7 
Target Version3.0.9Fixed in Version3.0.9 
Summary0017356: View bounces per list: normal admin can see bounces from other lists
DescriptionWith phpList versions 3.0.6 and 3.0.7, any normal (non-superuser) administrator can see the list the bouncing addresses of other administrator's lists via "View bounces per list". This can reveal addresses in other lists the admin shouldn't have access to: even if they are bouncing, names, domains or trivially fixable parts of addresses may give away lots of information. This is a very big privacy issue IMHO.
Steps To ReproduceLog in as a normal administrator and click on "View bounces per list" under the "Subscribers" menu. All lists having bounces are shown, even those of other admins, which is a privacy issue by itself. Then click on a list: bouncing addresses are directly available.
TagsNo tags attached.

Activities

michiel

05-09-14 21:33

manager   ~0054890


Hmm, if you need that much security, you'd be better of splitting it into different systems. The inter-admin security is not one of my main concerns. There should be a certain element of trust between them. It's mostly about workflow and segmentation, not security.

Also, bounced addresses are supposedly incorrect anyway, so that's not such a big deal. Targetting it for future research and review.

ivilata

08-09-14 09:58

reporter   ~0054906

That's really bad bad news for us. I saw no mention to the security vs workflow issue in phpList's web pages, so that led me to assume the different admins were independent, but even if we leave that aside and we stick to workflow, I see no point in cluttering one admin's views of lists and bounces with information from other admin's lists, so avoiding them would make it more clear and less confusing.

ivilata

08-09-14 12:07

reporter  

listbounces.php.diff (2,115 bytes)
--- listbounces.php.orig	2014-08-12 22:41:20.000000000 +0200
+++ listbounces.php	2014-09-08 13:00:45.398269356 +0200
@@ -1,14 +1,43 @@
 <?php
 
 require_once dirname(__FILE__).'/accesscheck.php';
+$access = accessLevel("listbounces");
 
 $listid = empty($_GET['id']) ? 0 : sprintf('%d',$_GET['id']);
 $download = isset($_GET['type']) && $_GET['type'] == 'dl';
 
+switch ($access) {
+  case "owner":
+    $subselect = " where owner = ".$_SESSION["logindetails"]["id"];
+    if ($listid) {
+      $query = "select id from " . $tables['list'] . $subselect . " and id = ?";
+      $rs = Sql_Query_Params($query, array($listid));
+      if (!Sql_Num_Rows($rs)) {
+        Fatal_Error($GLOBALS['I18N']->get("You do not have enough priviliges to view this page"));
+        return;
+      }
+    }
+    break;
+  case "all":
+  case "view":
+    break;
+  case "none":
+  default:
+    if ($listid) {
+      Fatal_Error($GLOBALS['I18N']->get("You do not have enough priviliges to view this page"));
+      return;
+    }
+    break;
+}
+
 if (!$listid) {
-  $req = Sql_Query(sprintf('select listuser.listid,count(distinct userid) as numusers from %s listuser,
-    %s umb, %s lm where listuser.listid = lm.listid and listuser.userid = umb.user group by listuser.listid
-    order by listuser.listid limit 150',$GLOBALS['tables']['listuser'],$GLOBALS['tables']['user_message_bounce'],$GLOBALS['tables']['listmessage']));
+  $isowner = sprintf(" list.owner = %d and ", $_SESSION["logindetails"]["id"]);
+  if ($access == "all") {
+    $isowner = "";
+  }
+  $req = Sql_Query(sprintf('select listuser.listid,count(distinct userid) as numusers from %s list, %s listuser,
+    %s umb, %s lm where %s list.id = listuser.listid and listuser.listid = lm.listid and listuser.userid = umb.user group by listuser.listid
+    order by listuser.listid limit 150',$GLOBALS['tables']['list'],$GLOBALS['tables']['listuser'],$GLOBALS['tables']['user_message_bounce'],$GLOBALS['tables']['listmessage'], $isowner));
   $ls = new WebblerListing($GLOBALS['I18N']->get('Choose a list'));
   $some = 0;
   while ($row = Sql_Fetch_Array($req)) {
listbounces.php.diff (2,115 bytes)

ivilata

08-09-14 12:10

reporter   ~0054912

I attached an emergency patch to listbounces.php (somehow based on members.php) that checks access before showing the bouncing addresses of a single list when a list ID is specified, and otherwise only shows the lists the logged in admin is owner of (superusers can see all lists).

michiel

08-09-14 20:21

manager   ~0054915

Ah, sure, I agree that for workflow you do not want to clutter the interface with unnecessary information.

Thanks for the patch. Any chance to make it a github pull request? That really help applying it. If not, I can try to find some time to apply the patch.

michiel

08-09-14 20:24

manager   ~0054916

Forget the last request. I applied the patch, and it works fine. I'll run some tests, to see if it does as it says and then commit it.

ivilata

09-09-14 09:14

reporter   ~0054926

Nice, thanks a lot!

michiel

09-09-14 17:49

manager   ~0054941

also applied it to the dropdown selection of lists.

https://github.com/michield/phplist/commit/c6948316ece1b5385f9a7dc4eddcdc7b31caff76

ivilata

20-10-14 11:22

reporter   ~0055464

Checked to work in 3.0.9, thanks!