diff --git a/lib/private/app.php b/lib/private/app.php index 6c155136fe..b4856f1186 100644 --- a/lib/private/app.php +++ b/lib/private/app.php @@ -313,14 +313,14 @@ class OC_App { * @param string $app * @return int */ - public static function downloadApp($app) { + private static function downloadApp($app) { $ocsClient = new OCSClient( \OC::$server->getHTTPClientService(), \OC::$server->getConfig(), \OC::$server->getLogger() ); $appData = $ocsClient->getApplication($app, \OCP\Util::getVersion()); - $download= $ocsClient->getApplicationDownload($app, \OCP\Util::getVersion()); + $download = $ocsClient->getApplicationDownload($app, \OCP\Util::getVersion()); if(isset($download['downloadlink']) and $download['downloadlink']!='') { // Replace spaces in download link without encoding entire URL $download['downloadlink'] = str_replace(' ', '%20', $download['downloadlink']); @@ -856,7 +856,7 @@ class OC_App { * @param string $ocsID * @return string|false */ - protected static function getInternalAppIdByOcs($ocsID) { + public static function getInternalAppIdByOcs($ocsID) { if(is_numeric($ocsID)) { $idArray = \OC::$server->getAppConfig()->getValues(false, 'ocsid'); if(array_search($ocsID, $idArray)) { @@ -1042,7 +1042,7 @@ class OC_App { /** - * @param mixed $app + * @param string $app * @return bool * @throws Exception if app is not compatible with this version of ownCloud * @throws Exception if no app-name was specified diff --git a/lib/private/installer.php b/lib/private/installer.php index 46e022b477..ef5efb09b4 100644 --- a/lib/private/installer.php +++ b/lib/private/installer.php @@ -82,9 +82,10 @@ class OC_Installer{ $l = \OC::$server->getL10N('lib'); list($extractDir, $path) = self::downloadApp($data); - $info = self::checkAppsIntegrity($data, $extractDir, $path); - $basedir=OC_App::getInstallPath().'/'.$info['id']; + $info = self::checkAppsIntegrity($data, $extractDir, $path); + $appId = OC_App::cleanAppId($info['id']); + $basedir = OC_App::getInstallPath().'/'.$appId; //check if the destination directory already exists if(is_dir($basedir)) { OC_Helper::rmdirr($extractDir); @@ -163,6 +164,8 @@ class OC_Installer{ * @brief Update an application * @param array $info * @param bool $isShipped + * @throws Exception + * @return bool * * This function could work like described below, but currently it disables and then * enables the app again. This does result in an updated app. @@ -191,7 +194,7 @@ class OC_Installer{ * upgrade.php can determine the current installed version of the app using * "\OC::$server->getAppConfig()->getValue($appid, 'installed_version')" */ - public static function updateApp( $info=array(), $isShipped=false) { + public static function updateApp($info=array(), $isShipped=false) { list($extractDir, $path) = self::downloadApp($info); $info = self::checkAppsIntegrity($info, $extractDir, $path, $isShipped); @@ -307,11 +310,12 @@ class OC_Installer{ * check an app's integrity * @param array $data * @param string $extractDir + * @param string $path * @param bool $isShipped * @return array * @throws \Exception */ - public static function checkAppsIntegrity($data, $extractDir, $path, $isShipped=false) { + public static function checkAppsIntegrity($data, $extractDir, $path, $isShipped = false) { $l = \OC::$server->getL10N('lib'); //load the info.xml file of the app if(!is_file($extractDir.'/appinfo/info.xml')) { @@ -329,12 +333,41 @@ class OC_Installer{ } if(!is_file($extractDir.'/appinfo/info.xml')) { OC_Helper::rmdirr($extractDir); - if($data['source']=='http') { + if($data['source'] === 'http') { unlink($path); } throw new \Exception($l->t("App does not provide an info.xml file")); } - $info=OC_App::getAppInfo($extractDir.'/appinfo/info.xml', true); + + $info = OC_App::getAppInfo($extractDir.'/appinfo/info.xml', true); + + // We can't trust the parsed info.xml file as it may have been tampered + // with by an attacker and thus we need to use the local data to check + // whether the application needs to be signed. + $appId = OC_App::cleanAppId($data['appdata']['id']); + $appBelongingToId = OC_App::getInternalAppIdByOcs($appId); + if(is_string($appBelongingToId)) { + $previouslySigned = \OC::$server->getConfig()->getAppValue($appBelongingToId, 'signed', 'false'); + } else { + $appBelongingToId = $info['id']; + $previouslySigned = 'false'; + } + if($data['appdata']['level'] === OC_App::officialApp || $previouslySigned === 'true') { + \OC::$server->getConfig()->setAppValue($appBelongingToId, 'signed', 'true'); + $integrityResult = \OC::$server->getIntegrityCodeChecker()->verifyAppSignature( + $appBelongingToId, + $extractDir + ); + if($integrityResult !== []) { + $e = new \Exception( + $l->t( + 'Signature could not get checked. Please contact the app developer and check your admin screen.' + ) + ); + throw $e; + } + } + // check the code for not allowed calls if(!$isShipped && !OC_Installer::checkCode($extractDir)) { OC_Helper::rmdirr($extractDir); diff --git a/lib/private/integritycheck/checker.php b/lib/private/integritycheck/checker.php index 770b64c6b9..0cd01df7fe 100644 --- a/lib/private/integritycheck/checker.php +++ b/lib/private/integritycheck/checker.php @@ -352,6 +352,14 @@ class Checker { $this->cache->set(self::CACHE_KEY, json_encode($resultArray)); } + /** + * + * Clean previous results for a proper rescanning. Otherwise + */ + private function cleanResults() { + $this->config->deleteAppValue('core', self::CACHE_KEY); + $this->cache->remove(self::CACHE_KEY); + } /** * Verify the signature of $appId. Returns an array with the following content: @@ -382,11 +390,14 @@ class Checker { * Array may be empty in case no problems have been found. * * @param string $appId + * @param string $path Optional path. If none is given it will be guessed. * @return array */ - public function verifyAppSignature($appId) { + public function verifyAppSignature($appId, $path = '') { try { - $path = $this->appLocator->getAppPath($appId); + if($path === '') { + $path = $this->appLocator->getAppPath($appId); + } $result = $this->verify( $path . '/appinfo/signature.json', $path, @@ -460,6 +471,7 @@ class Checker { * and store the results. */ public function runInstanceVerification() { + $this->cleanResults(); $this->verifyCoreSignature(); $appIds = $this->appLocator->getAllApps(); foreach($appIds as $appId) { diff --git a/lib/private/ocsclient.php b/lib/private/ocsclient.php index 0cb4ad5a69..a783a1f842 100644 --- a/lib/private/ocsclient.php +++ b/lib/private/ocsclient.php @@ -284,7 +284,7 @@ class OCSClient { } $app = []; - $app['id'] = (int)$tmp->id; + $app['id'] = (int)$id; $app['name'] = (string)$tmp->name; $app['version'] = (string)$tmp->version; $app['type'] = (string)$tmp->typeid; diff --git a/tests/lib/installer.php b/tests/lib/installer.php index cfad4d7f0d..ca210ba31e 100644 --- a/tests/lib/installer.php +++ b/tests/lib/installer.php @@ -38,6 +38,10 @@ class Test_Installer extends \Test\TestCase { $data = array( 'path' => $tmp, 'source' => 'path', + 'appdata' => [ + 'id' => 'Bar', + 'level' => 100, + ] ); OC_Installer::installApp($data); @@ -57,6 +61,10 @@ class Test_Installer extends \Test\TestCase { $oldData = array( 'path' => $oldTmp, 'source' => 'path', + 'appdata' => [ + 'id' => 'Bar', + 'level' => 100, + ] ); $pathOfNewTestApp = __DIR__; @@ -69,6 +77,10 @@ class Test_Installer extends \Test\TestCase { $newData = array( 'path' => $newTmp, 'source' => 'path', + 'appdata' => [ + 'id' => 'Bar', + 'level' => 100, + ] ); OC_Installer::installApp($oldData); diff --git a/tests/lib/integritycheck/checkertest.php b/tests/lib/integritycheck/checkertest.php index ed7ca4bceb..b02849abb1 100644 --- a/tests/lib/integritycheck/checkertest.php +++ b/tests/lib/integritycheck/checkertest.php @@ -23,6 +23,7 @@ namespace Test\IntegrityCheck; use OC\IntegrityCheck\Checker; use OC\Memcache\NullCache; +use OCP\ICache; use phpseclib\Crypt\RSA; use phpseclib\File\X509; use Test\TestCase; @@ -293,6 +294,59 @@ class CheckerTest extends TestCase { $this->assertSame($expected, $this->checker->verifyAppSignature('SomeApp')); } + public function testVerifyAppSignatureWithTamperedFilesAndAlternatePath() { + $this->appLocator + ->expects($this->never()) + ->method('getAppPath') + ->with('SomeApp'); + $signatureDataFile = '{ + "hashes": { + "AnotherFile.txt": "1570ca9420e37629de4328f48c51da29840ddeaa03ae733da4bf1d854b8364f594aac560601270f9e1797ed4cd57c1aea87bf44cf4245295c94f2e935a2f0112", + "subfolder\/file.txt": "410738545fb623c0a5c8a71f561e48ea69e3ada0981a455e920a5ae9bf17c6831ae654df324f9328ff8453de179276ae51931cca0fa71fe8ccde6c083ca0574b" + }, + "signature": "dYoohBaWIFR\/To1FXEbMQB5apUhVYlEauBGSPo12nq84wxWkBx2EM3KDRgkB5Sub2tr0CgmAc2EVjPhKIEzAam26cyUb48bJziz1V6wvW7z4GZAfaJpzLkyHdSfV5117VSf5w1rDcAeZDXfGUaaNEJPWytaF4ZIxVge7f3NGshHy4odFVPADy\/u6c43BWvaOtJ4m3aJQbP6sxCO9dxwcm5yJJJR3n36jfh229sdWBxyl8BhwhH1e1DEv78\/aiL6ckKFPVNzx01R6yDFt3TgEMR97YZ\/R6lWiXG+dsJ305jNFlusLu518zBUvl7g5yjzGN778H29b2C8VLZKmi\/h1CH9jGdD72fCqCYdenD2uZKzb6dsUtXtvBmVcVT6BUGz41W1pkkEEB+YJpMrHILIxAiHRGv1+aZa9\/Oz8LWFd+BEUQjC2LJgojPnpzaG\/msw1nBkX16NNVDWWtJ25Bc\/r\/mG46rwjWB\/cmV6Lwt6KODiqlxgrC4lm9ALOCEWw+23OcYhLwNfQTYevXqHqsFfXOkhUnM8z5vDUb\/HBraB1DjFXN8iLK+1YewD4P495e+SRzrR79Oi3F8SEqRIzRLfN2rnW1BTms\/wYsz0p67cup1Slk1XlNmHwbWX25NVd2PPlLOvZRGoqcKFpIjC5few8THiZfyjiNFwt3RM0AFdZcXY=", + "certificate": "-----BEGIN CERTIFICATE-----\r\nMIIEvjCCAqagAwIBAgIUc\/0FxYrsgSs9rDxp03EJmbjN0NwwDQYJKoZIhvcNAQEF\r\nBQAwIzEhMB8GA1UECgwYb3duQ2xvdWQgQ29kZSBTaWduaW5nIENBMB4XDTE1MTEw\r\nMzIxMDMzM1oXDTE2MTEwMzIxMDMzM1owDzENMAsGA1UEAwwEY29yZTCCAiIwDQYJ\r\nKoZIhvcNAQEBBQADggIPADCCAgoCggIBALb6EgHpkAqZbO5vRO8XSh7G7XGWHw5s\r\niOf4RwPXR6SE9bWZEm\/b72SfWk\/\/J6AbrD8WiOzBuT\/ODy6k5T1arEdHO+Pux0W1\r\nMxYJJI4kH74KKgMpC0SB0Rt+8WrMqV1r3hhJ46df6Xr\/xolP3oD+eLbShPcblhdS\r\nVtkZEkoev8Sh6L2wDCeHDyPxzvj1w2dTdGVO9Kztn0xIlyfEBakqvBWtcxyi3Ln0\r\nklnxlMx3tPDUE4kqvpia9qNiB1AN2PV93eNr5\/2riAzIssMFSCarWCx0AKYb54+d\r\nxLpcYFyqPJ0ydBCkF78DD45RCZet6PNYkdzgbqlUWEGGomkuDoJbBg4wzgzO0D77\r\nH87KFhYW8tKFFvF1V3AHl\/sFQ9tDHaxM9Y0pZ2jPp\/ccdiqnmdkBxBDqsiRvHvVB\r\nCn6qpb4vWGFC7vHOBfYspmEL1zLlKXZv3ezMZEZw7O9ZvUP3VO\/wAtd2vUW8UFiq\r\ns2v1QnNLN6jNh51obcwmrBvWhJy9vQIdtIjQbDxqWTHh1zUSrw9wrlklCBZ\/zrM0\r\ni8nfCFwTxWRxp3H9KoECzO\/zS5R5KIS7s3\/wq\/w9T2Ie4rcecgXwDizwnn0C\/aKc\r\nbDIjujpL1s9HO05pcD\/V3wKcPZ1izymBkmMyIbL52iRVN5FTVHeZdXPpFuq+CTQJ\r\nQ238lC+A\/KOVAgMBAAEwDQYJKoZIhvcNAQEFBQADggIBAGoKTnh8RfJV4sQItVC2\r\nAvfJagkrIqZ3iiQTUBQGTKBsTnAqE1H7QgUSV9vSd+8rgvHkyZsRjmtyR1e3A6Ji\r\noNCXUbExC\/0iCPUqdHZIVb+Lc\/vWuv4ByFMybGPydgtLoEUX2ZrKFWmcgZFDUSRd\r\n9Uj26vtUhCC4bU4jgu6hIrR9IuxOBLQUxGTRZyAcXvj7obqRAEZwFAKQgFpfpqTb\r\nH+kjcbZSaAlLVSF7vBc1syyI8RGYbqpwvtREqJtl5IEIwe6huEqJ3zPnlP2th\/55\r\ncf3Fovj6JJgbb9XFxrdnsOsDOu\/tpnaRWlvv5ib4+SzG5wWFT5UUEo4Wg2STQiiX\r\nuVSRQxK1LE1yg84bs3NZk9FSQh4B8vZVuRr5FaJsZZkwlFlhRO\/\/+TJtXRbyNgsf\r\noMRZGi8DLGU2SGEAHcRH\/QZHq\/XDUWVzdxrSBYcy7GSpT7UDVzGv1rEJUrn5veP1\r\n0KmauAqtiIaYRm4f6YBsn0INcZxzIPZ0p8qFtVZBPeHhvQtvOt0iXI\/XUxEWOa2F\r\nK2EqhErgMK\/N07U1JJJay5tYZRtvkGq46oP\/5kQG8hYST0MDK6VihJoPpvCmAm4E\r\npEYKQ96x6A4EH9Y9mZlYozH\/eqmxPbTK8n89\/p7Ydun4rI+B2iiLnY8REWWy6+UQ\r\nV204fGUkJqW5CrKy3P3XvY9X\r\n-----END CERTIFICATE-----" +}'; + $this->fileAccessHelper + ->expects($this->at(0)) + ->method('file_get_contents') + ->with( + \OC::$SERVERROOT . '/tests/data/integritycheck/appWithInvalidData//appinfo/signature.json' + ) + ->will($this->returnValue($signatureDataFile)); + $this->fileAccessHelper + ->expects($this->at(1)) + ->method('file_get_contents') + ->with( + '/resources/codesigning/root.crt' + ) + ->will($this->returnValue(file_get_contents(__DIR__ .'/../../data/integritycheck/root.crt'))); + + + $expected = [ + 'INVALID_HASH' => [ + 'AnotherFile.txt' => [ + 'expected' => '1570ca9420e37629de4328f48c51da29840ddeaa03ae733da4bf1d854b8364f594aac560601270f9e1797ed4cd57c1aea87bf44cf4245295c94f2e935a2f0112', + 'current' => '7322348ba269c6d5522efe02f424fa3a0da319a7cd9c33142a5afe32a2d9af2da3a411f086fcfc96ff4301ea566f481dba0960c2abeef3594c4d930462f6584c', + ], + ], + 'FILE_MISSING' => [ + 'subfolder/file.txt' => [ + 'expected' => '410738545fb623c0a5c8a71f561e48ea69e3ada0981a455e920a5ae9bf17c6831ae654df324f9328ff8453de179276ae51931cca0fa71fe8ccde6c083ca0574b', + 'current' => '', + ], + ], + 'EXTRA_FILE' => [ + 'UnecessaryFile' => [ + 'expected' => '', + 'current' => 'cf83e1357eefb8bdf1542850d66d8007d620e4050b5715dc83f4a921d36ce9ce47d0d13c5d85f2b0ff8318d2877eec2f63b931bd47417a81a538327af927da3e', + ], + ], + + ]; + $this->assertSame($expected, $this->checker->verifyAppSignature('SomeApp', \OC::$SERVERROOT . '/tests/data/integritycheck/appWithInvalidData/')); + } + public function testVerifyAppWithDifferentScope() { $this->environmentHelper ->expects($this->once()) @@ -794,6 +848,10 @@ class CheckerTest extends TestCase { ->expects($this->at(3)) ->method('verifyAppSignature') ->with('dav'); + $this->config + ->expects($this->once()) + ->method('deleteAppValue') + ->with('core', 'oc.integritycheck.checker'); $this->checker->runInstanceVerification(); } diff --git a/tests/lib/ocsclienttest.php b/tests/lib/ocsclienttest.php index 80102eb62e..66e4c4b938 100644 --- a/tests/lib/ocsclienttest.php +++ b/tests/lib/ocsclienttest.php @@ -748,7 +748,7 @@ class OCSClientTest extends \Test\TestCase { ->expects($this->once()) ->method('get') ->with( - 'https://api.owncloud.com/v1/content/data/MyId', + 'https://api.owncloud.com/v1/content/data/166053', [ 'timeout' => 5, 'query' => ['version' => '8x1x0x7'], @@ -779,8 +779,145 @@ class OCSClientTest extends \Test\TestCase { 'score' => 50, 'level' => 200, ]; - $this->assertSame($expected, $this->ocsClient->getApplication('MyId', [8, 1, 0, 7])); + $this->assertSame($expected, $this->ocsClient->getApplication(166053, [8, 1, 0, 7])); } + + public function testGetApplicationSuccessfulWithOldId() { + $this->config + ->expects($this->at(0)) + ->method('getSystemValue') + ->with('appstoreenabled', true) + ->will($this->returnValue(true)); + $this->config + ->expects($this->at(1)) + ->method('getSystemValue') + ->with('appstoreurl', 'https://api.owncloud.com/v1') + ->will($this->returnValue('https://api.owncloud.com/v1')); + + $response = $this->getMock('\OCP\Http\Client\IResponse'); + $response + ->expects($this->once()) + ->method('getBody') + ->will($this->returnValue(' + + + ok + 100 + + + + + 1337 + Versioning + 0.0.1 + + 925 + ownCloud other + + owncloud + http://opendesktop.org/usermanager/search.php?username=owncloud + 2014-07-07T16:34:40+02:00 + 2014-07-07T16:34:40+02:00 + 140 + 50 + Placeholder for future updates + + + + + + + + + + + + + + + + + + + + + + + + 16 + AGPL + + 0 + http://apps.owncloud.com/content/show.php?content=166053 + 0 + http://apps.owncloud.com/content/show.php?action=fan&content=166053 + 0 + http://apps.owncloud.com/content/show.php?action=knowledgebase&content=166053 + ownCloud 7 + + + + + + + + + + https://apps.owncloud.com/content/show.php?content=166053 + + 0 + http://apps.owncloud.com/content/download.php?content=166053&id=1 + + + + + + 1 + 200 + + + + ')); + + $client = $this->getMock('\OCP\Http\Client\IClient'); + $client + ->expects($this->once()) + ->method('get') + ->with( + 'https://api.owncloud.com/v1/content/data/166053', + [ + 'timeout' => 5, + 'query' => ['version' => '8x1x0x7'], + ] + ) + ->will($this->returnValue($response)); + + $this->clientService + ->expects($this->once()) + ->method('newClient') + ->will($this->returnValue($client)); + + $expected = [ + 'id' => 166053, + 'name' => 'Versioning', + 'version' => '0.0.1', + 'type' => '925', + 'label' => 'recommended', + 'typename' => 'ownCloud other', + 'personid' => 'owncloud', + 'profilepage' => 'http://opendesktop.org/usermanager/search.php?username=owncloud', + 'detailpage' => 'https://apps.owncloud.com/content/show.php?content=166053', + 'preview1' => '', + 'preview2' => '', + 'preview3' => '', + 'changed' => 1404743680, + 'description' => 'Placeholder for future updates', + 'score' => 50, + 'level' => 200, + ]; + $this->assertSame($expected, $this->ocsClient->getApplication(166053, [8, 1, 0, 7])); + } + public function testGetApplicationEmptyXml() { $this->config ->expects($this->at(0))