From a9099f9e2383affcdca60e86ab167216f1ec68b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Mon, 21 Dec 2015 16:08:14 +0100 Subject: [PATCH] SECURITY: ensure we never accept fake images --- app/models/upload.rb | 11 +++++++--- spec/controllers/uploads_controller_spec.rb | 21 ++++++++++++++++++++ spec/fixtures/images/fake.jpg | Bin 0 -> 2881 bytes 3 files changed, 29 insertions(+), 3 deletions(-) create mode 100644 spec/fixtures/images/fake.jpg diff --git a/app/models/upload.rb b/app/models/upload.rb index 8e2150541f9..6c693730184 100644 --- a/app/models/upload.rb +++ b/app/models/upload.rb @@ -67,7 +67,7 @@ class Upload < ActiveRecord::Base def self.create_for(user_id, file, filename, filesize, options = {}) DistributedMutex.synchronize("upload_#{user_id}_#{filename}") do # do some work on images - if FileHelper.is_image?(filename) + if FileHelper.is_image?(filename) && system("identify '#{file.path}' >/dev/null 2>&1") if filename =~ /\.svg$/i svg = Nokogiri::XML(file).at_css("svg") w = svg["width"].to_i @@ -122,7 +122,7 @@ class Upload < ActiveRecord::Base upload = find_by(sha1: sha1) # make sure the previous upload has not failed - if upload && upload.url.blank? + if upload && (upload.url.blank? || is_dimensionless_image?(filename, upload.width, upload.height)) upload.destroy upload = nil end @@ -141,8 +141,9 @@ class Upload < ActiveRecord::Base upload.height = height upload.origin = options[:origin][0...1000] if options[:origin] - if FileHelper.is_image?(filename) && (upload.width == 0 || upload.height == 0) + if is_dimensionless_image?(filename, upload.width, upload.height) upload.errors.add(:base, I18n.t("upload.images.size_not_found")) + return upload end return upload unless upload.save @@ -162,6 +163,10 @@ class Upload < ActiveRecord::Base end end + def self.is_dimensionless_image?(filename, width, height) + FileHelper.is_image?(filename) && (width.blank? || width == 0 || height.blank? || height == 0) + end + def self.get_from_url(url) return if url.blank? # we store relative urls, so we need to remove any host/cdn diff --git a/spec/controllers/uploads_controller_spec.rb b/spec/controllers/uploads_controller_spec.rb index 2a0ceaf22c9..4a57897e771 100644 --- a/spec/controllers/uploads_controller_spec.rb +++ b/spec/controllers/uploads_controller_spec.rb @@ -19,6 +19,13 @@ describe UploadsController do }) end + let(:fake_jpg) do + ActionDispatch::Http::UploadedFile.new({ + filename: 'fake.jpg', + tempfile: file_from_fixtures("fake.jpg") + }) + end + let(:text_file) do ActionDispatch::Http::UploadedFile.new({ filename: 'LICENSE.TXT', @@ -118,6 +125,20 @@ describe UploadsController do expect(response).to_not be_success end + it 'returns an error when it could not determine the dimensions of an image' do + Jobs.expects(:enqueue).with(:create_thumbnails, anything).never + + message = MessageBus.track_publish do + xhr :post, :create, file: fake_jpg, type: "composer" + end.first + + expect(response.status).to eq 200 + + expect(message.channel).to eq("/uploads/composer") + expect(message.data["errors"]).to be + expect(message.data["errors"][0]).to eq(I18n.t("upload.images.size_not_found")) + end + end end diff --git a/spec/fixtures/images/fake.jpg b/spec/fixtures/images/fake.jpg new file mode 100644 index 0000000000000000000000000000000000000000..e59fa523ee03a3f7f9b9ac66eb6344d33c383837 GIT binary patch literal 2881 zcmZ<`59SLIVqmDaH8tXHvG8@#{ptJHZP})ClZ~0JiGz(@Svir3lS!sWSK9i9(KHVE z&Zc!sc3(Q=wffDvKNFYr-flDd>vy?-`ni%j)9tqM|K+y2Jo8+M)n!S^TOV#6-}iK< zzVG?FQ+4f(W`-TUSN+cN`Mheoy6^fdA@(596O(c^n5vW zCZ}d^p6MNvb$b2RY}U{8Q@eicn&0}lwz<|ctxVzTuLrlK7Yq? zH}zU(<2$<6X_t-Py2Y%CJegNH-QC?fO#5l<;~E_MMqIrgA24 zwm#_43x6?1_MFY{$X?lb6Qb7y=5ExQe@5r$rn_6;D}Vm+#3id{Vs4NBl5=yo_XwQv zKKXpo-G>Q()lT}niT|W(q+9#(sCap)B=?$!i!=7`^!{^yz5U}Ov#9(nva&Z7VDnv-k0wq02@eaGT0XWGPPf4`dUv;E{A zqh&gq-rNXn-@Wd<{E3f6rw#Y^_B_7PcfKp`^z-S(a-X9bz24Vs)9$gA_g5)c(DY4r z+rp=_GJ0NrznhsBsTy)oXLjfgVZJL;cewAz?5wkWQ29D;o@MB}mmZ}&-_N9elD%|w zf?MvcIcGa7*T;w0-7T`+y-zwPWKK}&o3O=3xzXE<1GY)GMdn)m+4XXfjg+(xA|{=s&Rr76jdyIyOaUSqc9<(VM6 z&5MjSb|iQ{w*G3nisR0=Yj3BWHs%f6RH6E)X#MVZ{jE0V^L~8nWVzzAdXfIGS4=&x zzI^@p(mYw?<8lt6GtYBr%Ubyej{p`Z)!AH(Y?tb}P_1(?FjXlic;ci$1>{hM^BTv4|FL19Pdr{+Bc0Z*G&SMC$vvf1}=+Ob(N#kTC{Umks~`)tM| zhVX;)jwju{EWR^*vv${xs;Ilms|%(_aLs8CFaLJXDrZG@+R@dSrg>r6aVxqc*Ud}~ z*NvWI>27`FmD%5wcWyp$(JHlz%$hkZ-RzsqWyed7a&Gfd&#@G5E15t2uU}(%o3;DA z&`GkN#l zA88NvY&;ZuJF8Rt$XjuXa*h0umbn*7$_js0sm-_ib7tpx^K{kSb0WEP*QY!WGujyw zuV-9aoN8D9JT5I&b@wG@sn2_3>a^;O#b=#oJM-^(mGq+rE8k7)dtbe7|4)NQQ%n3a z4?oUpcr9?~baI&0y5DE^MDWx`%KIE=kAKnfvf|CnxEX!#Ij#7bN}exUuDna+;?xeg zi=}_=9az%gZMP@o_~}Nsn%DP@--_$RB`b=thX`fyxVOK3`0PXV;d2|Le>wDJD7>`Yo3%t=>b3lf%TE6K<{BO$ zK^iMUmWeot916L#@{04)I4#!|SKJl7rq-?E4al8vX>W-1qFZ*W-Y&nzxQaLUt-@8l zz_$ih;}+c#Ty=D1*_15(;I|T2-7;RVw1#WUS@JMgL`dY=%A#3LlbpH^v9uP8oC~&a zd3s%AkKaS4sh36cf_@6Mo)%Hlj9F;l`cSJilgl{dRKSd-2JVUT4l%X5a(Rnv5}BrX zXTb)iuQFFZhwS(Kx_w1m_odq+?*b}jAF>W{&uo14K4^#gSH@N1nyDgQQ>JN#Xp~vr zT~IaEskh1Vpxf@haRRSRe2tBB@%9ucY+?z?yJ2&n#Gz1AB*}9_b2Q# z{njti{$p8C`!^fSW5SDXE`H#z@BA_1k;Cm9lVvAPkjj&;W-Z-QaY!O{{Skv-Wm(px z(|c;ordfryA68sBx8;r9g9~>5#3dAu*Q@vPoaNIzWWvrZlzzi=O79D0b~m*$6(O%B zMFAU$^)EJF@zO*)RVMhV?eBk1aN^_#G{LY~o#{u|zB=#W-~O z#jcvBxq`ukhb&IV@$${G`saPa($jI_q>ARb!p66FmIWtCr9MBZQkEX?9M3(mmE&g5 zmCz?E)Ni~_65_hatt7B@hMC^B-h*#cR~9&~nRr)hd1&8%&tnT(e>3MMD-`d| zZ_b~rc4>0>@_Oz6asQfYO5M&11f0-Gns`Rg(siAntKw&uUIhfUzS&184{m5VRT&)JHYYUy-X>FOMK<`nC;asDcE#cz{7yqL&vQ274h zM-GqHeaQS~bu;NXU;7hFkMl9RcWLMgw$zFBtDSa0rOGhd%_z|?g|S>TRu5j(=d_8l>|-)_*ceN~0eg;wdK zjF0cU;XScWQI=1$xZv^I>TvO?qD)37IrndN5j&^NJ;U9%|KH8>eQRu<+O{8`G3UP_ zi>Pye^DF1Z(@yvJ75QgZHd@*%sdjic2ia)t)?D!@`j~E+bw`kJV4P6kG!c8BumAai z=Qe-0Z|Z;eYSA^`FMS*)-Xb&asAkDIUlZ%l%1r#5VjUVFCbDVegUH)&-cI_}u{StK zezTLOuI8J&hgv7?;8y)A^{+iR^vj8#=>}!_Z%!_BGK^MX4bw@L6gj1wvL?dN!p?2m z8j)2uwFD<91|0t9vO=fzD-Y8?4av0I78CaBbAEEXE%EK-gv71A&32MzuW}zx@PFmD zI9KM|Nx8#$()x;>fAv&)TyL!`dlbE4@7B4tN;4kDKHI+AC)VIQ=QH;WxhC6%?l-Q@ zeY{t;@cg@Z{kyBH%4gnxZKS;W%=_}0wOwNG{w|o?m4ED%7t=hEq<)@sGq%}=M+Gu>3CxT-WAhm?OMI^QPip(^R(PP+|Mn#@&9Vo Y#pipQAH8SP|FiM|m(hRjDTXQ*0AsX-fB*mh literal 0 HcmV?d00001