Bug 138499

Summary: amarok magnatune unsafe shell
Product: amarok Reporter: Dirk Mueller <mueller>
Component: Internet ServicesAssignee: Amarok Developers <amarok-bugs-dist>
Status: RESOLVED FIXED    
Severity: normal CC: gauret, nhn
Priority: NOR    
Version: 2.3-GIT   
Target Milestone: ---   
Platform: Compiled Sources   
OS: Linux   
Latest Commit: Version Fixed In:

Description Dirk Mueller 2006-12-07 14:53:43 UTC
Version:            (using KDE KDE 3.5.5)
Installed from:    Compiled From Sources

magnatune downloader uses this code: 

    QString unzipString = "unzip \""+m_tempDir.name() + m_currentAlbumFileName + "\" -d \"" + m_currentAlbumUnpackLocation + "\" &";

    debug() << "unpacking: " << unzipString << endl;

    system( unzipString.ascii() );



this is unsafe for various reasons: 

- it should KProcess::quote them to avoid shell insertion attacks
- running it with "&" but then using system() doesn't make sense, especially as it emits downloadComplete(true) already, but around that time the unzip hasn't finished. 

unzip isn't checked for existance first, and unzip is not documented as a dependency.
Comment 1 Philippe Cloutier 2007-02-14 02:20:00 UTC
CVE-2006-6979 was assigned for this issue.
Comment 2 Mark Kretschmann 2007-02-14 19:23:29 UTC
SVN commit 633660 by markey:

Fix missing quoting for shell call.

BUG: 138499


 M  +1 -1      magnatunealbumdownloader.cpp  


--- branches/stable/extragear/multimedia/amarok/src/magnatunebrowser/magnatunealbumdownloader.cpp #633659:633660
@@ -89,7 +89,7 @@
 
     //ok, now we have the .zip file downloaded. All we need is to unpack it to the desired location and add it to the collection.
 
-    QString unzipString = "unzip \""+m_tempDir.name() + m_currentAlbumFileName + "\" -d \"" + m_currentAlbumUnpackLocation + "\" &";
+    QString unzipString = KProcess::quote( "unzip \""+m_tempDir.name() + m_currentAlbumFileName + "\" -d \"" + m_currentAlbumUnpackLocation + "\" &" );
 
     debug() << "unpacking: " << unzipString << endl;
 
Comment 3 Kees Cook 2007-02-14 19:54:06 UTC
Hi!  I haven't tested this, but I assumed that KProcess::quote was for arguments.  Shouldn't it be used like this:

QString unzipString = "unzip " + KProcess::quote(m_tempDir.name() + m_currentAlbumFileName) + " -d " + KProcess::quote(m_currentAlbumUnpackLocation);
Comment 4 Alexandre Oliveira 2007-02-14 20:53:56 UTC
SVN commit 633677 by aoliveira:

use KProcess::quote correctly, I believe
CCBUG: 138499


 M  +4 -4      magnatunealbumdownloader.cpp  


--- branches/stable/extragear/multimedia/amarok/src/magnatunebrowser/magnatunealbumdownloader.cpp #633676:633677
@@ -89,19 +89,19 @@
 
     //ok, now we have the .zip file downloaded. All we need is to unpack it to the desired location and add it to the collection.
 
-    QString unzipString = KProcess::quote( "unzip \""+m_tempDir.name() + m_currentAlbumFileName + "\" -d \"" + m_currentAlbumUnpackLocation + "\" &" );
+    QString unzipString = "unzip "+ KProcess::quote( m_tempDir.name() + m_currentAlbumFileName) + " -d " +KProcess::quote( m_currentAlbumUnpackLocation ) + " &";
 
     debug() << "unpacking: " << unzipString << endl;
 
     system( unzipString.ascii() );
 
-  
 
+
     if (m_currentAlbumId != -1 ) {
 
-        //now I really want to add the album cover to the same folder where I just unzipped the album... The 
+        //now I really want to add the album cover to the same folder where I just unzipped the album... The
         //only way of getting the actual location where the album was unpacked is using the artist and album names
-   
+
         MagnatuneAlbum album = MagnatuneDatabaseHandler::instance()->getAlbumById( m_currentAlbumId );
         MagnatuneArtist artist = MagnatuneDatabaseHandler::instance()->getArtistById( album.getArtistId() );
 
Comment 5 Alexandre Oliveira 2007-02-15 04:27:48 UTC
SVN commit 633728 by aoliveira:

Forward port
CCBUG: 138499


 M  +5 -5      magnatunealbumdownloader.cpp  


--- trunk/extragear/multimedia/amarok/src/magnatunebrowser/magnatunealbumdownloader.cpp #633727:633728
@@ -45,15 +45,15 @@
     KUrl downloadUrl = info->getCompleteDownloadUrl();
     m_currentAlbumUnpackLocation = info->getUnpackLocation();
     debug() << "Download: " << downloadUrl.url() << " to: " << m_currentAlbumUnpackLocation << endl;
-   
 
+
     m_currentAlbumFileName = downloadUrl.fileName( false );
 
-   
 
 
 
-    
+
+
     debug() << "Using temporary location: " << m_tempDir.name() + m_currentAlbumFileName << endl;
 
     m_albumDownloadJob = KIO::file_copy( downloadUrl, KUrl( m_tempDir.name() + m_currentAlbumFileName ), -1, true, false, false );
@@ -70,7 +70,7 @@
     KUrl downloadUrl( albumCoverUrlString );
 
     debug() << "Download Cover: " << downloadUrl.url() << " to: " << m_tempDir.name() << fileName << endl;
-   
+
     m_albumDownloadJob = KIO::file_copy( downloadUrl, KUrl( m_tempDir.name() + fileName ), -1, true, false, false );
 
     connect( m_albumDownloadJob, SIGNAL( result( KJob* ) ), SLOT( coverDownloadComplete( KJob* ) ) );
@@ -98,7 +98,7 @@
 
     //ok, now we have the .zip file downloaded. All we need is to unpack it to the desired location and add it to the collection.
 
-    QString unzipString = "unzip \""+m_tempDir.name() + m_currentAlbumFileName + "\" -d \"" + m_currentAlbumUnpackLocation + "\" &";
+    QString unzipString = "unzip " + KProcess::quote( m_tempDir.name() + m_currentAlbumFileName ) + " -d " + KProcess::quote( m_currentAlbumUnpackLocation ) + " &";
 
     debug() << "unpacking: " << unzipString << endl;
 
Comment 6 Dirk Mueller 2007-02-19 19:38:08 UTC
reopen because these issues are not addressed: 

running it with "&" but then using system() doesn't make sense, especially as it emits downloadComplete(true) already, but around that time the unzip hasn't finished. 
 
 unzip isn't checked for existance first, and unzip is not documented as a dependency.
Comment 7 Seb Ruiz 2008-09-10 07:06:42 UTC
Nikolaj, is this relevant for Amarok 2?
Comment 8 Myriam Schweingruber 2009-08-02 19:02:25 UTC
Nikolaj, I found this in a query, is this still relevant?
Comment 9 Maximilian Kossick 2009-09-25 23:05:56 UTC
yes, the magnatune service is still using unzip
Comment 10 Nikolaj Hald Nielsen 2009-09-30 09:55:29 UTC
fixed in commit 79a765281da1b26a50bf2f035de456efd338d0de