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.
CVE-2006-6979 was assigned for this issue.
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;
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);
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() );
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;
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.
Nikolaj, is this relevant for Amarok 2?
Nikolaj, I found this in a query, is this still relevant?
yes, the magnatune service is still using unzip
fixed in commit 79a765281da1b26a50bf2f035de456efd338d0de