2010-02-27 Tatsuhiro Tsujikawa <t-tujikawa@users.sourceforge.net>

Discard torrent file if path data in it contains directory
	traversal directives.  Discard metalink:file element in Metalink3
	format if its name attribute contains directory traversal
	directives.  Ignore name attribute of metalink:signature element
	in Metalink3 format if it contains directory traversal directives.
	* src/MetalinkParserStateV3Impl.cc
	* src/bittorrent_helper.cc
	* src/message.h
	* test/BittorrentHelperTest.cc
	* test/Makefile.am
	* test/MetalinkProcessorTest.cc
	* test/metalink3-dirtraversal.xml
	* test/test.xml

ChangeLog

@@ -1,3 +1,19 @@
+2010-02-27  Tatsuhiro Tsujikawa  <t-tujikawa@users.sourceforge.net>
+
+	Discard torrent file if path data in it contains directory
+	traversal directives.  Discard metalink:file element in Metalink3
+	format if its name attribute contains directory traversal
+	directives.  Ignore name attribute of metalink:signature element
+	in Metalink3 format if it contains directory traversal directives.
+	* src/MetalinkParserStateV3Impl.cc
+	* src/bittorrent_helper.cc
+	* src/message.h
+	* test/BittorrentHelperTest.cc
+	* test/Makefile.am
+	* test/MetalinkProcessorTest.cc
+	* test/metalink3-dirtraversal.xml
+	* test/test.xml
+
 2010-02-27  Tatsuhiro Tsujikawa  <t-tujikawa@users.sourceforge.net>
 
 	Removed useless comment

src/MetalinkParserStateV3Impl.cc

@@ -113,8 +113,12 @@ void FilesMetalinkParserState::beginElement
     stm->setFileState();
     std::vector<XmlAttr>::const_iterator itr = findAttr(attrs, NAME);
     if(itr != attrs.end()) {
+      std::string name = util::trim((*itr).value);
+      if(util::detectDirTraversal(name)) {
+        return;
+      }
       stm->newEntryTransaction();
-      stm->setFileNameOfEntry(util::trim((*itr).value));
+      stm->setFileNameOfEntry(name);
     }
   } else {
     stm->setSkipTagState();
@@ -275,7 +279,10 @@ void VerificationMetalinkParserState::beginElement
         stm->setTypeOfSignature(util::trim((*itr).value));
         std::vector<XmlAttr>::const_iterator itr = findAttr(attrs, FILE);
         if(itr != attrs.end()) {
-          stm->setFileOfSignature(util::trim((*itr).value));
+          std::string file = util::trim((*itr).value);
+          if(!util::detectDirTraversal(file)) {
+            stm->setFileOfSignature(file);
+          }
         }
       }
     } else {

src/bittorrent_helper.cc

@@ -200,19 +200,18 @@ static void extractFileEntries
     }
     const BDE& nameData = infoDict[nameKey];
     if(nameData.isString()) {
-      // Split path by '/' just in case nasty ".." is included in name
+      if(util::detectDirTraversal(nameData.s())) {
-      std::vector<std::string> pathelems;
+        throw DL_ABORT_EX
-      util::split(nameData.s(), std::back_inserter(pathelems), "/");
+          (StringFormat(MSG_DIR_TRAVERSAL_DETECTED,nameData.s().c_str()).str());
-      name = util::joinPath(pathelems.begin(), pathelems.end());
+      }
-      torrent[NAME] = nameData.s();
+      name = nameData.s();
     } else {
       name = strconcat(File(defaultName).getBasename(), ".file");
-      torrent[NAME] = name;
     }
   } else {
     name = overrideName;
-    torrent[NAME] = name;
   }
+  torrent[NAME] = name;
 
   const BDE& filesList = infoDict[C_FILES];
   std::vector<SharedHandle<FileEntry> > fileEntries;
@@ -246,17 +245,15 @@ static void extractFileEntries
         throw DL_ABORT_EX("Path is empty.");
       }
       
-      std::vector<std::string> pathelem(pathList.size());
+      std::vector<std::string> pathelem(pathList.size()+1);
-      std::transform(pathList.listBegin(), pathList.listEnd(), pathelem.begin(),
+      pathelem[0] = name;
-                     std::mem_fun_ref(&BDE::s));
+      std::transform(pathList.listBegin(), pathList.listEnd(),
-      std::string path = name;
+                     pathelem.begin()+1, std::mem_fun_ref(&BDE::s));
-      strappend(path, "/", util::joinPath(pathelem.begin(), pathelem.end()));
+      std::string path = strjoin(pathelem.begin(), pathelem.end(), '/');
-      // Split path with '/' again because each pathList element can
+      if(util::detectDirTraversal(path)) {
-      // contain "/" inside.
+        throw DL_ABORT_EX
-      std::vector<std::string> elements;
+          (StringFormat(MSG_DIR_TRAVERSAL_DETECTED, path.c_str()).str());
-      util::split(path, std::back_inserter(elements), "/");
+      }
-      path = util::joinPath(elements.begin(), elements.end());
-
       std::deque<std::string> uris;
       createUri(urlList.begin(), urlList.end(), std::back_inserter(uris), path);
       SharedHandle<FileEntry> fileEntry
