[Gnash-commit] /srv/bzr/gnash/trunk r10406: Don't return a SharedObject when an impermissible path is requested. This is

[Benjamin Wolsey]

------------------------------------------------------------
revno: 10406
committer: Benjamin Wolsey <address@hidden>
branch nick: trunk
timestamp: Wed 2008-12-10 11:35:08 +0100
message:
  Don't return a SharedObject when an impermissible path is requested. This is
  the proper fix to the SharedObject.as test failures introduced yesterday.
modified:
  libcore/asobj/SharedObject.cpp
  testsuite/actionscript.all/Makefile.am
  testsuite/actionscript.all/SharedObject.as
  testsuite/misc-ming.all/SharedObjectTestRunner.sh
    ------------------------------------------------------------
    revno: 10405.1.1
    committer: Benjamin Wolsey <address@hidden>
    branch nick: work
    timestamp: Wed 2008-12-10 08:23:29 +0100
    message:
      Treat the localPath argument to SharedObject.getLocal() correctly. It
      must be part of the SWF path.
    modified:
      libcore/asobj/SharedObject.cpp
    ------------------------------------------------------------
    revno: 10405.1.2
    committer: Benjamin Wolsey <address@hidden>
    branch nick: work
    timestamp: Wed 2008-12-10 11:06:17 +0100
    message:
      Add tests and correct implementation for SharedObject path.
      
      Adjust misc-ming.all SharedObjectTestRunner.sh now that Gnash does the
      same as the pp.
    modified:
      libcore/asobj/SharedObject.cpp
      testsuite/actionscript.all/Makefile.am
      testsuite/actionscript.all/SharedObject.as
      testsuite/misc-ming.all/SharedObjectTestRunner.sh

=== modified file 'libcore/asobj/SharedObject.cpp'
--- a/libcore/asobj/SharedObject.cpp    2008-12-09 21:03:35 +0000
+++ b/libcore/asobj/SharedObject.cpp    2008-12-10 10:06:17 +0000
@@ -452,19 +452,27 @@
     const movie_root& mr = _vm.getRoot();
     const std::string& swfURL = mr.getOriginalURL();
 
-    // Get the domain part, or take as 'localhost' if none
-    // (loaded from filesystem)
     URL url(swfURL);
 
-    //  log_debug(_("BASE URL=%s (%s)"), url.str(), url.hostname());
+    // Remember the hostname of our SWF URL. This can be empty if loaded
+    // from the filesystem
     _baseDomain = url.hostname();
-    if ( _baseDomain.empty() ) _baseDomain = "localhost";
-
-    // Get the path part
-    _basePath = url.path();
-
-       // TODO: if the original url was a relative one, the pp uses just
-       // the relative portion rather then the resolved absolute path !
+
+    const std::string& urlPath = url.path();
+
+    // Get the path part. If loaded from the filesystem, the pp stupidly
+    // removes the first directory.
+    if (!_baseDomain.empty()) {
+        _basePath = urlPath;
+    }
+    else if (!urlPath.empty()) {
+        // _basePath should be empty if there are no slashes or just one.
+        std::string::size_type pos = urlPath.find('/', 1);
+        if (pos != std::string::npos) {
+            _basePath = urlPath.substr(pos);
+        }
+    }
+
 }
 
 void
