Fix edition/deletion of scripts and file path manipulation

The commit includes fixes for the following secvuln: https://www.openmicroscopy.org/site/products/omero/secvuln/2017-SV1-filename https://www.openmicroscopy.org/site/products/omero/secvuln/2017-SV2-edit-rw https://www.openmicroscopy.org/site/products/omero/secvuln/2017-SV3-delete-script
ome · Mar 21, 2017 · 4df8bc5 · 4df8bc5
1 parent b249698
commit 4df8bc5
Show file tree

Hide file tree

Showing 8 changed files with 136 additions and 12 deletions.
diff --git a/components/blitz/resources/ome/services/blitz-servantDefinitions.xml b/components/blitz/resources/ome/services/blitz-servantDefinitions.xml
@@ -64,6 +64,8 @@
         <bean class="ome.services.blitz.impl.ScriptI">
             <constructor-arg ref="throttlingStrategy"/>
             <constructor-arg ref="scriptRepoHelper"/>
+            <constructor-arg ref="aclVoter"/>
+            <constructor-arg ref="omeroInterceptor"/>
             <constructor-arg ref="checksumProviderFactory"/>
             <constructor-arg ref="paramsCache"/>
         </bean>

diff --git a/components/blitz/resources/omero/api/ISession.ice b/components/blitz/resources/omero/api/ISession.ice
@@ -1,9 +1,6 @@
 /*
- *   $Id$
- *
  *   Copyright 2010 Glencoe Software, Inc. All rights reserved.
  *   Use is subject to license terms supplied in LICENSE.txt
- *
  */
 
 #ifndef OMERO_API_ISESSION_ICE
@@ -51,8 +48,6 @@ module omero {
                 /**
                  * Allows a user to open up another session for him/herself with the given
                  * defaults without needing to re-enter password.
-                 *
-                 * TODO Review the security of this method.
                  */
                 omero::model::Session createUserSession(long timeToLiveMilliseconds, long timeToIdleMilliseconds, string defaultGroup)
                 throws ServerError, Glacier2::CannotCreateSessionException;

diff --git a/components/blitz/src/ome/services/blitz/impl/ScriptI.java b/components/blitz/src/ome/services/blitz/impl/ScriptI.java
@@ -1,5 +1,5 @@
 /*
- *   Copyright 2008 University of Dundee. All rights reserved.
+ *   Copyright 2008-2016 University of Dundee. All rights reserved.
  *   Use is subject to license terms supplied in LICENSE.txt
  */
 
@@ -15,6 +15,8 @@
 import ome.api.RawFileStore;
 import ome.model.core.OriginalFile;
 import ome.model.enums.ChecksumAlgorithm;
+import ome.security.ACLVoter;
+import ome.security.basic.OmeroInterceptor;
 import ome.services.blitz.util.BlitzExecutor;
 import ome.services.blitz.util.BlitzOnly;
 import ome.services.blitz.util.ParamsCache;
@@ -31,6 +33,7 @@
 import omero.RInt;
 import omero.RType;
 import omero.ResourceError;
+import omero.SecurityViolation;
 import omero.ServerError;
 import omero.ValidationException;
 import omero.api.AMD_IScript_canRunScript;
@@ -89,12 +92,18 @@ public class ScriptI extends AbstractAmdServant implements _IScriptOperations,
 
     protected final ScriptRepoHelper scripts;
 
+    protected final ACLVoter aclVoter;
+
+    protected final OmeroInterceptor interceptor;
+
     protected final ChecksumProviderFactory cpf;
 
-    public ScriptI(BlitzExecutor be, ScriptRepoHelper scripts,
+    public ScriptI(BlitzExecutor be, ScriptRepoHelper scripts, ACLVoter aclVoter, OmeroInterceptor interceptor,
             ChecksumProviderFactory cpf, ParamsCache cache) {
         super(null, be);
         this.scripts = scripts;
+        this.aclVoter = aclVoter;
+        this.interceptor = interceptor;
         this.cpf = cpf;
         this.cache = cache;
     }
@@ -193,6 +202,38 @@ public Long call() {
         });
     }
 
