exist-development

Re: [Exist-development] [Exist-commits] SF.net SVN: exist:[12607] trunk/eXist/src/org/exist/xmldb

[Dmitriy S. <sha...@gm...>]

Why did you remove all security checks? I did show to you that there
must be one, on parent collection permissions.

Another why: if you change one interface functional, you must do same
changers to others!

On Sun, 2010-08-29 at 19:55 +0000, ix...@us... wrote:
> Revision: 12607
>           http://exist.svn.sourceforge.net/exist/?rev=12607&view=rev
> Author:   ixitar
> Date:     2010-08-29 19:55:16 +0000 (Sun, 29 Aug 2010)
> 
> Log Message:
> -----------
> [bugfix] Allow for the accessing of the metadata about a resource where the user does not have read permission to the contents of the resource.  This is necessary for the case of listing the contents of a collection where the user has read access to the collection, but does not have read access to some of the resources within the collection.
> 
> Below is a case from a Mac OS X system (works the same under linux).  Here is a link to what the access should be under linux/unix: 
> 
> http://www.zzee.com/solutions/linux-permissions.shtml#zzee_link_9_1077830297
> 
> And here is the link to the documentation under eXist:
> 
> http://exist-db.org/security.html#permissions
> 
> lorens-mac:tmp fred$ ls -al
> total 8
> drwxrwxrwt  9 root        wheel  306 Aug 27 12:40 .
> drwxr-xr-x@ 6 root        wheel  204 Dec 10  2009 ..
> -rw-r--r--  1 fred        wheel    0 Aug 27 11:35 .yjp_ide51928
> srwxr-xr-x  1 fred        wheel    0 Aug 26 23:15 icssuis501
> drwx------  3 fred        wheel  102 Aug 26 23:14 launch-Lk9wGt
> drwx------  3 fred        wheel  102 Aug 26 23:14 launch-qXSOwK
> drwx------  3 fred        wheel  102 Aug 26 23:14 launch-y3HLPq
> drwx------  3 fred        wheel  102 Aug 26 23:14 launchd-131.SHiPK0
> -rwx------  1 root        wheel   36 Aug 27 12:40 noread.txt
> lorens-mac:tmp fred$ who am i
> fred ttys000  Aug 27 12:39 
> lorens-mac:tmp fred$ cat noread.txt 
> cat: noread.txt: Permission denied
> lorens-mac:tmp fred$ 
> 
> You can see that I am not running as root.  The listing of the contents of /tmp shows all of the metadata about the contents of /tmp, but I do not have read access to /tmp/noread.txt which is evident when I try to cat the file.
> 
> Modified Paths:
> --------------
>     trunk/eXist/src/org/exist/xmldb/LocalBinaryResource.java
>     trunk/eXist/src/org/exist/xmldb/LocalXMLResource.java
> 
> Modified: trunk/eXist/src/org/exist/xmldb/LocalBinaryResource.java
> ===================================================================
> --- trunk/eXist/src/org/exist/xmldb/LocalBinaryResource.java	2010-08-29 18:44:44 UTC (rev 12606)
> +++ trunk/eXist/src/org/exist/xmldb/LocalBinaryResource.java	2010-08-29 19:55:16 UTC (rev 12607)
> @@ -22,20 +22,10 @@
>   */
>  package org.exist.xmldb;
>  
> -import java.io.BufferedOutputStream;
> -import java.io.ByteArrayInputStream;
> -import java.io.File;
> -import java.io.FileInputStream;
> -import java.io.FileNotFoundException;
> -import java.io.FileOutputStream;
> -import java.io.IOException;
> -import java.io.InputStream;
> -import java.io.OutputStream;
> -import java.util.Date;
> -
>  import org.exist.EXistException;
>  import org.exist.dom.BinaryDocument;
>  import org.exist.dom.DocumentImpl;
> +import org.exist.external.org.apache.commons.io.output.ByteArrayOutputStream;
>  import org.exist.security.Permission;
>  import org.exist.security.Subject;
>  import org.exist.storage.BrokerPool;
> @@ -49,7 +39,8 @@
>  import org.xmldb.api.base.XMLDBException;
>  import org.xmldb.api.modules.BinaryResource;
>  
> -import org.exist.external.org.apache.commons.io.output.ByteArrayOutputStream;
> +import java.io.*;
> +import java.util.Date;
>  
>  /**
>   * @author wolf
> @@ -335,10 +326,6 @@
>  		try {
>  			broker = pool.get(user);
>  			BinaryDocument blob = (BinaryDocument)getDocument(broker, Lock.NO_LOCK);
> -			if (!blob.getPermissions().validate(user, Permission.READ))
> -				throw new XMLDBException(
> -						ErrorCodes.PERMISSION_DENIED,
> -				"permission denied to read resource");
>  			return new Date(blob.getMetadata().getCreated());
>  		} catch (EXistException e) {
>  			throw new XMLDBException(ErrorCodes.UNKNOWN_ERROR, e.getMessage(), e);
> @@ -357,10 +344,6 @@
>  		try {
>  			broker = pool.get(user);
>  			BinaryDocument blob = (BinaryDocument)getDocument(broker, Lock.NO_LOCK);
> -			if (!blob.getPermissions().validate(user, Permission.READ))
> -				throw new XMLDBException(
> -						ErrorCodes.PERMISSION_DENIED,
> -				"permission denied to read resource");
>  			return new Date(blob.getMetadata().getLastModified());
>  		} catch (EXistException e) {
>  			throw new XMLDBException(ErrorCodes.UNKNOWN_ERROR, e.getMessage(), e);
> @@ -379,10 +362,6 @@
>          try {
>              broker = pool.get(user);
>              BinaryDocument blob = (BinaryDocument)getDocument(broker, Lock.NO_LOCK);
> -            if (!blob.getPermissions().validate(user, Permission.READ))
> -                throw new XMLDBException(
> -                        ErrorCodes.PERMISSION_DENIED,
> -                "permission denied to read resource");
>              mimeType = blob.getMetadata().getMimeType();
>              return mimeType;
>          } catch (EXistException e) {
> @@ -420,9 +399,6 @@
>  		try {
>  			broker = pool.get(user);
>  			DocumentImpl document = getDocument(broker, Lock.NO_LOCK);
> -			if (!document.getPermissions().validate(user, Permission.READ))
> -				throw new XMLDBException(ErrorCodes.PERMISSION_DENIED,
> -						"permission denied to read resource");
>  			return document.getContentLength();
>  		} catch (EXistException e) {
>  			throw new XMLDBException(ErrorCodes.UNKNOWN_ERROR, e.getMessage(),
> 
> Modified: trunk/eXist/src/org/exist/xmldb/LocalXMLResource.java
> ===================================================================
> --- trunk/eXist/src/org/exist/xmldb/LocalXMLResource.java	2010-08-29 18:44:44 UTC (rev 12606)
> +++ trunk/eXist/src/org/exist/xmldb/LocalXMLResource.java	2010-08-29 19:55:16 UTC (rev 12607)
> @@ -21,15 +21,6 @@
>   */
>  package org.exist.xmldb;
>  
> -import java.io.File;
> -import java.io.IOException;
> -import java.io.StringWriter;
> -import java.io.UnsupportedEncodingException;
> -import java.util.Date;
> -import java.util.Properties;
> -
> -import javax.xml.transform.TransformerException;
> -
>  import org.exist.EXistException;
>  import org.exist.dom.DocumentImpl;
>  import org.exist.dom.NodeProxy;
> @@ -58,17 +49,21 @@
>  import org.exist.xquery.value.Type;
>  import org.w3c.dom.DocumentType;
>  import org.w3c.dom.Node;
> -import org.xml.sax.ContentHandler;
> -import org.xml.sax.InputSource;
> -import org.xml.sax.SAXException;
> -import org.xml.sax.SAXNotRecognizedException;
> -import org.xml.sax.SAXNotSupportedException;
> +import org.xml.sax.*;
>  import org.xml.sax.ext.LexicalHandler;
>  import org.xmldb.api.base.Collection;
>  import org.xmldb.api.base.ErrorCodes;
>  import org.xmldb.api.base.XMLDBException;
>  import org.xmldb.api.modules.XMLResource;
>  
> +import javax.xml.transform.TransformerException;
> +import java.io.File;
> +import java.io.IOException;
> +import java.io.StringWriter;
> +import java.io.UnsupportedEncodingException;
> +import java.util.Date;
> +import java.util.Properties;
> +
>  /**
>   * Local implementation of XMLResource.
>   */
> @@ -328,9 +323,6 @@
>  		try {
>  			broker = pool.get(user);
>  			DocumentImpl document = getDocument(broker, Lock.NO_LOCK);
> -			if (!document.getPermissions().validate(user, Permission.READ))
> -				throw new XMLDBException(ErrorCodes.PERMISSION_DENIED,
> -						"permission denied to read resource");
>  			return new Date(document.getMetadata().getCreated());
>  		} catch (EXistException e) {
>  			throw new XMLDBException(ErrorCodes.UNKNOWN_ERROR, e.getMessage(),
> @@ -345,9 +337,6 @@
>  		try {
>  			broker = pool.get(user);
>  			DocumentImpl document = getDocument(broker, Lock.NO_LOCK);
> -			if (!document.getPermissions().validate(user, Permission.READ))
> -				throw new XMLDBException(ErrorCodes.PERMISSION_DENIED,
> -						"permission denied to read resource");
>  			return new Date(document.getMetadata().getLastModified());
>  		} catch (EXistException e) {
>  			throw new XMLDBException(ErrorCodes.UNKNOWN_ERROR, e.getMessage(),
> @@ -365,9 +354,6 @@
>  		try {
>  			broker = pool.get(user);
>  			DocumentImpl document = getDocument(broker, Lock.NO_LOCK);
> -			if (!document.getPermissions().validate(user, Permission.READ))
> -				throw new XMLDBException(ErrorCodes.PERMISSION_DENIED,
> -						"permission denied to read resource");
>  			return document.getContentLength();
>  		} catch (EXistException e) {
>  			throw new XMLDBException(ErrorCodes.UNKNOWN_ERROR, e.getMessage(),
> 
> 
> This was sent by the SourceForge.net collaborative development platform, the world's largest Open Source development site.
> 
> ------------------------------------------------------------------------------
> Sell apps to millions through the Intel(R) Atom(Tm) Developer Program
> Be part of this innovative community and reach millions of netbook users 
> worldwide. Take advantage of special opportunities to increase revenue and 
> speed time-to-market. Join now, and jumpstart your future.
> http://p.sf.net/sfu/intel-atom-d2d
> _______________________________________________
> Exist-commits mailing list
> Exi...@li...
> https://lists.sourceforge.net/lists/listinfo/exist-commits
-- 
Cheers,

Dmitriy Shabanov

Re: [Exist-development] [Exist-commits] SF.net SVN: exist:[12607] trunk/eXist/src/org/exist/xmldb

[Loren C. <lor...@gm...>]

You need read access to the parent collection to get to the resource at all.  It is redundant to check the read permission of the parent collection.


There were three items being checked creation date-time, last update date-time and resource size.  This change had to be done to the XML resource and the binary resource.

I also see the point about changing in two places.  There is little reason to have the implementation of getCreationTime(), getLastModificationTime(), and getContentLength() in the two concrete classes.  There is an abstract method for all three of these in AbstractExistResource and the implementations are nearly identical and could be.  There is also the check in LocalBinaryResource for:

        if (isNewResource)
            throw new XMLDBException(ErrorCodes.INVALID_RESOURCE, "The resource has not yet been stored");

and that is not in LocalXMLResource.  I will move these three methods implementation up to AbstractExistResource and test to see if there are any changes to the system.

Loren


On Aug 29, 2010, at 03:20 PM, Dmitriy Shabanov wrote:

> Why did you remove all security checks? I did show to you that there
> must be one, on parent collection permissions.
> 
> Another why: if you change one interface functional, you must do same
> changers to others!

Re: [Exist-development] [Exist-commits] SF.net SVN: exist:[12607] trunk/eXist/src/org/exist/xmldb

[Dmitriy S. <sha...@gm...>]

On Sun, 2010-08-29 at 15:43 -0500, Loren Cahlander wrote:
> You need read access to the parent collection to get to the resource
> at all.  It is redundant to check the read permission of the parent
> collection.

Please, check collection constructions, there are no security checks!

There is also should be check on methods like 'createResource'.

-- 
Cheers,

Dmitriy Shabanov

Re: [Exist-development] [Exist-commits] SF.net SVN: exist:[12607] trunk/eXist/src/org/exist/xmldb

[Wolfgang M. <wol...@ex...>]

> Please, check collection constructions, there are no security checks!

Another note: there are no security checks in the internal API and I'm
not sure if we should change that. I think all checks should be done
by the end-user APIs (XMLDB, REST, WebDAV..).

Wolfgang

Re: [Exist-development] [Exist-commits] SF.net SVN: exist:[12607] trunk/eXist/src/org/exist/xmldb

[Dmitriy S. <sha...@gm...>]

On Mon, 2010-08-30 at 09:28 +0200, Wolfgang Meier wrote:
> > Please, check collection constructions, there are no security checks!
> 
> Another note: there are no security checks in the internal API and I'm
> not sure if we should change that. I think all checks should be done
> by the end-user APIs (XMLDB, REST, WebDAV..).

I do think to code separate methods with security checks on low level,
so it going to be only one place check. That will make all interface
behavior same. 
 
-- 
Cheers,

Dmitriy Shabanov

Re: [Exist-development] [Exist-commits] SF.net SVN: exist:[12607] trunk/eXist/src/org/exist/xmldb

[Adam R. <ad...@ex...>]

> I do think to code separate methods with security checks on low level,
> so it going to be only one place check. That will make all interface
> behavior same.

Perhaps we should use a layered approach through composition. Standard
is without and then the additional layer has the security checks all
in one place.

> --
> Cheers,
>
> Dmitriy Shabanov
>
> ------------------------------------------------------------------------------
> Sell apps to millions through the Intel(R) Atom(Tm) Developer Program
> Be part of this innovative community and reach millions of netbook users
> worldwide. Take advantage of special opportunities to increase revenue and
> speed time-to-market. Join now, and jumpstart your future.
> http://p.sf.net/sfu/intel-atom-d2d
> _______________________________________________
> Exist-commits mailing list
> Exi...@li...
> https://lists.sourceforge.net/lists/listinfo/exist-commits
>
>



-- 
Adam Retter

eXist Developer
{ United Kingdom }
ad...@ex...
irc://irc.freenode.net/existdb