@@ -482,28 +490,73 @@
 SharedObjectLibrary::getLocal(const std::string& objName,
         const std::string& root)
 {
-    assert ( ! objName.empty() );
+    assert (!objName.empty());
 
     // already warned about it at construction time
-    if ( _solSafeDir.empty() ) return 0; 
+    if (_solSafeDir.empty()) return 0; 
 
-    // TODO: this check sounds kind of lame, fix it
-    if ( rcfile.getSOLLocalDomain() && _baseDomain != "localhost") 
+    if (rcfile.getSOLLocalDomain() && !_baseDomain.empty()) 
     {
         log_security("Attempting to open SOL file from non "
                 "localhost-loaded SWF");
         return 0;
     }
 
-    // The optional second argument drops the domain and the swf file name
-    std::string key;
-    if ( root.empty() ) {
-        key = "/" + _baseDomain + "/" + _basePath + "/" + objName;
+    // The 'root' argument, otherwise known as localPath, specifies where
+    // in the SWF path the SOL should be stored. It cannot be outside this
+    // path.
+    std::string requestedPath;
+
+    // If a root is specified, check it first for validity
+    if (!root.empty()) {
+
+        const movie_root& mr = _vm.getRoot();
+        const std::string& swfURL = mr.getOriginalURL();
+        // The specified root may or may not have a domain. If it doesn't,
+        // this constructor will add the SWF's domain.
+        URL localPath(root, swfURL);
+        
+        StringNoCaseEqual noCaseCompare;
+
+        // All we care about is whether the domains match. They may be 
+        // empty filesystem-loaded.
+        if (!noCaseCompare(localPath.hostname(), _baseDomain)) {
+            log_security(_("SharedObject path %s is outside the SWF domain "
+                        "%s. Cannot access this object."), localPath, 
+                        _baseDomain);
+            return 0;
+        }
+
+        requestedPath = localPath.path();
+
+        // The domains match. Now check that the path is a sub-path of 
+        // the SWF's URL. It is done by case-insensitive string comparison,
+        // so a double slash in the requested path will fail.
+        if (!noCaseCompare(requestedPath,
+                    _basePath.substr(0, requestedPath.size()))) {
+            log_security(_("SharedObject path %s is not part of the SWF path "
+                        "%s. Cannot access this object."), requestedPath, 
+                        _basePath);
+            return 0;
+        }
+
     }
-    else key = root + "/" + objName;
+
+    // A leading slash is added later
+    std::ostringstream solPath;
+
+    // If the domain name is empty, the SWF was loaded from the filesystem.
+    // Use "localhost".
+    solPath << (_baseDomain.empty() ? "localhost" : _baseDomain) << "/";
+
+    // If no path was requested, use the SWF's path.
+    solPath << (requestedPath.empty() ? _basePath : requestedPath) << "/"
+            << objName;
 
     // TODO: normalize key!
 
+    const std::string& key = solPath.str();
+
     // If the shared object was already opened, use it.
     SoLib::iterator it = _soLib.find(key);
     if ( it != _soLib.end() )

=== modified file 'testsuite/actionscript.all/Makefile.am'
--- a/testsuite/actionscript.all/Makefile.am    2008-10-25 21:39:56 +0000
+++ b/testsuite/actionscript.all/Makefile.am    2008-12-10 10:06:17 +0000
@@ -25,7 +25,8 @@
 # so I take responsibility of dropping that for the moment...
 
 abs_mediadir = `cd $(srcdir)/../media; pwd`
-DEF_MAKESWF_FLAGS=-DMING_VERSION_CODE=$(MING_VERSION_CODE) 
-DMEDIADIR='\"$(abs_mediadir)\"'
+swfdir = `cd $(builddir); pwd`
+DEF_MAKESWF_FLAGS=-DMING_VERSION_CODE=$(MING_VERSION_CODE) 
-DMEDIADIR='\"$(abs_mediadir)\"' -DSWFDIR='\"$(swfdir)\"'
 
 quickcheck_RUNNERS = \
        alltests-v5-Runner \

=== modified file 'testsuite/actionscript.all/SharedObject.as'
--- a/testsuite/actionscript.all/SharedObject.as        2008-12-09 21:26:59 
+0000
+++ b/testsuite/actionscript.all/SharedObject.as        2008-12-10 10:06:17 
+0000
@@ -163,12 +163,73 @@
     trace("New Shared Object doesn't exist!");
 }
 
+/// Check localPath argument.
+
+// It seems this is quite restrictive, using a non-normalized text match
+// for the path, and removing the first directory for filesystem loads. Case
+// is irrelevant, however.
+
+note("Some of the following tests (all preceded by a 'checking' message) will 
fail when run over the network. Only the tests where an object is expected 
should fail");
+
 so4 = SharedObject.getLocal("Another-one", "/subdir");
-xcheck_equals(typeof(so4), "null");
+check_equals(typeof(so4), "null");
 check(so4 != so3);
-xcheck_equals(typeof(so4.data), 'undefined');
+check_equals(typeof(so4.data), 'undefined');
 ret = so4.flush();
-xcheck_equals(typeof(ret), 'undefined');
+check_equals(typeof(ret), 'undefined');
+
+so4 = SharedObject.getLocal("name", "subdir");
+check_equals(typeof(so4), "null");
+
+so4 = SharedObject.getLocal("name", "http://ahost/subdir";);
+check_equals(typeof(so4), "null");
+
+ourPath = SWFDIR + "/";
+note ("checking getLocal with path: " + ourPath);
+so4 = SharedObject.getLocal("name", ourPath);
+check_equals(typeof(so4), "null");
+
+ourPath = ourPath.substr(0, ourPath.lastIndexOf("/"));
+note ("checking getLocal with path: " + ourPath);
+so4 = SharedObject.getLocal("name", ourPath);
+check_equals(typeof(so4), "null");
+
+// Knock the first directory off. This is also confirmed to work with 
+// a SWF in /tmp/, but it can't really be tested here.
+ourPath = ourPath.substr(ourPath.indexOf("/", 1));
+note ("checking getLocal with path: " + ourPath);
+so4 = SharedObject.getLocal("name", ourPath);
+check_equals(typeof(so4), "object");
+
+// Upper case
+note ("checking getLocal with path: " + ourPath.toUpperCase());
+so4 = SharedObject.getLocal("name", ourPath.toUpperCase());
+check_equals(typeof(so4), "object");
+
+// Knock the last directory off
+ourPath = ourPath.substr(0, ourPath.lastIndexOf("/"));
+note ("checking getLocal with path: " + ourPath);
+so4 = SharedObject.getLocal("name", ourPath);
+check_equals(typeof(so4), "object");
+
+// Put one trailing slash on 
+ourPath += "/";
+note ("checking getLocal with path: " + ourPath);
+so4 = SharedObject.getLocal("name", ourPath);
+check_equals(typeof(so4), "object");
+
+// Put another trailing slash on 
+ourPath += "/";
+note ("checking getLocal with path: " + ourPath);
+so4 = SharedObject.getLocal("name", ourPath);
+check_equals(typeof(so4), "null");
+
+// Take the last slash off and add a bit of rubbish
+ourPath = ourPath.substr(0, ourPath.lastIndexOf("/"));
+ourPath += "aswfd.df";
+note ("checking getLocal with path: " + ourPath);
+so4 = SharedObject.getLocal("name", ourPath);
+check_equals(typeof(so4), "null");
 
 //------------------------------------------
 // Test that if 'data' is a getter-setter,
@@ -235,7 +296,7 @@
 // END OF TESTS
 //------------------------------------------
 
-check_totals(78);
+check_totals(88);
 
 #else
 

=== modified file 'testsuite/misc-ming.all/SharedObjectTestRunner.sh'
--- a/testsuite/misc-ming.all/SharedObjectTestRunner.sh 2008-09-27 07:54:15 
+0000
+++ b/testsuite/misc-ming.all/SharedObjectTestRunner.sh 2008-12-10 10:06:17 
+0000
@@ -30,8 +30,8 @@
 #
 # $ ./SharedObjectTestRunner flashplayer 
'/home/strk/.macromedia/Flash_Player/#SharedObjects/KZNLGTEU/'
 #
-# When you pass the second argument, the script will assume you're running with
-# the proprietary player thus stripping the ^/home/ portion out of SWF filename
+# The first directory (usually "/home/") is stripped, as the proprietary
+# player and now Gnash both do this.
 # (as this is what seems to happen)
 #
 #
@@ -42,11 +42,11 @@
 
 INPUTSOLDIR=${BASEINPUTSOLDIR}
 SWFTEST=${TOP_BUILDDIR}/testsuite/misc-ming.all/SharedObjectTest.swf
-SOLDIR=${TOP_BUILDDIR}/testsuite/tmpSharedObject/localhost/${SWFTEST}/
+homestrip=`echo ${SWFTEST} | sed -e 's/\/[^\/]*//'`
+SOLDIR=${TOP_BUILDDIR}/testsuite/tmpSharedObject/localhost/${homestrip}/
 
 if [ -n "$1" ]; then PLAYER="$1"; fi
 if [ -n "$2" ]; then
-    homestrip=`echo ${SWFTEST} | sed -e 's#^/home/##'`
     SOLDIR="$2/localhost/${homestrip}";
 fi