Merge pull request #21591 from owncloud/add-code-checking-for-apps

Verify signature of apps with level "Official" coming from the appstore
2016-01-13 10:35:00 +01:00 · 2016-01-13 10:35:00 +01:00 · 37e8a87d46
parent e0aa6e01ab c009d5dcc1
commit 37e8a87d46
7 changed files with 267 additions and 15 deletions
--- 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
--- 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);
--- 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) {
--- 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;
--- 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);
--- 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();
 	}
--- 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&amp;content=166053</fanspage>
+			   <knowledgebaseentries>0</knowledgebaseentries>
+			   <knowledgebasepage>http://apps.owncloud.com/content/show.php?action=knowledgebase&amp;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&amp;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))