+    /**
+     * Check that the user can write files in the current context.
+     * @param __current Ice method invocation context
+     * @throws SecurityViolation if the user may not write files in the current context
+     */
+    private void assertCanWriteFiles(final Current __current) throws SecurityViolation {
+        boolean allowCreation = false;
+        try {
+            allowCreation = (Boolean) factory.executor.execute(
+                    __current.ctx, factory.principal, new Executor.SimpleWork(this, "uploadScript-prep") {
+                        @Transactional(readOnly = true)
+                        public Boolean doWork(Session session, ServiceFactory sf) {
+                            final OriginalFile file = new OriginalFile();
+                            /* check with interceptor */
+                            try {
+                                interceptor.newTransientDetails(file);
+                            } catch (ome.conditions.SecurityViolation sv) {
+                                return false;
+                            }
+                            /* check with ACL voter */
+                            return aclVoter.allowCreation(file);
+                        }
+                    });
+        } catch (ome.conditions.SecurityViolation sv) {
+            /* cannot even access the current context */
+        }
+        if (!allowCreation) {
+            throw new SecurityViolation(null, null,
+                    "No permission to upload script");
+        }
+    }
+
     /**
      * Upload script to the server.
      *
@@ -203,6 +244,7 @@ public Long call() {
      */
     public void uploadScript_async(final AMD_IScript_uploadScript __cb,
             final String path, final String scriptText, final Current __current) throws ServerError {
+        assertCanWriteFiles(__current);
         safeRunnableCall(__current, __cb, false, new Callable<Long>() {
             public Long call() throws Exception {
                 OriginalFile file = makeFile(path, scriptText, __current);
@@ -216,6 +258,7 @@ public Long call() throws Exception {
     public void uploadOfficialScript_async(
             AMD_IScript_uploadOfficialScript __cb, final String path,
             final String scriptText, final Current __current) throws ServerError {
+        assertCanWriteFiles(__current);
         safeRunnableCall(__current, __cb, false, new Callable<Long>() {
             public Long call() throws Exception {
                 EventContext ec = factory.getEventContext();
@@ -528,11 +571,22 @@ public void deleteScript_async(final AMD_IScript_deleteScript cb,
         safeRunnableCall(__current, cb, true, new Callable<Object>() {
             public Object call() throws Exception {
 
-                OriginalFile file = getOriginalFileOrNull(id, __current);
+                final OriginalFile file = getOriginalFileOrNull(id, __current);
                 if (file == null) {
                     throw new ApiUsageException(null, null,
                             "No script with id " + id + " on server.");
                 }
+                final boolean allowDelete = (Boolean) factory.executor.execute(
+                        __current.ctx, factory.principal, new Executor.SimpleWork(this, "deleteScript-prep") {
+                            @Transactional(readOnly = true)
+                            public Boolean doWork(Session session, ServiceFactory sf) {
+                                return aclVoter.allowDelete(file, file.getDetails());
+                            }
+                        });
+                if (!allowDelete) {
+                    throw new SecurityViolation(null, null,
+                            "No permission to delete script with id " + id);
+                }
 
                 deleteOriginalFile(file, __current);
                 return null; // void

diff --git a/components/blitz/src/ome/services/blitz/repo/RepositoryDaoImpl.java b/components/blitz/src/ome/services/blitz/repo/RepositoryDaoImpl.java
@@ -16,6 +16,8 @@
 import ome.io.nio.FileBuffer;
 import ome.model.IObject;
 import ome.model.fs.FilesetJobLink;
+import ome.model.internal.Permissions.Right;
+import ome.model.internal.Permissions.Role;
 import ome.model.meta.Experimenter;
 import ome.parameters.Parameters;
 import ome.services.RawFileBean;
@@ -1063,7 +1065,11 @@ protected void canWriteParentDirectory(ServiceFactory sf, SqlAction sql,
 
         if (parentObjectOwnerId != roles.getRootId() || parentObjectGroupId != roles.getUserGroupId()) {
             final LocalAdmin admin = (LocalAdmin) sf.getAdminService();
-            if (!admin.canAnnotate(parentObject)) {
+            final EventContext ec = admin.getEventContext();
+            if (!(admin.canAnnotate(parentObject) ||
+                    parentObjectGroupId == roles.getUserGroupId() &&
+                    ec.getCurrentGroupPermissions().isGranted(
+                            parentObjectOwnerId == ec.getCurrentUserId() ? Role.USER : Role.GROUP, Right.ANNOTATE))) {
                 throw new ome.conditions.SecurityViolation(
                         "No annotate access for parent directory: "
                                 + parentId);

diff --git a/components/common/src/ome/api/ISession.java b/components/common/src/ome/api/ISession.java
@@ -43,8 +43,6 @@ public interface ISession extends ServiceInterface {
     /**
      * Allows a user to open up another session for him/herself with the given
      * defaults without needing to re-enter password.
-     * 
-     * TODO Review the security of this method.
      */
     Session createUserSession(long timeToLiveMilliseconds,
             long timeToIdleMillisecond, String defaultGroup);

diff --git a/components/server/src/ome/security/basic/BasicACLVoter.java b/components/server/src/ome/security/basic/BasicACLVoter.java
@@ -17,6 +17,7 @@
 import ome.conditions.InternalException;
 import ome.conditions.SecurityViolation;
 import ome.model.IObject;
+import ome.model.core.OriginalFile;
 import ome.model.internal.Details;
 import ome.model.internal.Permissions;
 import ome.model.internal.Permissions.Right;
@@ -31,6 +32,7 @@
 import ome.system.EventContext;
 import ome.system.Roles;
 
+import org.hibernate.LazyInitializationException;
 import org.hibernate.Session;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -339,6 +341,20 @@ private int allowUpdateOrDelete(BasicEventContext c, IObject iObject,
         }
 
         Permissions grpPermissions = c.getCurrentGroupPermissions();
+        final ExperimenterGroup grp = d.getGroup();
+        if (!(sysType || grp == null)) {
+            try {
+                if (grp.isLoaded()) {
+                    /* allow current group context to apply for adjusting directories in "user" group */
+                    if (!(iObject instanceof OriginalFile && iObject.isLoaded() && grp.getId() == roles.getUserGroupId() &&
+                            "Directory".equals(((OriginalFile) iObject).getMimetype()))) {
+                        grpPermissions = grp.getDetails().getPermissions();
+                    }
+                }
+            } catch (LazyInitializationException lie) {
+                /* cannot check if group is loaded */
+            }
+        }
         if (grpPermissions == null || grpPermissions == Permissions.DUMMY) {
             if (d.getGroup() != null) {
                 Long gid = d.getGroup().getId();
@@ -396,7 +412,7 @@ public Set<String> restrictions(IObject object) {
     public void postProcess(IObject object) {
         if (object.isLoaded()) {
             Details details = object.getDetails();
-            // Sets context values.s
+            // Sets context values.
             this.currentUser.applyContext(details,
                     !(object instanceof ExperimenterGroup));
 

diff --git a/components/server/src/ome/security/basic/OmeroInterceptor.java b/components/server/src/ome/security/basic/OmeroInterceptor.java
@@ -28,6 +28,8 @@
 import org.hibernate.type.Type;
 import org.springframework.util.Assert;
 
+import com.google.common.base.Splitter;
+
 import ome.conditions.ApiUsageException;
 import ome.conditions.GroupSecurityViolation;
 import ome.conditions.InternalException;
@@ -40,6 +42,7 @@
 import ome.model.IMutable;
 import ome.model.IObject;
 import ome.model.core.Image;
+import ome.model.core.OriginalFile;
 import ome.model.core.Pixels;
 import ome.model.display.RenderingDef;
 import ome.model.display.Thumbnail;
@@ -59,6 +62,7 @@
 import ome.system.Roles;
 import ome.tools.hibernate.ExtendedMetadata;
 import ome.tools.hibernate.HibernateUtils;
+import ome.tools.lsid.LsidUtils;
 
 /**
  * implements {@link org.hibernate.Interceptor} for controlling various aspects
@@ -81,6 +85,10 @@ public class OmeroInterceptor implements Interceptor {
 
     private static Logger log = LoggerFactory.getLogger(OmeroInterceptor.class);
 
+    /* array indices for OriginalFile "path" and "name" properties */
+    private static final String IDX_FILE_PATH = LsidUtils.parseField(OriginalFile.PATH);
+    private static final String IDX_FILE_NAME = LsidUtils.parseField(OriginalFile.NAME);
+
     private final Interceptor EMPTY = EmptyInterceptor.INSTANCE;
 
     private final SystemTypes sysTypes;
@@ -164,6 +172,22 @@ public boolean onSave(Object entity, Serializable id, Object[] state,
         return true; // transferDetails ALWAYS edits the new entity.
     }
 
+    /**
+     * Do not allow <q>.</q> and <q>..</q> components in file paths.
+     * @param filepath a file path
+     * @return if the file path contains any prohibited components
+     */
+    private static boolean isProblemFilepath(String filepath) {
+        for (final String component : Splitter.on('/').split(filepath)) {
+            switch (component) {
+            case ".":
+            case "..":
+                return true;
+            }
+        }
+        return false;
+    }
+
     /**
      * calls back to
      * {@link BasicSecuritySystem#checkManagedDetails(IObject, Details)} for
@@ -181,6 +205,18 @@ public boolean onFlushDirty(Object entity, Serializable id,
 
             Details newDetails = evaluateLinkages(iobj);
 
+            if (previousState != null && iobj instanceof OriginalFile && !currentUser.current().isCurrentUserAdmin()) {
+                final int pathIndex = HibernateUtils.index(IDX_FILE_PATH, propertyNames);
+                final int nameIndex = HibernateUtils.index(IDX_FILE_NAME, propertyNames);
+                final String currentPath = (String) currentState[pathIndex];
+                final String currentName = (String) currentState[nameIndex];
+                if (currentPath != null && currentName != null &&
+                        !(currentPath.equals(previousState[pathIndex]) && currentName.equals(previousState[nameIndex])) &&
+                        isProblemFilepath(currentPath + currentName)) {
+                    throw new SecurityViolation("only administrators may introduce non-canonical OriginalFile path or name");
+                }
+            }
+
             altered |= resetDetails(iobj, currentState, previousState, idx,
                     newDetails);
 

diff --git a/history.txt b/history.txt
@@ -1,6 +1,23 @@
 OMERO version history
 =====================
 
+5.2.8 (March 2017)
+------------------
+
+This is a security release including three security vulnerability fixes.
+
+:secvuln:`2017-SV1-filename` prevents users from accessing and manipulating
+other people's data by creating an original file and changing its path to
+point to another user's file on the underlying filesystem.
+
+:secvuln:`2017-SV2-edit-rw` prevents users in read-write groups from
+editing official scripts.
+
+:secvuln:`2017-SV3-delete-script` prevents the deletion of official
+scripts by users without the correct permissions to do so.
+
+It is highly recommended that you upgrade your server.
+
 5.2.7 (December 2016)
 ---------------------