Fix 2017-SV4

See https://www.openmicroscopy.org/security/advisories/2017-SV4-guest-user/ for more information
ome · Oct 2, 2017 · e65be82 · e65be82
1 parent dfea9a7
commit e65be82
Show file tree

Hide file tree

Showing 5 changed files with 96 additions and 12 deletions.
diff --git a/components/blitz/src/ome/services/blitz/impl/ServiceFactoryI.java b/components/blitz/src/ome/services/blitz/impl/ServiceFactoryI.java
@@ -275,12 +275,12 @@ protected omero.ServerError handleException(Throwable t) {
 
     public IAdminPrx getAdminService(Ice.Current current) throws ServerError {
         return IAdminPrxHelper.uncheckedCast(getByName(ADMINSERVICE.value,
-                current));
+                current, true));
     }
 
     public IConfigPrx getConfigService(Ice.Current current) throws ServerError {
         return IConfigPrxHelper.uncheckedCast(getByName(CONFIGSERVICE.value,
-                current));
+                current, true));
     }
 
     public ILdapPrx getLdapService(Ice.Current current) throws ServerError {
@@ -322,7 +322,7 @@ public IScriptPrx getScriptService(Ice.Current current) throws ServerError {
 
     public ISessionPrx getSessionService(Current current) throws ServerError {
         return ISessionPrxHelper.uncheckedCast(getByName(SESSIONSERVICE.value,
-                current));
+                current, true));
     }
 
     public ISharePrx getShareService(Current current) throws ServerError {
@@ -427,6 +427,16 @@ public Object getServant(Ice.Identity id) {
 
     public ServiceInterfacePrx getByName(String blankname, Current dontUse)
             throws ServerError {
+        return getByName(blankname, dontUse, false);
+
+    }
+
+    public ServiceInterfacePrx getByName(String blankname, Current dontUse,
+            boolean allowGuest) throws ServerError {
+
+        if (!allowGuest) {
+            disallowGuest(blankname);
+        }
 
         // First try to get the blankname as is in case a value from
         // activeServices is being passed back in.
@@ -463,6 +473,15 @@ public ServiceInterfacePrx getByName(String blankname, Current dontUse)
 
     public StatefulServiceInterfacePrx createByName(String name, Current current)
             throws ServerError {
+        return createByName(name, current, false);
+    }
+
+    public StatefulServiceInterfacePrx createByName(String name, Current current,
+            boolean allowGuest) throws ServerError {
+
+        if (!allowGuest) {
+            disallowGuest(name);
+        }
 
         Ice.Identity id = holder.getIdentity(UUID.randomUUID().toString() + name);
         if (null != adapter.find(id)) {
@@ -548,6 +567,27 @@ public Object doWork(Session session, ServiceFactory sf) {
                 });
     }
 
+    private boolean isGuest() {
+        return (Boolean) executor.execute(this.principal,
+                new Executor.SimpleWork(this, "isGuest") {
+                    @Transactional(readOnly=true)
+                    public Object doWork(Session session, ServiceFactory sf) {
+                        LocalAdmin admin = (LocalAdmin) sf.getAdminService();
+                        EventContext ec = admin.getEventContextQuiet();
+                        long guestId = admin.getSecurityRoles().getGuestId();
+                        return ec.getCurrentUserId().equals(guestId);
+                    }
+                });
+    }
+
+    private void disallowGuest(String service)
+        throws SecurityViolation {
+        if (isGuest()) {
+            throw new SecurityViolation(null, null,
+                "Access denied to guest user: " + service);
+        }
+    }
+
     public long keepAllAlive(ServiceInterfacePrx[] proxies, Current __current)
             throws ServerError {
 

diff --git a/components/blitz/src/omero/cmd/Helper.java b/components/blitz/src/omero/cmd/Helper.java
@@ -27,7 +27,9 @@
 
 import ome.api.local.LocalAdmin;
 import ome.conditions.InternalException;
+import ome.conditions.SecurityViolation;
 import ome.system.EventContext;
+import ome.system.Roles;
 import ome.system.ServiceFactory;
 import ome.util.SqlAction;
 import omero.ServerError;
@@ -69,6 +71,10 @@ public class Helper {
 
     private boolean stepsSet = false;
 
+    private transient boolean isGuest = true;
+
+    private transient boolean preventGuest = true;
+
     public Helper(Request request, Status status, SqlAction sql,
             Session session, ServiceFactory sf) {
         synchronized (status) {
@@ -81,6 +87,11 @@ public Helper(Request request, Status status, SqlAction sql,
         this.sql = sql;
         this.session = session;
         this.sf = sf;
+        if (sf != null) {
+            long userId = getEventContext().getCurrentUserId();
+            Roles roles = sf.getAdminService().getSecurityRoles();
+            isGuest = (userId == roles.getGuestId());
+        }
         this.log = LoggerFactory.getLogger(
             this.request.toString().replaceAll("@", ".@"));
     }
@@ -102,7 +113,19 @@ private void requireStepsSet() {
         }
     }
 
+    /**
+     * Must be called before setSteps if access by the guest user is to be
+     * allowed.
+     */
+    public void allowGuests() {
+        preventGuest = false;
+    }
+
     public void setSteps(int steps) {
+        if (preventGuest && isGuest) {
+            cancel(new ERR(), new SecurityViolation("Disallowed for guests"),
+                    "guest-restriction");
+        }
         if (stepsSet) {
             cancel(new ERR(), new InternalException("Steps set!"), "steps-set");
         }

diff --git a/components/blitz/src/omero/cmd/admin/CurrentSessionsRequestI.java b/components/blitz/src/omero/cmd/admin/CurrentSessionsRequestI.java
@@ -87,6 +87,8 @@ public CurrentSessionsRequestI(CurrentDetails current,
         this.manager = manager;
     }
 
+    private boolean isCurrentUserGuest = false;
+
     //
     // CMD API
     //
@@ -99,6 +101,7 @@ public Map<String, String> getCallContext() {
     public void init(Helper helper) {
         this.helper = helper;
         this.helper.setSteps(1);
+        isCurrentUserGuest = current.isCurrentUserGuest();
     }
 
     public Object step(int step) throws Cancel {
@@ -135,30 +138,34 @@ public void buildResponse(int step, Object object) {
             CurrentSessionsResponse rsp = new CurrentSessionsResponse();
             rsp.sessions = new ArrayList<Session>(size);
             rsp.contexts = new ArrayList<omero.sys.EventContext>(size);
-            rsp.data = new Map[size];
-            int count = 0;
+            final List<Map<String, RType>> dataMaps = new ArrayList<Map<String, RType>>(size);
             for (Map.Entry<String, Map<String, Object>> entry : contexts.entrySet()) {
                 String uuid = entry.getKey();
                 Map<String, Object> data = entry.getValue();
                 EventContext orig = new SimpleEventContext(
                         (EventContext) data.get("sessionContext"));
                 Session s = objects.get(uuid);
-                rsp.sessions.add(s);
                 if (s == null) {
                     // Non-admin
+                    if (isCurrentUserGuest) {
+                        continue;
+                    }
+                    rsp.sessions.add(s);
                     omero.sys.EventContext ec = new omero.sys.EventContext();
                     rsp.contexts.add(ec);
                     ec.userId = orig.getCurrentUserId();
                     ec.userName = orig.getCurrentUserName();
                     ec.groupId = orig.getCurrentGroupId();
                     ec.groupName = orig.getCurrentGroupName();
                     ec.isAdmin = orig.isCurrentUserAdmin();
-                    rsp.data[count++] = new HashMap<String, RType>();
+                    dataMaps.add(Collections.<String, RType>emptyMap());
                 } else {
+                    rsp.sessions.add(s);
                     rsp.contexts.add(IceMapper.convert(orig));
-                    rsp.data[count++] = parseData(rsp, data);
+                    dataMaps.add(parseData(rsp, data));
                 }
             }
+            rsp.data = dataMaps.toArray(new Map[dataMaps.size()]);
             helper.setResponseIfNull(rsp);
         }
     }

diff --git a/components/server/src/ome/security/basic/BasicACLVoter.java b/components/server/src/ome/security/basic/BasicACLVoter.java
@@ -185,11 +185,13 @@ public boolean allowCreation(IObject iObject) {
                 || currentUser.getCurrentEventContext().isCurrentUserAdmin()) {
             return true;
         }
-
         else if (sysType) {
             return false;
         }
-
+        final EventContext ec = currentUser.getCurrentEventContext();
+        if (ec.getCurrentUserId() == roles.getGuestId()) {
+            return false;
+        }
         return true;
     }
 
@@ -378,7 +380,13 @@ private int allowUpdateOrDelete(BasicEventContext c, IObject iObject,
         for (int i = 0; i < scopes.length; i++) {
             Scope scope = scopes[i];
             if (scope == null) continue;
-
+            if (!sysType) {
+                if (c.getCurrentUserId() == roles.getGuestId()) {
+                    return 0;
+                }
+            } else if (sysType) {
+                // no privilege
+            } 
             if (leader) {
                 rv |= (1<<i);
             }

diff --git a/components/server/src/ome/security/basic/CurrentDetails.java b/components/server/src/ome/security/basic/CurrentDetails.java
@@ -461,6 +461,13 @@ void updateEvent(Event event) {
         current().setEvent(event);
     }
 
+    /**
+     * @return if the current user is the system's <q>guest</q> user
+     */
+    public boolean isCurrentUserGuest() {
+        return current().getCurrentUserId() == roles.getGuestId();
+    }
+
     // ~ Cleanups
     // =========================================================================
 
@@ -559,5 +566,4 @@ public boolean isDisabled(String id) {
         }
 
     }
-
 }