From c009d5dcc1be69d280a71e01c5302f7fc3e5edc7 Mon Sep 17 00:00:00 2001 From: Lukas Reschke Date: Sat, 9 Jan 2016 22:15:50 +0100 Subject: [PATCH] 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. --- lib/private/app.php | 8 +- lib/private/installer.php | 45 +++++++- lib/private/integritycheck/checker.php | 16 ++- lib/private/ocsclient.php | 2 +- tests/lib/installer.php | 12 ++ tests/lib/integritycheck/checkertest.php | 58 ++++++++++ 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 500a60060e..74a89b8bc7 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 ed50503b3d..3072cfd3b2 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 edfe6b082e..baffa9f735 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 81c9abee05..84845896e9 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 17117a4274..c5e26d4823 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 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))