diff options
author | Lukas Reschke <lukas@owncloud.com> | 2016-01-09 22:15:50 +0100 |
---|---|---|
committer | Lukas Reschke <lukas@owncloud.com> | 2016-01-10 19:40:28 +0100 |
commit | c009d5dcc1be69d280a71e01c5302f7fc3e5edc7 (patch) | |
tree | 41cf627cb0c3a11b2450e3f0737bd360060b2a8f | |
parent | 656b5418996744b5ba095afac59b4fdb4db37337 (diff) | |
download | nextcloud-server-c009d5dcc1be69d280a71e01c5302f7fc3e5edc7.tar.gz nextcloud-server-c009d5dcc1be69d280a71e01c5302f7fc3e5edc7.zip |
Verify signature of apps with level "Official" coming from the appstore
This change will verify the signature of all apps with the level "Official" coming from the appstore or if they have been signed before.
-rw-r--r-- | lib/private/app.php | 8 | ||||
-rw-r--r-- | lib/private/installer.php | 45 | ||||
-rw-r--r-- | lib/private/integritycheck/checker.php | 16 | ||||
-rw-r--r-- | lib/private/ocsclient.php | 2 | ||||
-rw-r--r-- | tests/lib/installer.php | 12 | ||||
-rw-r--r-- | tests/lib/integritycheck/checkertest.php | 58 | ||||
-rw-r--r-- | tests/lib/ocsclienttest.php | 141 |
7 files changed, 267 insertions, 15 deletions
diff --git a/lib/private/app.php b/lib/private/app.php index 500a60060e6..74a89b8bc78 100644 --- a/lib/private/app.php +++ b/lib/private/app.php @@ -312,14 +312,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']); @@ -855,7 +855,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)) { @@ -1036,7 +1036,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 ed50503b3dc..3072cfd3b28 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 edfe6b082e7..baffa9f735b 100644 --- a/lib/private/integritycheck/checker.php +++ b/lib/private/integritycheck/checker.php @@ -320,6 +320,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: @@ -350,11 +358,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, @@ -428,6 +439,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 81c9abee058..84845896e90 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 cfad4d7f0de..ca210ba31ef 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 17117a4274b..c5e26d48237 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; @@ -253,6 +254,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->appLocator ->expects($this->once()) @@ -664,6 +718,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 80102eb62ee..66e4c4b938e 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('<?xml version="1.0"?> + <ocs> + <meta> + <status>ok</status> + <statuscode>100</statuscode> + <message></message> + </meta> + <data> + <content details="full"> + <id>1337</id> + <name>Versioning</name> + <version>0.0.1</version> + <label>recommended</label> + <typeid>925</typeid> + <typename>ownCloud other</typename> + <language></language> + <personid>owncloud</personid> + <profilepage>http://opendesktop.org/usermanager/search.php?username=owncloud</profilepage> + <created>2014-07-07T16:34:40+02:00</created> + <changed>2014-07-07T16:34:40+02:00</changed> + <downloads>140</downloads> + <score>50</score> + <description>Placeholder for future updates</description> + <summary></summary> + <feedbackurl></feedbackurl> + <changelog></changelog> + <homepage></homepage> + <homepagetype></homepagetype> + <homepage2></homepage2> + <homepagetype2></homepagetype2> + <homepage3></homepage3> + <homepagetype3></homepagetype3> + <homepage4></homepage4> + <homepagetype4></homepagetype4> + <homepage5></homepage5> + <homepagetype5></homepagetype5> + <homepage6></homepage6> + <homepagetype6></homepagetype6> + <homepage7></homepage7> + <homepagetype7></homepagetype7> + <homepage8></homepage8> + <homepagetype8></homepagetype8> + <homepage9></homepage9> + <homepagetype9></homepagetype9> + <homepage10></homepage10> + <homepagetype10></homepagetype10> + <licensetype>16</licensetype> + <license>AGPL</license> + <donationpage></donationpage> + <comments>0</comments> + <commentspage>http://apps.owncloud.com/content/show.php?content=166053</commentspage> + <fans>0</fans> + <fanspage>http://apps.owncloud.com/content/show.php?action=fan&content=166053</fanspage> + <knowledgebaseentries>0</knowledgebaseentries> + <knowledgebasepage>http://apps.owncloud.com/content/show.php?action=knowledgebase&content=166053</knowledgebasepage> + <depend>ownCloud 7</depend> + <preview1></preview1> + <preview2></preview2> + <preview3></preview3> + <previewpic1></previewpic1> + <previewpic2></previewpic2> + <previewpic3></previewpic3> + <picsmall1></picsmall1> + <picsmall2></picsmall2> + <picsmall3></picsmall3> + <detailpage>https://apps.owncloud.com/content/show.php?content=166053</detailpage> + <downloadtype1></downloadtype1> + <downloadprice1>0</downloadprice1> + <downloadlink1>http://apps.owncloud.com/content/download.php?content=166053&id=1</downloadlink1> + <downloadname1></downloadname1> + <downloadgpgfingerprint1></downloadgpgfingerprint1> + <downloadgpgsignature1></downloadgpgsignature1> + <downloadpackagename1></downloadpackagename1> + <downloadrepository1></downloadrepository1> + <downloadsize1>1</downloadsize1> + <approved>200</approved> + </content> + </data> + </ocs> + ')); + + $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)) |