From ca50adb2e5efa88e6ffea6a5cff84939e477bf22 Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Thu, 14 Dec 2017 11:06:22 +0100 Subject: [PATCH 1/8] update: switch tests to using standardized setUp() method --- tests/update.TestCase | 36 ++++++++++++++++-------------------- 1 file changed, 16 insertions(+), 20 deletions(-) diff --git a/tests/update.TestCase b/tests/update.TestCase index da573714..e49e1bf0 100755 --- a/tests/update.TestCase +++ b/tests/update.TestCase @@ -32,6 +32,14 @@ from fdroidserver.common import FDroidPopen class UpdateTest(unittest.TestCase): '''fdroid update''' + def setUp(self): + logging.basicConfig(level=logging.INFO) + self.basedir = os.path.join(localmodule, 'tests') + self.tmpdir = os.path.abspath(os.path.join(self.basedir, '..', '.testfiles')) + if not os.path.exists(self.tmpdir): + os.makedirs(self.tmpdir) + os.chdir(self.basedir) + def testInsertStoreMetadata(self): config = dict() fdroidserver.common.fill_config_defaults(config) @@ -117,15 +125,13 @@ class UpdateTest(unittest.TestCase): self.assertEqual('Conversations', app['localized']['en-US']['name']) def test_insert_triple_t_metadata(self): - importer = os.path.join(localmodule, 'tests', 'tmp', 'importer') + importer = os.path.join(self.basedir, 'tmp', 'importer') packageName = 'org.fdroid.ci.test.app' if not os.path.isdir(importer): logging.warning('skipping test_insert_triple_t_metadata, import.TestCase must run first!') return - tmpdir = os.path.join(localmodule, '.testfiles') - if not os.path.exists(tmpdir): - os.makedirs(tmpdir) - tmptestsdir = tempfile.mkdtemp(prefix=inspect.currentframe().f_code.co_name, dir=tmpdir) + tmptestsdir = tempfile.mkdtemp(prefix=inspect.currentframe().f_code.co_name, + dir=self.tmpdir) packageDir = os.path.join(tmptestsdir, 'build', packageName) shutil.copytree(importer, packageDir) @@ -387,10 +393,6 @@ class UpdateTest(unittest.TestCase): self.assertEqual(apk, frompickle) def test_process_apk_signed_by_disabled_algorithms(self): - os.chdir(os.path.join(localmodule, 'tests')) - if os.path.basename(os.getcwd()) != 'tests': - raise Exception('This test must be run in the "tests/" subdir') - config = dict() fdroidserver.common.fill_config_defaults(config) fdroidserver.update.config = config @@ -408,12 +410,9 @@ class UpdateTest(unittest.TestCase): fdroidserver.update.options.allow_disabled_algorithms = False knownapks = fdroidserver.common.KnownApks() - apksourcedir = os.getcwd() - tmpdir = os.path.join(localmodule, '.testfiles') - if not os.path.exists(tmpdir): - os.makedirs(tmpdir) + tmptestsdir = tempfile.mkdtemp(prefix=inspect.currentframe().f_code.co_name, - dir=tmpdir) + dir=self.tmpdir) print('tmptestsdir', tmptestsdir) os.chdir(tmptestsdir) os.mkdir('repo') @@ -424,7 +423,7 @@ class UpdateTest(unittest.TestCase): disabledsigs = ['org.bitbucket.tickytacky.mirrormirror_2.apk', ] for apkName in disabledsigs: - shutil.copy(os.path.join(apksourcedir, apkName), + shutil.copy(os.path.join(self.basedir, apkName), os.path.join(tmptestsdir, 'repo')) skip, apk, cachechanged = fdroidserver.update.process_apk({}, apkName, 'repo', @@ -475,7 +474,7 @@ class UpdateTest(unittest.TestCase): badsigs = ['urzip-badcert.apk', 'urzip-badsig.apk', 'urzip-release-unsigned.apk', ] for apkName in badsigs: - shutil.copy(os.path.join(apksourcedir, apkName), + shutil.copy(os.path.join(self.basedir, apkName), os.path.join(tmptestsdir, 'repo')) skip, apk, cachechanged = fdroidserver.update.process_apk({}, apkName, 'repo', @@ -539,11 +538,8 @@ class UpdateTest(unittest.TestCase): self.assertTrue(foundtest) def test_create_metadata_from_template(self): - tmpdir = os.path.join(localmodule, '.testfiles') - if not os.path.exists(tmpdir): - os.makedirs(tmpdir) tmptestsdir = tempfile.mkdtemp(prefix=inspect.currentframe().f_code.co_name, - dir=tmpdir) + dir=self.tmpdir) print('tmptestsdir', tmptestsdir) os.chdir(tmptestsdir) os.mkdir('repo') From 5ce950e748fc064fe27d92eb81c0456b6a7b9d1b Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Mon, 11 Dec 2017 17:56:04 +0100 Subject: [PATCH 2/8] update: print warnings for all KnownVulns found Some baby steps towards making the KnownVuln stuff more visible. --- fdroidserver/update.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/fdroidserver/update.py b/fdroidserver/update.py index 02382d2b..e548df43 100644 --- a/fdroidserver/update.py +++ b/fdroidserver/update.py @@ -500,6 +500,8 @@ def has_known_vulnerability(filename): http://www.saurik.com/id/17 """ + found_vuln = False + # statically load this pattern if not hasattr(has_known_vulnerability, "pattern"): has_known_vulnerability.pattern = re.compile(b'.*OpenSSL ([01][0-9a-z.-]+)') @@ -524,14 +526,15 @@ def has_known_vulnerability(filename): else: logging.warning(_('"{path}" contains outdated {name} ({version})') .format(path=filename, name=name, version=version)) - return True + found_vuln = True break elif name == 'AndroidManifest.xml' or name == 'classes.dex' or name.endswith('.so'): if name in files_in_apk: - return True + logging.warning(_('{apkfilename} has multiple {name} files, looks like Master Key exploit!') + .format(apkfilename=filename, name=name)) + found_vuln = True files_in_apk.add(name) - - return False + return found_vuln def insert_obbs(repodir, apps, apks): From bde0558d82eb68c39d6c95eb80ed9c2eddea6ae6 Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Mon, 11 Dec 2017 18:36:21 +0100 Subject: [PATCH 3/8] update: reject APKs with invalid file sig, probably Janus exploits This just checks the first four bytes of the APK file, aka the "file signature", to make sure it is the ZIP signature and not the DEX signature. This was checked against the test APK, and I ran it against some known malware and all of f-droid.org to make sure it works. All valid ZIP files (therefore APK files) should start with the ZIP Local File Header of four bytes. https://www.guardsquare.com/en/blog/new-android-vulnerability-allows-attackers-modify-apps-without-affecting-their-signatures --- fdroidserver/update.py | 11 ++++++++++- tests/janus.apk | Bin 0 -> 10067 bytes tests/run-tests | 2 +- tests/update.TestCase | 29 +++++++++++++++++++++++++++++ 4 files changed, 40 insertions(+), 2 deletions(-) create mode 100644 tests/janus.apk diff --git a/fdroidserver/update.py b/fdroidserver/update.py index e548df43..f90e4993 100644 --- a/fdroidserver/update.py +++ b/fdroidserver/update.py @@ -496,8 +496,10 @@ def has_known_vulnerability(filename): Checks whether there are more than one classes.dex or AndroidManifest.xml files, which is invalid and an essential part of the "Master Key" attack. - http://www.saurik.com/id/17 + + Janus is similar to Master Key but is perhaps easier to scan for. + https://www.guardsquare.com/en/blog/new-android-vulnerability-allows-attackers-modify-apps-without-affecting-their-signatures """ found_vuln = False @@ -506,6 +508,13 @@ def has_known_vulnerability(filename): if not hasattr(has_known_vulnerability, "pattern"): has_known_vulnerability.pattern = re.compile(b'.*OpenSSL ([01][0-9a-z.-]+)') + with open(filename.encode(), 'rb') as fp: + first4 = fp.read(4) + if first4 != b'\x50\x4b\x03\x04': + raise FDroidException(_('{path} has bad file signature "{pattern}", possible Janus exploit!') + .format(path=filename, pattern=first4.decode().replace('\n', ' ')) + '\n' + + 'https://www.guardsquare.com/en/blog/new-android-vulnerability-allows-attackers-modify-apps-without-affecting-their-signatures') + files_in_apk = set() with zipfile.ZipFile(filename) as zf: for name in zf.namelist(): diff --git a/tests/janus.apk b/tests/janus.apk new file mode 100644 index 0000000000000000000000000000000000000000..ed4eed96c01647047e6420b778e885236b833c67 GIT binary patch literal 10067 zcmds7cRZHe|G#c~hsY>fX1PUVB-wkD@yNVy5pLtQvMEAYS(%~8C>hx+g~*odogyPE zB+2hwo_wF4e$VUo`@LSzf4{@~I-m17=QGYZpL4G3I@gUtdyxnVi$LW&@-nT&;u(de zoqU4|IwHqOt=HOGwQKaaA;=kmAg?RusX=y_oN*zD7Emys7@!$|0AK;&C=mfd5CK8p zA94m4FB}Mp0XdBO0C@mS0L=j706PF=pbZn zb!#U_B-+c;(b)lw{O=A*LiFmek4Ohg%r&H(t);6T+Ql7>u?DsfS0J1#Dgf#bkrPJF z6>aH;hN!WY9%z&b2J2=C8tJ*gJPIt@?a*z^p;d?kl$Bg9;Q@dM0S&tY;knt`VIe%& zZ7_yMV+-f-_`*3nyl@W337ivx*`R=E5tI~{OmGpj1STfZfDivc&^I6iw{wOk479=n zT3|gtz~uzUomdf&5X6qahn!h)AU22*NMTsz}glnnD0y;dXFL;{rac z3+@NvCrt(`1T-vb0n?y8Ov7s#PTFd?c<@>wg4zjR`9&^X;tK*b&46_raAp3CFAYnK zoedi6CgA1hkgngMNh3oeC3TFxmBGfV&6?g!*6Mt3aG&kCNSLbr`M}vIGN+fX+STRU za$G*VK|0T*R5`B%hpZpJ>$1|Hpyuhl=pmx_Mg8^Q=d_~5w>udU2JW@9$}?ETV(sSJ zoPrUfTaycPVf@V(U6kwCCn=r6IcATqhtQV=5?e)xaYlt%&z@XQ|0?sHUFh5ku8Oc| zc`k}4l&fLSnzh(tAq+LYe25r(RJgEPaTH0xaI4<3bxMzj(3bf}?Uxz8?WGf3WiQ&8 zE?%O=pPLaX4c6GeD60o+nV*&MFxXUCzGu$X$nPXjF}yR|KWmRqs6QE>cXcLpWNN;W zM5JN_hc5ZUhQNCQf%PKYskgi(PK2|L41Uorr)`#tp9)s+tv+^VdzjGvOsI%c7|*+n zXGlPl-Z-nDMhX-62We+*MMWR>dSM3j!QR}3<_G*fb2lp6FUGZ%nk9&FwQCmBy+WHN zysJ~R(ajv7$jfqTB`#OG+&%qfR(1vZZn(0crre(9nss6W-MLpJ58h*Aljk{XBusPc zKAu@KYdx!zV!6u99OIcM(lI$6!0+v!n0GF^*4KPGmBTEEl!`X#z9heRM{`_UvV_|w zUOsoiN`-Ri?8WDE{2{liII#L7^#?J2=DuRObOnh9XY&Ho-X%BQj0%(!{Vn zig5U*9WX1#F@Xc*;qs*1D>aIbL%i!Yzl%X$<1~-Q{1&O<`P}z!JpXmIYGC9xu zr263Q6fg4nTvnqkzOX~lvztl-sUOE}=I{v2OnC3+FGd+(iEX1qvr}|LP@QPfn|PBy z_-1p}Ab@r))$mT5x-a*^==8e;>&7@w-3Lr|69rDZ%<+jI^~bn`2*S6nHYP7-Uu;D>0#X;N1UEAnr@)GJ zMPrdDS4&SzE3h@&S(`gpx?`+u(XIl{m}}wMno8s(jPP9-5tDcm!B0GtnGZ68CU4;rp5hapViL#opJ28 z+0xDui~R#(PqX8{M<37)Z4G!xtaV z$Ois=vx@wcsVx)NYvu8$6WZ@HSkc=MBtA==VmH5wx*wj$Ucg=;rWgAtjJH}Uyu+~A zH~o(i)IDme_Ff{!*u2*Gm+?2}Z_E2+fNlpTO?{kC#+XL%QeJ5NJK4mSVzzVT%-!&ZT{ieNs8ajBNsFg(i_yz)6 z8;)v6njh%C$7+e*44=D3C)V7;N04`4C9tsI*fB?X+we}+So_@O&-P~y`h~wQuU3ST zs`J)AjP**VeMQNx9X4{|wtA8M)l1YqYA?83)BUbFd@9#!W}2GpR(9I;Vv<4NY@?Ld zsYMJw(RO?xqPF12_+Gc^7&6nRV?0GNw8Rj6vFU+piweufAmW~HAM#0GI3zG~gdh)o zRDXOMu%*7Y6HJ%Qz-@HyvV8SWk;xC@hRQ#xymQ)f{SxZzj4e}h3!c?|;MNOZ)~dR} zqD}L{dMLM0Ygf|{r*rw~>h7oJR;8V0YHo7PwONa$MYh2eI}XHj)O;?+=Sz8i-NV?Z zE9+g5oWv0BDBZ&N=^b14t@<!!Z1iQ1*{ED=#KyerR-g9(YIyuzjlYSx;0p z2v0bRskc47)FYmd@G1yRuH+mnt%y0#(5e6C!h@6Z8FabB_G_tj_$(4UT_*$k9a=(8 zdYIB%sV3EnnFYt~hSzfGzMBo*Z2daQl16$tnFiU<)Kij8WoE%CcFAWZ-(hTQc744~ z{PBs4QpP0@@x`PAge{8w3QSo1rF5>!=TM@i+vzw9KAt~j?k>tAX3d~Oj_h6gTt$Bt z5gKe^&eJy{tULZ%DILA3rXB_Ibhc;77QKX$_SRXAWFG z7}vRs^kxgAg{_CwL%3ShxW9+$1;2OX_VE|7D=$n!yudR^#8X=og7(Ap`Nn)de%HD5 z{PWzTY_><|!`4TNGt-lgJ%#3&uBS-dmC;4Jzj7JWw!&g9>hDdX#4^K8_bxsoF0`rM zv!dJ%IYl z>-_`giU5@+BgO46sRQ?I6Tg4mCB8*)Vx@|!8xQ@yyUj=IB%2BM_Ck#CS~rib0@}|j zh*cmQ?=4<|RAis2~-n(P5LFfj8P`1GFg?_)WrG8B_0Uz z%CpOXV6t65gy8d)p$;+uA8B#{@6FrmK5 zaF4oK2D%)4y0E44*m3=pF8$~JHYE;$b2N%WM1zWbYS+%xBVvk379fFF1lmE5#A&1^ zA?0l`W1KnYn^ockC6&YpS#x4RVqRm}xK+CkXSWg|*9bE4AAGFtW~{XrzY8OMj!;ZwSG&W7kik=`pG~(U^2&3(8~`aR_M%eGR()ZU{U(`%)4(Og zbn-G&z{(DeALJaCR~ozOfgfq?e-6&~oyA$ixB-1n%gKy;UPpgOj?;BSXR0@7n5L@?JL)?2_znnz>GLDvgy$xQv%u)o1&$t@_93 zs;aNAGiRCTR;;F=hfv3*aM)`0?gTWCbsOE=Ek9lN>16@4{!-YNC$t$_0dr^L(T}Ju z;+(IWPU|I#()gowsokze+#QiuV1oZGGeo@Mptp>siG>{>Qkc2%_z51M;_LXPF*n|9P~)#7chQhC>_8n7b2*SGH~!3!nbs&YI+szB%}MrcyU0h zG_F1MS=~|#ov2pY+nrlsyxC{Jgr};gtlt(VJ6E~?!STJqmMd-?vl1e4Br8ZbIn!vD2QxPZ{|; zC_CbX!ebG$YcIAc9j2EmOeU)=sPa=4o z%2WHUE8XLJ74lfsz07ehF+aKYZf5V`HreX|vkF9m5dt-%M%XRN+G<97^)|@jCXf^I+yjh7v{)=@7`KDkgDA@mlhjhcJJ6gKfoT(FvnTFSi;#vmPRAA z?knQovm2Q(B=Tm$GwBp2ILl>U`xN63DrdX&4{pyk+m+UuM!vR|w^UA+iA;>zOm*Ep zaG-c`c31E5TXO@3`gI?ICG!I)aG{9BWT_!q$Rp1BBg+?yBwt&G`o6by93dqqr5&$V z=Ai5;b)TO2;b|nY_ z;3x;Y@Hx4=TBET7mabT9aQ;JfSOuT)Aix`;7$D&Dp`#>tM1cGvfEPdoz%;-q$Q43E zSO^KB0A&ez0$4!~fQN*xLGGaB3Tm*B%TF2&34pU;45<5`W$hqq$QwsUT(0#sT~}L52XXfI>m6kRH(P2wK8DJxn;Ph)(cM8~_T|pq(SIh6eV2 zYxQT_|L0d$5I^v_dL78&g9A9hag_#gn17VSjtG#$XPvA7d;llECexteqj!K4o55s#Osli zIp`xC<8Z{=fxg%s#w{G{aE~oPEgat%z=g-%`LO?vfbR;yBSiKa2VN&|ojs@{|8I4$ z_G@4j!n%k+yFXVJ22hTND}V@+IppaduGBwyI5?Dt-!Z`O945F-4=DJ&8OBjU;HV$! zJ>u(wieF>nsNMfH2glWiXW(#qu;U$`TEl1Jupe+4&S76@Kn_TeC|C7c6w8&2y%u2w~!^5B7cL)VdI27UV1-*yAj#@id zVzJ=S0fqKL$JII=m!WLEwu*j_*CAGVv(Ltcj*nsF;M0YftaVCDp!~ z9FC%-rebC5dnAAUQFaJ3g8Gqai2TjGJZ)P_4W7KO+aBqs-l#ODkAT!OnI0IehMh1U?+=X@iACNQyeu5qQNHIVnjwz(zJ*dX+;x7;3?iXho=uS zdy+;~EmZXeMsv##7<{6yAJ}&vWNdn3e>lnQGw+^DGwJR=Us@g6R?<1D|4xFZjISr? zWki&)M4q*6Eo$+5hvlh=8!2YfI`LHxAF<$7^N;f{7-M?d_q~we_|YLl@g8Rw&zCjElhxVVyHKwCI@52&4mHIfg-C9q1?p0H zG84x=kj7hIhs`xOJ~N#d&OGjz-*@M`*NA=5*L?+lr^we*X)O{cHYjoWAlBVXW)%*?%B_|7rfy97n_j zom|Gk`1x9{Lh*^uw9Up9+po26rS4Mqc==jd!m(L zJYYFxq%QCRt5f1^$E;#RjFoSQ%@CL1+pf7V)MN<)}A)~3H zsGz4WprL5ct>Y9dNE15az;QG2MRX7aiX?+U|9)jMaiHUcXd)6zKM%b8YdVdiz&-np(O29lYuj6@rRn-qWRym)$~3 z*vY^0P^z~_Gn-Uw)*YkZud+#a%n{bSA_L{shHAA zS@OoWi~r1(nvE&5u9KD+v%Ivo7wVe5Wh7iR-Wrv;=qxbkBoJD9)z$O9!>0NraVh5a z$iEMkJD$6e6d0%;0C>0k&tSNoSG z+T22LyMPR!sS?$r7G1N4^>wog|A7#^B69t zY2U`c5>B;JGHN$Pv|j<`jO;XhgzXI3Ie{Y9rcZtr?2Ku8z7FD667=Fx<)4#pQ1Xqb zV=1!iCKt$7pGAu9UdXz+Gn*|I`Elh=PXanGmBCI_#wM|L*OHyc-iVEfo-<2C^GD2p z!3V4c;VazSv7FQ>^4DgMLTi_lP;nXcMfB3_G$sDBGx*GE1FzQ&iT|LU4^!p7S3g)| zKwD#$j3gDE+sN6xaN(h0W#qG(p7NTUb5_)k&iHTnpF_H)u+)BNUsjC8=NL1%+M@Hg z>~^$T+iSXkJ1doqrCDBBu-bK9yPBXhv1eIXSwTS|Sy?5?`Ihlw&%(2Ua!L^KY7I@1 zo3RsZZEs$T4|iDf4z~{sx2<_azwUhCZHQj{l764&Sio1;ntD6Sidd|b&f8Y}#m4=& zYPhEjlJeA&Cw2QI{g;~$-Y3O3Hrs@ykJknspltO5KJ2;~pF}fPTXk;~Zpm!CtwZQOAsyXV2VPd)#AsIKCxEzf-1!zGF-zX8ua8^7ZByG5R-6+~8RB+Z4- z2@45kxF{`VQOP6F7sZ_ z!dtMITXH(<6E3fKmLjg2^-Y=aMO@2}_0JtBk9cOe7kl0df8i2D7kzYG3;qxehEkoA}4`O9tNcMZSqo_}e$#rBJapL^-=(!XyA fe@U;h|1J%-2W>Tc_&F6k*bzdFV7vA|4VV54kn;P< literal 0 HcmV?d00001 diff --git a/tests/run-tests b/tests/run-tests index 696bcd75..af29f471 100755 --- a/tests/run-tests +++ b/tests/run-tests @@ -9,7 +9,7 @@ echo_header() { copy_apks_into_repo() { set +x find $APKDIR -type f -name '*.apk' -print0 | while IFS= read -r -d '' f; do - echo $f | grep -F -v -e unaligned -e unsigned -e badsig -e badcert -e bad-unicode || continue + echo $f | grep -F -v -e unaligned -e unsigned -e badsig -e badcert -e bad-unicode -e janus.apk || continue apk=`$aapt dump badging "$f" | sed -n "s,^package: name='\(.*\)' versionCode='\([0-9][0-9]*\)' .*,\1_\2.apk,p"` test "$f" -nt repo/$apk && rm -f repo/$apk # delete existing if $f is newer if [ ! -e repo/$apk ] && [ ! -e archive/$apk ]; then diff --git a/tests/update.TestCase b/tests/update.TestCase index e49e1bf0..db463a89 100755 --- a/tests/update.TestCase +++ b/tests/update.TestCase @@ -601,6 +601,35 @@ class UpdateTest(unittest.TestCase): self.assertEqual('urzip', data['Name']) self.assertEqual('urzip', data['Summary']) + def test_has_known_vulnerability(self): + good = [ + 'org.bitbucket.tickytacky.mirrormirror_1.apk', + 'org.bitbucket.tickytacky.mirrormirror_2.apk', + 'org.bitbucket.tickytacky.mirrormirror_3.apk', + 'org.bitbucket.tickytacky.mirrormirror_4.apk', + 'org.dyndns.fules.ck_20.apk', + 'urzip.apk', + 'urzip-badcert.apk', + 'urzip-badsig.apk', + 'urzip-release.apk', + 'urzip-release-unsigned.apk', + 'repo/com.politedroid_3.apk', + 'repo/com.politedroid_4.apk', + 'repo/com.politedroid_5.apk', + 'repo/com.politedroid_6.apk', + 'repo/obb.main.oldversion_1444412523.apk', + 'repo/obb.mainpatch.current_1619_another-release-key.apk', + 'repo/obb.mainpatch.current_1619.apk', + 'repo/obb.main.twoversions_1101613.apk', + 'repo/obb.main.twoversions_1101615.apk', + 'repo/obb.main.twoversions_1101617.apk', + 'repo/urzip-; Рахма́нинов, [rɐxˈmanʲɪnəf] سيرجي_رخمانينوف 谢尔盖·.apk', + ] + for f in good: + self.assertFalse(fdroidserver.update.has_known_vulnerability(f)) + with self.assertRaises(fdroidserver.exception.FDroidException): + fdroidserver.update.has_known_vulnerability('janus.apk') + if __name__ == "__main__": parser = optparse.OptionParser() From 67b9514c5a830159e6cc39a6ba180cb2774635b9 Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Wed, 13 Dec 2017 11:57:36 +0100 Subject: [PATCH 4/8] update: strip EXIF data from all JPEGs EXIF data can be abused to exploit systems a lot easier than the JPEG image data can. The F-Droid ecosystem does not use the EXIF data, so keep things safe and strip it all away. There is a chance that some images might rely on the rotation to be set by EXIF, but I think having a safe system is more important. If needed, only the rotation data could be saved. But that then makes it hard to tell which images have been stripped. This way, if there is no EXIF, it has been stripped. And if there is EXIF data, then it is suspect. https://securityaffairs.co/wordpress/51043/mobile-2/android-cve-2016-3862-flaw.html https://threatpost.com/google-shuts-down-potentially-massive-android-bug/120393/ https://blog.sucuri.net/2013/07/malware-hidden-inside-jpg-exif-headers.html The big downside of this is that it decompresses and recompresses the image data. That should be replaced by a technique from jhead, exiftool, ObscuraCam, etc. that only strips the metadata. --- fdroidserver/update.py | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/fdroidserver/update.py b/fdroidserver/update.py index f90e4993..b6a33f77 100644 --- a/fdroidserver/update.py +++ b/fdroidserver/update.py @@ -672,6 +672,27 @@ def _set_author_entry(app, key, f): app[key] = text +def _strip_and_copy_image(inpath, outpath): + """Remove any metadata from image and copy it to new path + + Sadly, image metadata like EXIF can be used to exploit devices. + It is not used at all in the F-Droid ecosystem, so its much safer + just to remove it entirely. PNG does not have the same kind of + issues. + + """ + + if common.has_extension(inpath, 'png'): + shutil.copy(inpath, outpath) + else: + with open(inpath) as fp: + in_image = Image.open(fp) + data = list(in_image.getdata()) + out_image = Image.new(in_image.mode, in_image.size) + out_image.putdata(data) + out_image.save(outpath, "JPEG", optimize=True) + + def copy_triple_t_store_metadata(apps): """Include store metadata from the app's source repo @@ -744,7 +765,7 @@ def copy_triple_t_store_metadata(apps): sourcefile = os.path.join(root, f) destfile = os.path.join(destdir, os.path.basename(f)) logging.debug('copying ' + sourcefile + ' ' + destfile) - shutil.copy(sourcefile, destfile) + _strip_and_copy_image(sourcefile, destfile) def insert_localized_app_metadata(apps): @@ -842,7 +863,7 @@ def insert_localized_app_metadata(apps): if base in GRAPHIC_NAMES and extension in ALLOWED_EXTENSIONS: os.makedirs(destdir, mode=0o755, exist_ok=True) logging.debug('copying ' + os.path.join(root, f) + ' ' + destdir) - shutil.copy(os.path.join(root, f), destdir) + _strip_and_copy_image(os.path.join(root, f), destdir) for d in dirs: if d in SCREENSHOT_DIRS: if locale == 'images': @@ -854,7 +875,7 @@ def insert_localized_app_metadata(apps): screenshotdestdir = os.path.join(destdir, d) os.makedirs(screenshotdestdir, mode=0o755, exist_ok=True) logging.debug('copying ' + f + ' ' + screenshotdestdir) - shutil.copy(f, screenshotdestdir) + _strip_and_copy_image(f, screenshotdestdir) repofiles = sorted(glob.glob(os.path.join('repo', '[A-Za-z]*', '[a-z][a-z][A-Z-.@]*'))) for d in repofiles: From 387eebc4d69e0c89fd27a0bbe20ab31a5ea4be20 Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Wed, 13 Dec 2017 11:51:34 +0100 Subject: [PATCH 5/8] update: strip all metadata from PNGs This strips metadata and optimizes the compression of all PNGs copied from the app's source repo as well as all the icons extracted from the APKs. There have been exploits delivered via image metadata, and F-Droid isn't using it all, so its best to just remove it. This unfortunately uncompresses and recompresses the files. Luckily, that's a lossless procedure with PNGs, and we might end up with smaller files. The only tool I could find that strips without changing the image data is exiftool, but that is written in Perl. --- fdroidserver/update.py | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/fdroidserver/update.py b/fdroidserver/update.py index b6a33f77..4611b127 100644 --- a/fdroidserver/update.py +++ b/fdroidserver/update.py @@ -35,7 +35,7 @@ from argparse import ArgumentParser import collections from binascii import hexlify -from PIL import Image +from PIL import Image, PngImagePlugin import logging from . import _ @@ -84,6 +84,8 @@ GRAPHIC_NAMES = ('featureGraphic', 'icon', 'promoGraphic', 'tvBanner') SCREENSHOT_DIRS = ('phoneScreenshots', 'sevenInchScreenshots', 'tenInchScreenshots', 'tvScreenshots', 'wearScreenshots') +BLANK_PNG_INFO = PngImagePlugin.PngInfo() + def dpi_to_px(density): return (int(density) * 48) / 160 @@ -371,7 +373,8 @@ def resize_icon(iconpath, density): im.thumbnail((size, size), Image.ANTIALIAS) logging.debug("%s was too large at %s - new size is %s" % ( iconpath, oldsize, im.size)) - im.save(iconpath, "PNG") + im.save(iconpath, "PNG", optimize=True, + pnginfo=BLANK_PNG_INFO, icc_profile=None) except Exception as e: logging.error(_("Failed resizing {path}: {error}".format(path=iconpath, error=e))) @@ -677,20 +680,28 @@ def _strip_and_copy_image(inpath, outpath): Sadly, image metadata like EXIF can be used to exploit devices. It is not used at all in the F-Droid ecosystem, so its much safer - just to remove it entirely. PNG does not have the same kind of - issues. + just to remove it entirely. """ - if common.has_extension(inpath, 'png'): - shutil.copy(inpath, outpath) - else: - with open(inpath) as fp: + extension = common.get_extension(inpath)[1] + if os.path.isdir(outpath): + outpath = os.path.join(outpath, os.path.basename(inpath)) + if extension == 'png': + with open(inpath, 'rb') as fp: + in_image = Image.open(fp) + in_image.save(outpath, "PNG", optimize=True, + pnginfo=BLANK_PNG_INFO, icc_profile=None) + elif extension == 'jpg' or extension == 'jpeg': + with open(inpath, 'rb') as fp: in_image = Image.open(fp) data = list(in_image.getdata()) out_image = Image.new(in_image.mode, in_image.size) out_image.putdata(data) out_image.save(outpath, "JPEG", optimize=True) + else: + raise FDroidException(_('Unsupported file type "{extension}" for repo graphic') + .format(extension=extension)) def copy_triple_t_store_metadata(apps): @@ -1512,7 +1523,8 @@ def fill_missing_icon_densities(empty_densities, icon_filename, apk, repo_dir): size = dpi_to_px(density) im.thumbnail((size, size), Image.ANTIALIAS) - im.save(icon_path, "PNG") + im.save(icon_path, "PNG", optimize=True, + pnginfo=BLANK_PNG_INFO, icc_profile=None) empty_densities.remove(density) except Exception as e: logging.warning("Invalid image file at %s: %s", last_icon_path, e) From 8f45796ecbb51303d32b5ae0136715400b4ce4ea Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Wed, 13 Dec 2017 12:28:11 +0100 Subject: [PATCH 6/8] update: close unclosed Image instance --- fdroidserver/update.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/fdroidserver/update.py b/fdroidserver/update.py index 4611b127..15013347 100644 --- a/fdroidserver/update.py +++ b/fdroidserver/update.py @@ -1487,6 +1487,8 @@ def extract_apk_icons(icon_filename, apk, apkzip, repo_dir): except Exception as e: logging.warning(_("Failed reading {path}: {error}") .format(path=icon_path, error=e)) + finally: + im.close() if apk['icons']: apk['icon'] = icon_filename From 42522c23c9dc16319eb66811467dca85bef227a4 Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Thu, 14 Dec 2017 10:58:02 +0100 Subject: [PATCH 7/8] update: do not crash if AndroidManifest.xml in APK has invalid date This crash actually blocked a Janus exploit APK from being added to the repo, but crashing isn't really the appropriate way to do that. --- fdroidserver/update.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/fdroidserver/update.py b/fdroidserver/update.py index 15013347..782d18fc 100644 --- a/fdroidserver/update.py +++ b/fdroidserver/update.py @@ -1355,10 +1355,13 @@ def process_apk(apkcache, apkfilename, repodir, knownapks, use_date_from_apk=Fal apkzip = zipfile.ZipFile(apkfile, 'r') manifest = apkzip.getinfo('AndroidManifest.xml') - if manifest.date_time[1] == 0: # month can't be zero - logging.debug(_('AndroidManifest.xml has no date')) - else: - common.check_system_clock(datetime(*manifest.date_time), apkfilename) + # 1980-0-0 means zeroed out, any other invalid date should trigger a warning + if (1980, 0, 0) != manifest.date_time[0:3]: + try: + common.check_system_clock(datetime(*manifest.date_time), apkfilename) + except ValueError as e: + logging.warning(_("{apkfilename}'s AndroidManifest.xml has a bad date: ") + .format(apkfilename=apkfile) + str(e)) # extract icons from APK zip file iconfilename = "%s.%s.png" % (apk['packageName'], apk['versionCode']) From 2e531af58f8fd329c610bb84e16e6ff1bb13d0c4 Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Thu, 14 Dec 2017 14:42:09 +0100 Subject: [PATCH 8/8] build: force purging of sudo, ignore error message Fixes bb758d3f, spotted by @bubu: DEBUG: buildserver > DEBUG: > sudo apt-get -y purge sudo DEBUG: buildserver > Reading package lists... DEBUG: buildserver > Building dependency tree... DEBUG: buildserver > Reading state information... DEBUG: buildserver > The following packages will be REMOVED: DEBUG: buildserver > sudo* DEBUG: buildserver > 0 upgraded, 0 newly installed, 1 to remove and 0 not upgraded. DEBUG: buildserver > After this operation, 2,391 kB disk space will be freed. (Reading database ... 68491 files and directories currently installed.) DEBUG: buildserver > Removing sudo (1.8.10p3-1+deb8u4) ... DEBUG: buildserver > You have asked that the sudo package be removed, DEBUG: buildserver > but no root password has been set. DEBUG: buildserver > Without sudo, you may not be able to gain administrative privileges. DEBUG: buildserver > DEBUG: buildserver > If you would prefer to access the root account with su(1) DEBUG: buildserver > or by logging in directly, DEBUG: buildserver > you must set a root password with "sudo passwd". DEBUG: buildserver > DEBUG: buildserver > If you have arranged other means to access the root account, DEBUG: buildserver > and you are sure this is what you want, DEBUG: buildserver > you may bypass this check by setting an environment variable DEBUG: buildserver > (export SUDO_FORCE_REMOVE=yes). DEBUG: buildserver > DEBUG: buildserver > Refusing to remove sudo. DEBUG: buildserver > dpkg: error processing package sudo (--purge): DEBUG: buildserver > subprocess installed pre-removal script returned error exit status 1 DEBUG: buildserver > Errors were encountered while processing: DEBUG: buildserver > sudo DEBUG: buildserver > E: Sub-process /usr/bin/dpkg returned an error code (1) --- fdroidserver/build.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/fdroidserver/build.py b/fdroidserver/build.py index e2866fbd..d4f50005 100644 --- a/fdroidserver/build.py +++ b/fdroidserver/build.py @@ -414,7 +414,12 @@ def build_local(app, build, vcs, build_dir, output_dir, log_dir, srclib_dir, ext raise BuildException("Error running sudo command for %s:%s" % (app.id, build.versionName), p.output) - p = FDroidPopen(['sudo', 'apt-get', '-y', 'purge', 'sudo']) + p = FDroidPopen(['sudo', 'passwd', '--lock', 'root']) + if p.returncode != 0: + raise BuildException("Error locking root account for %s:%s" % + (app.id, build.versionName), p.output) + + p = FDroidPopen(['sudo', 'SUDO_FORCE_REMOVE=yes', 'apt-get', '-y', 'purge', 'sudo']) if p.returncode != 0: raise BuildException("Error removing sudo for %s:%s" % (app.id, build.versionName), p.output)