@@ -282,7 +279,7 @@ static void extractFileEntries
     std::deque<std::string> uris;
     for(std::vector<std::string>::const_iterator i = urlList.begin();
         i != urlList.end(); ++i) {
-      if(util::endsWith(*i, "/")) {
+      if(util::endsWith(*i, A2STR::SLASH_C)) {
         uris.push_back((*i)+name);
       } else {
         uris.push_back(*i);

src/message.h

@@ -180,6 +180,7 @@
 #define MSG_METADATA_SAVED _("Saved metadata as %s.")
 #define MSG_METADATA_NOT_SAVED _("Saving metadata as %s failed. Maybe file" \
                                  " already exists.")
+#define MSG_DIR_TRAVERSAL_DETECTED _("Detected directory traversal directive in %s")
 
 #define EX_TIME_OUT _("Timeout.")
 #define EX_INVALID_CHUNK_SIZE _("Invalid chunk size.")

test/BittorrentHelperTest.cc

@@ -48,8 +48,8 @@ class BittorrentHelperTest:public CppUnit::TestFixture {
   CPPUNIT_TEST(testLoadFromMemory);
   CPPUNIT_TEST(testLoadFromMemory_somethingMissing);
   CPPUNIT_TEST(testLoadFromMemory_overrideName);
-  CPPUNIT_TEST(testLoadFromMemory_joinPathMulti);
+  CPPUNIT_TEST(testLoadFromMemory_multiFileDirTraversal);
-  CPPUNIT_TEST(testLoadFromMemory_joinPathSingle);
+  CPPUNIT_TEST(testLoadFromMemory_singleFileDirTraversal);
   CPPUNIT_TEST(testGetNodes);
   CPPUNIT_TEST(testGetBasePath);
   CPPUNIT_TEST(testSetFileFilter_single);
@@ -91,8 +91,8 @@ public:
   void testLoadFromMemory();
   void testLoadFromMemory_somethingMissing();
   void testLoadFromMemory_overrideName();
-  void testLoadFromMemory_joinPathMulti();
+  void testLoadFromMemory_multiFileDirTraversal();
-  void testLoadFromMemory_joinPathSingle();
+  void testLoadFromMemory_singleFileDirTraversal();
   void testGetNodes();
   void testGetBasePath();
   void testSetFileFilter_single();
@@ -443,33 +443,33 @@ void BittorrentHelperTest::testLoadFromMemory_overrideName()
   CPPUNIT_ASSERT_EQUAL(std::string("aria2-override.name"), getName(dctx));
 }
 
-void BittorrentHelperTest::testLoadFromMemory_joinPathMulti()
+void BittorrentHelperTest::testLoadFromMemory_multiFileDirTraversal()
 {
   std::string memory =
     "d8:announce27:http://example.com/announce4:infod5:filesld6:lengthi262144e4:pathl7:../dir14:dir28:file.imgeee4:name14:../name1/name212:piece lengthi262144e6:pieces20:00000000000000000000ee";
 
   SharedHandle<DownloadContext> dctx(new DownloadContext());
   dctx->setDir("/tmp");
-  loadFromMemory(memory, dctx, "default");
+  try {
-
+    loadFromMemory(memory, dctx, "default");
-  // remove ".." element
+    CPPUNIT_FAIL("Exception must be thrown.");
-  CPPUNIT_ASSERT_EQUAL(std::string("../name1/name2"), getName(dctx));
+  } catch(RecoverableException& e) {
-  CPPUNIT_ASSERT_EQUAL(std::string("/tmp/name1/dir1/dir2/file.img"),
+    // success
-                       dctx->getFirstFileEntry()->getPath());
+  }
 }
 
-void BittorrentHelperTest::testLoadFromMemory_joinPathSingle()
+void BittorrentHelperTest::testLoadFromMemory_singleFileDirTraversal()
 {
   std::string memory =
     "d8:announce27:http://example.com/announce4:infod4:name14:../name1/name26:lengthi262144e12:piece lengthi262144e6:pieces20:00000000000000000000ee";
 
   SharedHandle<DownloadContext> dctx(new DownloadContext());
   dctx->setDir("/tmp");
-  loadFromMemory(memory, dctx, "default");
+  try {
-
+    loadFromMemory(memory, dctx, "default");
-  CPPUNIT_ASSERT_EQUAL(std::string("../name1/name2"), getName(dctx));
+  } catch(RecoverableException& e) {
-  CPPUNIT_ASSERT_EQUAL(std::string("/tmp/name1/name2"),
+    // success
-                       dctx->getFirstFileEntry()->getPath());
+  }
 }
 
 void BittorrentHelperTest::testGetNodes()

test/Makefile.am

@@ -250,4 +250,5 @@ EXTRA_DIST = 4096chunk.txt\
 	utf8.torrent\
 	metalink4.xml\
 	metalink4-attrs.xml\
-	metalink4-dirtraversal.xml
+	metalink4-dirtraversal.xml\
+	metalink3-dirtraversal.xml

test/Makefile.in

@@ -688,7 +688,8 @@ EXTRA_DIST = 4096chunk.txt\
 	utf8.torrent\
 	metalink4.xml\
 	metalink4-attrs.xml\
-	metalink4-dirtraversal.xml
+	metalink4-dirtraversal.xml\
+	metalink3-dirtraversal.xml
 
 all: all-am
 

test/MetalinkProcessorTest.cc

@@ -27,6 +27,7 @@ class MetalinkProcessorTest:public CppUnit::TestFixture {
   CPPUNIT_TEST(testParseFileV4_dirtraversal);
   CPPUNIT_TEST(testParseFileV4_attrs);
   CPPUNIT_TEST(testParseFile);
+  CPPUNIT_TEST(testParseFile_dirtraversal);
   CPPUNIT_TEST(testParseFromBinaryStream);
   CPPUNIT_TEST(testMalformedXML);
   CPPUNIT_TEST(testMalformedXML2);
@@ -52,6 +53,7 @@ public:
   void testParseFileV4_dirtraversal();
   void testParseFileV4_attrs();
   void testParseFile();
+  void testParseFile_dirtraversal();
   void testParseFromBinaryStream();
   void testMalformedXML();
   void testMalformedXML2();
@@ -271,6 +273,18 @@ void MetalinkProcessorTest::testParseFile()
   }
 }
 
+void MetalinkProcessorTest::testParseFile_dirtraversal()
+{
+  MetalinkProcessor proc;
+  SharedHandle<Metalinker> metalinker =
+    proc.parseFile("metalink3-dirtraversal.xml");
+  CPPUNIT_ASSERT_EQUAL((size_t)1, metalinker->entries.size());
+  SharedHandle<MetalinkEntry> e = metalinker->entries[0];
+  CPPUNIT_ASSERT_EQUAL(std::string("aria2-0.5.3.tar.bz2"), e->getPath());
+  CPPUNIT_ASSERT(!e->getSignature().isNull());
+  CPPUNIT_ASSERT_EQUAL(std::string(""), e->getSignature()->getFile());
+}
+
 void MetalinkProcessorTest::testParseFromBinaryStream()
 {
   MetalinkProcessor proc;

test/metalink3-dirtraversal.xml

@@ -0,0 +1,37 @@
+<?xml version="1.0" encoding="utf-8"?>
+<metalink version="3.0" xmlns="http://www.metalinker.org/">
+  <files>
+    <file name="../aria2-0.5.2.tar.bz2">
+      <verification>
+	<signature type="pgp" file="aria2-0.5.2.tar.bz2.sig">
+-----BEGIN PGP SIGNATURE-----
+Version: GnuPG v1.4.9 (GNU/Linux)
+
+ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
+ffffffffffffffffffffffff
+fffff
+-----END PGP SIGNATURE-----
+	</signature>
+      </verification>
+      <resources>
+        <url type="http">http://example.org/aria2-0.5.2.tar.bz2</url>
+      </resources>
+    </file>
+    <file name="aria2-0.5.3.tar.bz2">
+      <verification>
+	<signature type="pgp" file="../aria2-0.5.3.tar.bz2.sig">
+-----BEGIN PGP SIGNATURE-----
+Version: GnuPG v1.4.9 (GNU/Linux)
+
+ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
+ffffffffffffffffffffffff
+fffff
+-----END PGP SIGNATURE-----
+	</signature>
+      </verification>
+      <resources>
+        <url type="http">http://example.org/aria2-0.5.3.tar.bz2</url>
+      </resources>
+    </file>
+  </files>
+</metalink>

test/test.xml

@@ -28,7 +28,7 @@ fffff
         <url type="http" location="us" preference="100">http://httphost/aria2-0.5.2.tar.bz2</url>
       </resources>
     </file>
-    <file name="dir/../aria2-0.5.1.tar.bz2">
+    <file name="aria2-0.5.1.tar.bz2">
       <size>345689</size>
       <version>0.5.1</version>
       <language>ja-JP</language>