1275724 - PNG with empty first IDAT chunk crashes FF process

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[shekyan]

Attachment: first_idat_empty.png

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/50.0.2661.102 Safari/537.36

Steps to reproduce:

create Image element and load it from data URL below, 
like
```
var i = new Image();
i.src = ...
```


```
'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAP0AAAD9CAIAAAD4V+arAAAAAXNSR0IArs4c6QAAAAlwSFlzAAAOwwAADsMBx2+oZAAAABZ0RVh0U29mdHdhcmUAcGFpbnQubmV0IDQuMDvo9WkAAAAASURBVDWvBh4AABEVSURBVHhe7dhPqKV3fcfxI92I0CKIUITQ0o34b1OUllJas1BjdROiWcSgobSlrV20ZFJEUJOGLhpFF7UN1ZRUSBexQRe1Nu0iaemUlko3rYZkEaKDIppASUCMUHp6k3kmc+/7d+/kzvi6u++H1+Kc58zq8n6e+Z2zu/6mPfebN3j3/Ir3pbd6//UG74c/4+1fewZe9R1v949nYbrHkiyRZIkmSyRZYkmWmO6xJEskWaLJEkmWWJIlpnssyV7ZEw/sX9rzD/TTlyRZoskSSZZYkiWmeyzJHnHL/vkL+ycuvX0h+kNvnz05/SRLNFkiyRJLssR0jyXZI452fxD692458dPDkizRZIkkSyzJEtM9lmSPOFz2cZUf3AnfOnrloiRLNFkiyRJLssR0jyXZiw6Czp4/P92fzpIsMd1jSfaIOedcgyVZYrrHkuwRR8tev9c+e9f2OpIs0WSJJEssyRLTPZZkj1ie6Id/xzwp+gNJlmiyRJIllmSJ6R5Lsqdy8gnnoiRLNFkiyRJLssR0jyXZU/rW+e2pf+TEf0mSJZoskWSJJVliuseSLJFkiSZLJFliSZaY7rEkSyRZoskSSZZYkiWmeyzJEkmWaLJEkiWWZInpHkuyRJIlmiyRZIklWWK6x5IskWSJJkskWWJJlpjusSRLJFmiyRJJlliSJaZ7LMkSSZZoskSSJZZkiekeS7JEkiWaLJFkiSVZYrrHkiyRZIkmSyRZYkmWmO6xJEskWaLJEkmWWJIlpnssyRJJlmiyRJIllmSJ6R5LskSSJZoskWSJJVliuseSLJFkiSZLJFliSZaY7rEkSyRZoskSSZZYkiWmeyzJEkmWaLJEkiWWZIndm397z934Qe+Om7y/eLf3yK96F97q7d/4mPezD3mv/aOzMN1jSZZIskSTJZIssSRLTPdYkiWSLNFkiSRLLMkS0z2WZIkkSzRZIskSS7LEdI8lWSLJEk2WSLLEkiwx3WNJlkiyRJMlkiyxJEtM91iSJZIs0WSJJEssyRLTPZZkiSRLNFkiyRJLssR0jyVZIskSTZZIssSSLDHdY0mWSLJEkyWSLLEkS0z3WJIlkizRZIkkSyzJEtM9lmSJJEs0WSLJEkuyxHSPJVkiyRJNlkiyxJIsMd1jSZZIskSTJZIssSRLTPdYkiWSLNFkiSRLLMkS0z2WZIkkSzRZIskSS7LEdI8lWSLJEk2WSLLEkiwx3WNJlkiyRJMlkiyxJEtM91iSJZIs0WSJJEssyRLTPZZkiSRLNFkiyRJLssR0jyVZIskSTZZIssSSLDHdY0mWSLJEkyWSLLEkS0z3WJIlkizRZIkkSyzJEtM9lmSJJEs0WSLJEkuyxHSPJVkiyRJNlkiyxJIsMd1jSZZIskSTJZIssSRLTPdYkiWSLNFkiSRLLMkS0z2WZIkkSzRZIskSS7LE7qc/vud++Xbvtt/x/vhD3oPve5b7z1/7N+65t/8lt3/b7d6b3n0WpnssyRJJlkiyRJMllmSJ6R5LskSSJZIs0WSJJVliuseSLJFkiSRLNFliSZaY7rEkSyRZIskSTZZYkiWmeyzJEkmWSLJEkyWWZInpHkuyRJIlkizRZIklWWK6x5IskWSJJEs0WWJJlpjusSRLJFkiyRJNlliSJaZ7LMkSSZZIskSTJZZkiekeS7JEkiWSLNFkiSVZYrrHkiyRZIkkSzRZYkmWmO6xJEskWSLJEk2WWJIlpnssyRJJlkiyRJMllmSJ6R5LskSSJZIs0WSJJVliuseSLJFkiSRLNFliSZaY7rEkSyRZIskSTZZYkiWmeyzJEkmWSLJEkyWWZInpHkuyRJIlkizRZIklWWK6x5IskWSJJEs0WWJJlpjusSRLJFkiyRJNlliSJaZ7LMkSSZZIskSTJZZkiekeS7JEkiWSLNFkiSVZYrrHkiyRZIkkSzRZYkmWmO6xJEskWSLJEk2WWJIlpnssyRJJlkiyRJMllmSJ6R5LskSSJZIs0WSJJVliuseSLJFkiSRLNFliSZbY/cSf7rnX/4n33k94v3/uAvfZ332Y+4fbPs09+f7f4Pbv+SXv+lefhekeS7JEkiWSLNFkiSVZYrrHkiyRZIkkSzRZYkmWmO6xJEskWSLJEk2WWJIlpnssyRJJlkiyRJMllmSJ6R5LskSSJZIs0WSJJVliuseSLJFkiSRLNFliSZaY7rEkSyRZIskSTZZYkiWmeyzJEkmWSLJEkyWWZInpHkuyRJIlkizRZIklWWK6x5IskWSJJEs0WWJJlpjusSRLJFkiyRJNlliSJaZ7LMkSSZZIskSTJZZkiekeS7JEkiWSLNFkiSVZYrrHkiyRZIkkSzRZYkmWmO6xJEskWSLJEk2WWJIlpnssyRJJlkiyRJMllmSJ6R5LskSSJZIs0WSJJVliuseSLJFkiSRLNFliSZaY7rEkSyRZIskSTZZYkiWmeyzJEkmWSLJEkyWWZInpHkuyRJIlkizRZIklWWK6x5IskWSJJEs0WWJJlpjusSRLJFkiyRJNlliSJaZ7LMkSSZZIskSTJZZkiekeS7JEkiWSLNFkiSVZYrrHkiyRZIkkSzRZYkmWmO6xJEskWSLJEk2WWJIldrsH99xr7v8/7hc++3XuA/d8kbvzDPbAHe/n/v3Db+Keue0V3P7m3VmY7rEtVbokSyRZIskS6VWZ7rEtVbokSyRZIskS6VVZun94/9R+27l8dGpJlkiyRJIltlRP3r3nn9n+vvv9M+fv3a5ecUmWSLJEkiXS67X7yH3bH33/6MHb4573B+k/u78hF69GkiWSLJFkiS3VE/ZC9Jdj//Ljp0s/yRJJlkiyxJF2r9ZB69+979i30z22pXr8DkI/2vnBfXCK8JMskWSJJEtcrvYaTPfHSrLEluqxO6bygzvh8S9vr09ckiWSLJFkicvVXoMrdP/odui5vHPT/Y9hS/XYTfdX73K1V+HclvLhfeWGed5vkiyxpXr85pxz1Q7VfPXmnHOsJEtsqZ6w+V57tS5Xew2uovuD6Lf/GOZ3zGuxpXryXkj/0l7+iPPikiyRZIkkS1yu9hoc7v7g9baTfsf8sSVZIskSSZbYUqVLskSSJZIssVWrTffYlipdkiWSLJFkifSqTPfYlipdkiWSLJFkifSqTPfYlipdkiWSLJFkifSqTPfYlipdkiWSLJFkifSqTPfYlipdkiWSLJFkifSqTPfYlipdkiWSLJFkifSqTPfYlipdkiWSLJFkifSqTPfYlipdkiWSLJFkifSqTPfYlipdkiWSLJFkifSqTPfYlipdkiWSLJFkifSqTPfYlipdkiWSLJFkifSqTPfYlipdkiWSLJFkifSqTPfYlipdkiWSLJFkifSqTPfYlipdkiWSLJFkifSqTPfYlipdkiWSLJFkifSqTPfYlipdkiWSLJFkifSq7HaP/I/3lX/lfu6Ln+feef8fcB/+s3dxn/nkddzf3rnjHv9D739/70xM91iSJZIskWSJJEukV2W6x5IskWSJJEskWSK9KtM9lmSJJEskWSLJEulVme6xJEskWSLJEkmWSK/KdI8lWSLJEkmWSLJEelWmeyzJEkmWSLJEkiXSqzLdY0mWSLJEkiWSLJFelekeS7JEkiWSLJFkifSqTPdYkiWSLJFkiSRLpFdluseSLJFkiSRLJFkivSrTPZZkiSRLJFkiyRLpVZnusSRLJFkiyRJJlkivynSPJVkiyRJJlkiyRHpVpnssyRJJlkiyRJIl0qsy3WNJlkiyRJIlkiyRXpXpHkuyRJIlkiyRZIn0qkz3WJIlkiyRZIkkS6RXZbrHkiyRZIkkSyRZIr0q0z2WZIkkSyRZIskS6VWZ7rEkSyRZIskSSZZIr8p0jyVZIskSSZZIskR6VaZ7LMkSSZZIskSSJdKrMt1jSZZIskSSJZIskV6V6R5LskSSJZIskWSJ9KpM91iSJZIskWSJJEukV2W6x5IskWSJJEskWSK9KtM9lmSJJEskWSLJEulV2e2+/lXvPz7F/eSjv879/N/9Infz3/wU99G/2nH3/7n3L5/yvnvXmZjusSRLJFkiyRJJlkivynSPJVkiyRJJlkiyRHpVpnssyRJJlkiyRJIl0qsy3WNJlkiyRJIlkiyRXpXpHkuyRJIlkiyRZIn0qkz3WJIlkiyRZIkkS6RXZbrHkiyRZIkkSyRZIr0q0z2WZIkkSyRZIskS6VWZ7rEkSyRZIskSSZZIr8p0jyVZIskSSZZIskR6VaZ7LMkSSZZIskSSJdKrMt1jSZZIskSSJZIskV6V6R5LskSSJZIskWSJ9KpM91iSJZIskWSJJEukV2W6x5IskWSJJEskWSK9KtM9lmSJJEskWSLJEulVme6xJEskWSLJEkmWSK/KdI8lWSLJEkmWSLJEelWmeyzJEkmWSLJEkiXSqzLdY0mWSLJEkiWSLJFelekeS7JEkiWSLJFkifSqTPdYkiWSLJFkiSRLpFdluseSLJFkiSRLJFkivSrTPZZkiSRLJFkiyRLpVZnusSRLJFkiyRJJlkivynSPJVkiyRJJlkiyRHpVpnssyRJJlkiyRJIl0quy233/Y95TN3nfeAN33dd23PX/5P3WV717HvK+9AXvv+89E9M9lmSJJEskWSLJEulVme6xJEskWSLJEkmWSK/KdI8lWSLJEkmWSLJEelWmeyzJEkmWSLJEkiXSqzLdY0mWSLJEkiWSLJFelekeS7JEkiWSLJFkifSqTPdYkiWSLJFkiSRLpFdluseSLJFkiSRLJFkivSrTPZZkiSRLJFkiyRLpVZnusSRLJFkiyRJJlkivynSPJVkiyRJJlkiyRHpVpnssyRJJlkiyRJIl0qsy3WNJlkiyRJIlkiyRXpXpHkuyRJIlkiyRZIn0qkz3WJIlkiyRZIkkS6RXZbrHkiyRZIkkSyRZIr0q0z2WZIkkSyRZIskS6VWZ7rEkSyRZIskSSZZIr8p0jyVZIskSSZZIskR6VaZ7LMkSSZZIskSSJdKrMt1jSZZIskSSJZIskV6V6R5LskSSJZIskWSJ9KpM91iSJZIskWSJJEukV2W6x5IskWSJJEskWSK9KtM9lmSJJEskWSLJEulVme6xJEskWSLJEkmWSK/KdI8lWSLJEkmWSLJEelV2u/07vB+8zvv+wS2KvfKb3lse8278mnfHP3uf+3vvkYfOxHSPJVkiyRJJlkiyRHpVpnssyRJJlkiyRJIl0qsy3WNJ9gpuffqp/aVdePqGfHpYkiWSLJFkifSqTPdYkj3JC9H/6L5bX3z95sfOnb9i+kmWSLJEkiXSqzLdY0n2WC+G/tRfP3noypP3Xbh0G6ySLJFkiSRLpFdluseS7LHWyl+8Ex69+9CVw5IskWSJJEukV2W6x5LsgYvHmG66P530qkz3WJI91pxzTi+9KtM9lmRPsn6vPf/tI//gsCRLJFkiyRLpVZnusSR7BYd/x7xC9AeSLJFkiSRLpFdluseS7Mu68gnnoiRLJFkiyRLpVZnusSR7Gnc/d/Ghf+TEf1iSJZIskWSJ9KpM91iSJZIskWSJJEukV2W6x5IskWSJJEskWSK9KtM9lmSJJEskWSLJEulVme6xJEskWSLJEkmWSK/KdI8lWSLJEkmWSLJEelWmeyzJEkmWSLJEkiXSqzLdY0mWSLJEkiWSLJFelekeS7JEkiWSLJFkifSqTPdYkiWSLJFkiSRLpFdluseSLJFkiSRLJFkivSrTPZZkiSRLJFkiyRLpVZnusSRLJFkiyRJJlkivynSPJVkiyRJJlkiyRHpVpnssyRJJlkiyRJIl0qsy3WNJlkiyRJIlkiyRXpXpHkuyRJIlkiyRZIn0ajy0+38V5h1S+M1kTAAAAABJRU5ErkJggg==' 
``` 



Actual results:

Crashes the browser


Expected results:

error should be handled. Don't know what happens in the PNG parsing library that you use, but probably not very good things.

Comment 1

[:Gijs (out for now; he/him)]

Seth or :njn, can either of you take a look? Here's a content crashreport from nightly:

https://crash-stats.mozilla.com/report/index/51a59a2c-65ff-4ed6-8001-7dec12160525

Comment 2

[Nicholas Nethercote [inactive]]

I tried loading the attached PNG. It failed to display because "it contains errors". No crash.

I then tried pasting the data: URL from comment 0 into the address bar. I got the data: URL showing up in the content window, which is a bit weird, but again no crash.

shekyan, are you able to attach an HTML file that I can just load to reproduce the problem? I'm having trouble following your instructions in comment 0. Thank you.

Comment 3

[shekyan]

Attachment: self contained HTML page that causes FF 46.0.1 to crash

Comment 4

[shekyan]

Here are Crash Reporter generated details:

AdapterDeviceID: 0x0fe9
AdapterVendorID: 0x10de
Add-ons: %7B972ce4c6-7e08-4474-a285-3208198ce6fd%7D:46.0.1,e10srollout%40mozilla.org:1.0,firefox%40getpocket.com:1.0,loop%40mozilla.org:1.2.6
BuildID: 20160502172042
CrashTime: 1464222377
EMCheckCompatibility: true
EventLoopNestingLevel: 1
FramePoisonBase: 7ffffffff0dea000
FramePoisonSize: 4096
InstallTime: 1462817332
Notes: AdapterVendorID: 0x10de, AdapterDeviceID: 0x0fe9GL Layers? GL Context? GL Context+ GL Layers+ 
ProductID: {ec8030f7-c20a-464f-9b0e-13a3a9e97384}
ProductName: Firefox
ReleaseChannel: release
SafeMode: 0
SecondsSinceLastCrash: 7508
StartupTime: 1464219616
TelemetryEnvironment: {"build":{"applicationId":"{ec8030f7-c20a-464f-9b0e-13a3a9e97384}","applicationName":"Firefox","architecture":"x86-64","buildId":"20160502172042","version":"46.0.1","vendor":"Mozilla","platformVersion":"46.0.1","xpcomAbi":"x86_64-gcc3","hotfixVersion":"20160106.01","architecturesInBinary":"i386-x86_64"},"partner":{"distributionId":null,"distributionVersion":null,"partnerId":null,"distributor":null,"distributorChannel":null,"partnerNames":[]},"system":{"memoryMB":16384,"virtualMaxMB":null,"cpu":{"count":8,"cores":4,"vendor":"GenuineIntel","family":6,"model":70,"stepping":1,"l2cacheKB":256,"l3cacheKB":6144,"speedMHz":2300,"extensions":["hasMMX","hasSSE","hasSSE2","hasSSE3","hasSSSE3","hasSSE4_1","hasSSE4_2"]},"os":{"name":"Darwin","version":"15.5.0","locale":"en-US"},"hdd":{"profile":{"model":null,"revision":null},"binary":{"model":null,"revision":null},"system":{"model":null,"revision":null}},"gfx":{"D2DEnabled":null,"DWriteEnabled":null,"adapters":[{"description":null,"vendorID":"0x10de","deviceID":"0x0fe9","subsysID":null,"RAM":null,"driver":null,"driverVersion":null,"driverDate":null,"GPUActive":true}],"monitors":[{"screenWidth":2560,"screenHeight":1440,"scale":1}],"features":{"compositor":"none"}}},"settings":{"blocklistEnabled":true,"e10sEnabled":false,"e10sCohort":"unsupportedChannel","telemetryEnabled":false,"isInOptoutSample":false,"locale":"en-US","update":{"channel":"release","enabled":true,"autoDownload":true},"userPrefs":{"browser.cache.disk.capacity":358400,"browser.newtabpage.enhanced":true,"browser.shell.checkDefaultBrowser":false,"browser.urlbar.userMadeSearchSuggestionsChoice":true},"addonCompatibilityCheckEnabled":true,"isDefaultBrowser":false},"profile":{"creationDate":16930},"addons":{"activeAddons":{"[e-mail]":{"blocklisted":false,"description":"Staged rollout of Firefox multi-process feature.","name":"Multi-process staged rollout","userDisabled":false,"appDisabled":false,"version":"1.0","scope":1,"type":"extension","foreignInstall":false,"hasBinaryComponents":false,"installDay":16924,"updateDay":16924},"[e-mail]":{"blocklisted":false,"description":"When you find something you want to view later, put it in Pocket.","name":"Pocket","userDisabled":false,"appDisabled":false,"version":"1.0","scope":1,"type":"extension","foreignInstall":false,"hasBinaryComponents":false,"installDay":16924,"updateDay":16924},"[e-mail]":{"blocklisted":false,"description":"Web sharing for Firefox","name":"Firefox Hello","userDisabled":false,"appDisabled":false,"version":"1.2.6","scope":1,"type":"extension","foreignInstall":false,"hasBinaryComponents":false,"installDay":16924,"updateDay":16924}},"theme":{"id":"{972ce4c6-7e08-4474-a285-3208198ce6fd}","blocklisted":false,"description":"The default theme.","name":"Default","userDisabled":false,"appDisabled":false,"version":"46.0.1","scope":4,"foreignInstall":false,"hasBinaryComponents":false,"installDay":16924,"updateDay":16924},"activePlugins":[{"name":"Default Browser Helper","version":"601","description":"Provides information about the default web browser","blocklisted":false,"disabled":false,"clicktoplay":true,"mimeTypes":["application/apple-default-browser"],"updateDay":16881},{"name":"Java Applet Plug-in","version":"Java 8 Update 91 build 14","description":"Displays Java applet content, or a placeholder if Java is not installed.","blocklisted":false,"disabled":false,"clicktoplay":true,"mimeTypes":["application/x-java-applet;version=1.4","application/x-java-applet;version=1.2.1","application/x-java-applet;version=1.2.2","application/x-java-applet;version=1.3","application/x-java-applet;version=1.8","application/x-java-vm","application/x-java-applet;deploy=11.91.2","application/x-java-applet;version=1.1.1","application/x-java-applet;version=1.2","application/x-java-applet;version=1.7","application/x-java-applet;version=1.4.1","application/x-java-applet;version=1.1.2","application/x-java-applet","application/x-java-applet;version=1.1","application/x-java-applet;version=1.1.3","application/x-java-applet;jpi-version=1.8.0_91","application/x-java-applet;javafx=8.0.91","application/x-java-applet;version=1.6","application/x-java-applet;version=1.4.2","application/x-java-applet;version=1.3.1","application/x-java-applet;version=1.5"],"updateDay":16892}],"activeGMPlugins":{"gmp-gmpopenh264":{"version":"1.5.3","userDisabled":false,"applyBackgroundUpdates":1}},"activeExperiment":{},"persona":null}}
Theme: classic/1.0
Throttleable: 1
URL: file:///Users/sergey/Projects/medusa/sift_tests/page.html
UptimeTS: 2035.3576795
Vendor: Mozilla
Version: 46.0.1
useragent_locale: en-US

This report also contains technical information about the state of the application when it crashed.

Comment 5

[Nicholas Nethercote [inactive]]

On Linux it doesn't crash for me on either release or trunk. Instead, the page stays white and the page load spinner just spins endlessly.

I think seth or tn will be a better assignee than me.

Comment 6

[shekyan]

I can confirm the crash on OS X and Windows 7.

Comment 7

[:Gijs (out for now; he/him)]

(In reply to Nicholas Nethercote [:njn] from comment #5)
> On Linux it doesn't crash for me on either release or trunk. Instead, the
> page stays white and the page load spinner just spins endlessly.

Iiiinteresting. I also tested on OS X, and the crash reproduced there just trying to load the attached png from disk in a tab. Wonder why there'd be a difference.

> I think seth or tn will be a better assignee than me.

Pinging :tn as well, then. Thanks for looking!

Comment 8

[Timothy Nikkel (:tnikkel)]

Had a really quick look at this before going to bed. In a debug build I get

Assertion failure: !IsMetadataDecode() (Stopping frame during metadata decode), at /Users/tim/ff/src/image/Decoder.cpp:435

From the stack it looks like the "end callback" is getting called by the libpng decoder during a metadata only decode. From the description of the png it sounds like it will generate a decode error when reading the header. So my guess is that we need to make our "end callback" aware that it can be called during metadata only decodes if the image is malformed and causes the decoder to error out while reading just the header.

If that's the problem then it's really surprising that our fuzzers didn't catch this sooner.

Comment 9

[Andrew McCreight [:mccr8]]

(Christoph might be interested in comment 8.)

Comment 10

[Daniel Veditz [:dveditz]]

(In reply to shekyan from comment #4)
> Here are Crash Reporter generated details:
 ... 
> This report also contains technical information about the state of the application when it crashed.

That last bit hides the most important info. If you submitted the crash to us you can open the page about:crashes and find a link to your crash report, then paste the link (not all the data!) here as Gijs did in comment 1. Although since we have his you don't need to do so unless it looks different (for example, if the crashing address is not 0x8, or the functions in the stack differ).

Comment 11

[shekyan]

Sorry, didn't know about about:crashes! Report details are similar to posted above.

Comment 12

[Timothy Nikkel (:tnikkel)]

(In reply to Timothy Nikkel (:tnikkel) from comment #8)
> libpng decoder during a metadata only decode. From the description of the
> png it sounds like it will generate a decode error when reading the header.

I was confusing IDAT with IHDR. The header is fine, it's an empty image data chunk that is unusual. Which makes it much less likely that a fuzzer would trip on it. So this comment:

> If that's the problem then it's really surprising that our fuzzers didn't
> catch this sooner.

was probably not on the mark.

Comment 13

[Timothy Nikkel (:tnikkel)]

Attachment: patch

Glenn, can you advise how to get this patch reviewed and into libpng?

Comment 14

[Timothy Nikkel (:tnikkel)]

The crash seems to be a null-deref because we don't have an imgFrame. So doesn't seem like a security issue.

Comment 15

[Glenn Randers-Pehrson]

(In reply to Timothy Nikkel (:tnikkel) from comment #13)
> Created attachment 8757146 [details] [diff] [review]
> patch
> 
> Glenn, can you advise how to get this patch reviewed and into libpng?

Posting it here was sufficient, thanks.

Comment 16

[Glenn Randers-Pehrson]

Verified that nightly plus the libpng-1.6.22 upgrade (bug 1275901) crashes the tab.  libpng's
contrib/pngminim/preader/rpng2-x fails to display the image, without crashing.  pngcrush repairs the file.

Comment 17

[Glenn Randers-Pehrson]

Verified that Timothy's patch fixes the behavior of contrib/pngminim/preader/rpng2-x.

Comment 18

[Glenn Randers-Pehrson]

Note that the test image isn't "malformed".  Zero-length IDAT chunks are allowed by the PNG specification. See <https://www.w3.org/TR/PNG/#12Compression>

Comment 19

[Glenn Randers-Pehrson]

Comment on attachment 8757146 [details] [diff] [review]
patch

Looks good, r+

Comment 20

[Timothy Nikkel (:tnikkel)]

Can I just land this patch to mozilla-central as is, and then when you do the next libpng update that has this patch it'll just overwrite the libpng copy we have in the tree?

(I checked and your 1.6.22 update patch doesn't touch this file, so no worries about that.)

Comment 21

[Glenn Randers-Pehrson]

(In reply to Timothy Nikkel (:tnikkel) from comment #20)
> Can I just land this patch to mozilla-central as is, and then when you do
> the next libpng update that has this patch it'll just overwrite the libpng
> copy we have in the tree?
> 
> (I checked and your 1.6.22 update patch doesn't touch this file, so no
> worries about that.)

Sure.  I guess there ought to be an entry in media/libpng/MOZCHANGES

Then after your patch lands publicly we'll work on it openly in libpng-1.6.23 (most
likely we'll just apply your patch).

Glenn

Comment 22

[Timothy Nikkel (:tnikkel)]

(In reply to Glenn Randers-Pehrson from comment #21)
> (In reply to Timothy Nikkel (:tnikkel) from comment #20)
> > Can I just land this patch to mozilla-central as is, and then when you do
> > the next libpng update that has this patch it'll just overwrite the libpng
> > copy we have in the tree?
> > 
> > (I checked and your 1.6.22 update patch doesn't touch this file, so no
> > worries about that.)
> 
> Sure.  I guess there ought to be an entry in media/libpng/MOZCHANGES
> 
> Then after your patch lands publicly we'll work on it openly in
> libpng-1.6.23 (most
> likely we'll just apply your patch).

Is there a security issue here? This leads to a null ptr crash in gecko code, but I'm not sure what else could happen in libpng with a different png that has a zero-sized IDAT chunk.

Comment 23

[Glenn Randers-Pehrson]

I don't see a security issue.  It looks to me like just a bug that fails to read all of the IDAT chunks.

Comment 24

[u153838]

There's a whole lot I'm not understanding here; so far as I can see it *IS* a security issue, but it may be that all the hoopla about crashes and asserts is unrelated to libpng.  So:

Can someone confirm (or deny) that the reason these apps are crashing is because libpng fails to make the 'info' callback, the app keeps on reading the PNG, then hits unexpected code resulting in an assert (or maybe a SEGV/BUS; it's effectively the same)?

I ask because it is very important; libpng APPARENTLY skips TWO pieces of code and one of them might result in a crash in libpng; if it does we need to know about it PDQ.

Either way it is a security issue; it's only *NOT* a security issue if that second test on png_IDAT (in pngpread.c) gets executed some time later, but I don't see how that can be.

Comment 25

[Timothy Nikkel (:tnikkel)]

(In reply to [e-mail] from comment #24)
> There's a whole lot I'm not understanding here; so far as I can see it *IS*
> a security issue, but it may be that all the hoopla about crashes and
> asserts is unrelated to libpng.  So:

Crashes and asserts are not security issues on their own.

> Can someone confirm (or deny) that the reason these apps are crashing is
> because libpng fails to make the 'info' callback, the app keeps on reading
> the PNG, then hits unexpected code resulting in an assert (or maybe a
> SEGV/BUS; it's effectively the same)?

Yes, the info callback is not called, when Firefox is doing a size-only decode. A size-only decode means we stop the decode when we have the size from the decoder. The info callback isn't called, and libpng keeps decoding, it hits the end of the png and calls the end callback. Firefox only expects the end callback to be called on a full decode.

> I ask because it is very important; libpng APPARENTLY skips TWO pieces of
> code and one of them might result in a crash in libpng; if it does we need
> to know about it PDQ.

A crash (like dereferencing null) is not necessarily a security issue.

> Either way it is a security issue; it's only *NOT* a security issue if that
> second test on png_IDAT (in pngpread.c) gets executed some time later, but I
> don't see how that can be.

Can you explain how this would lead to a security issue? (use after free, accessing out of bounds memory, etc)

Comment 26

[Glenn Randers-Pehrson]

The test image crashes my Google Chrome browser on Ubuntu 14:04LTS.  Screen goes black, tom-tom sounds, then the only thing still working is the power button.  So it does appear to be a security issue.  I don't know if chrome is using libpng-1.6 now.

Comment 27

[Glenn Randers-Pehrson]

I'm not positive that the chrome crash was related to this test.  It showed the image, then when I hit the back arrow it crashed.

Comment 28

[u153838]

(In reply to Timothy Nikkel (:tnikkel) from comment #25)
> Crashes and asserts are not security issues on their own.

If it's inside libpng it ends up as a security issue.  After all, there's not a lot of motivation for the app writer or bug seeker to downplay it.

> Yes, the info callback is not called, when Firefox is doing a size-only
> decode. [* * *] The info callback isn't called, and libpng keeps decoding,
> it hits the end of the png and calls the end callback.

So that *is* a security issue.  We know what your app does and we know that the behavior is highly variable (on some platforms it crashes, on some it aborts (asserts), on some it just keeps on truckin.)  The mis-behavior is inside libpng however the bug of assuming that function 'a' has been called by the time function 'b' is called is such a common, easy and obviously hackable error that it's hacker heaven when it is discovered.

Glenn has now detected a similar issue with Chrome, indeed it sounds like it might be a memory-trasher.  The image produces a bad potential DNS issue in a program distributed with libpng; pngfix, though that is almost certainly a bug in pngfix itself.  Once this bug becomes public hackers need only search their list of apps, and other libraries, that use the progressive reader then examine what happens in the 'info' callback and compare it with what happens in the 'end' and, if it is called, the 'row' callback (I am pretty sure it is called as well.)

Anyway, this is the software, I assume it is an unhacked image:

http://www.getpaint.net/index.html

The image is identified as 'paint.net 4.0', but they may not include the minor version number.  The behavior is consistent with something the Microsoft PNG code used to do (and maybe still does) which is to try to align the IDAT chunks with page boundaries and given the compression setting it sure looks like a slightly damaged version of the original MS code (i.e. paint.net is using the OS service).  It's certainly not a bug to write a zero length IDAT at the start; if an app is trying to align the image data it needs to make IDAT offset + 8 be a multiple of a page boundary, sometimes this is impossible, sometimes it requires exactly 12 bytes ;-)

Comment 29

[Glenn Randers-Pehrson]

We are ready to take Tim's patch, as is, in libpng16. That will of course disclose the bug.  Tim, do you wish to have the "blame" for the patch to GIT?  i.e., should I run git commit --author "Timothy Nikkel <[e-mail]>?

Comment 30

[Timothy Nikkel (:tnikkel)]

Sure, you cab blame me :)

Comment 31

[Glenn Randers-Pehrson]

Removed "malformed" from title (see my comment #18).

Comment 32

[Glenn Randers-Pehrson]

This bug has been disclosed in libpng-1.6.23beta01 which was released today.

Comment 33

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8757146 [details] [diff] [review]
patch

This is public now, and I don't think this exposes a security bug in firefox, but I'll fill out the sec-approval just in case.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
I don't think this bug is exploitable in Firefox

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
yes

Which older supported branches are affected by this flaw?
all of them

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
trivial

How likely is this patch to cause regressions; how much testing does it need?
pretty safe

Comment 34

[shekyan]

(In reply to Timothy Nikkel (:tnikkel) from comment #33)
> Comment on attachment 8757146 [details] [diff] [review]

> Which older supported branches are affected by this flaw?
> all of them

Not really. Bug was introduced in Firefox 42.0, earlier versions (tried 41, 40) are not affected (tested on OS X).

Comment 35

[:Gijs (out for now; he/him)]

(In reply to shekyan from comment #34)
> (In reply to Timothy Nikkel (:tnikkel) from comment #33)
> > Comment on attachment 8757146 [details] [diff] [review]
> 
> > Which older supported branches are affected by this flaw?
> > all of them
> 
> Not really. Bug was introduced in Firefox 42.0, earlier versions (tried 41,
> 40) are not affected (tested on OS X).

Note "supported" in that sentence. 45 ESR is the oldest supported thing for which more releases are still happening, so in that sense all supported branches are indeed affected.

Comment 36

[shekyan]

(In reply to :Gijs Kruitbosch from comment #35)
> (In reply to shekyan from comment #34)
> > (In reply to Timothy Nikkel (:tnikkel) from comment #33)
> > > Comment on attachment 8757146 [details] [diff] [review]
> > 
> > > Which older supported branches are affected by this flaw?
> > > all of them
> > 
> > Not really. Bug was introduced in Firefox 42.0, earlier versions (tried 41,
> > 40) are not affected (tested on OS X).
> 
> Note "supported" in that sentence. 45 ESR is the oldest supported thing for
> which more releases are still happening, so in that sense all supported
> branches are indeed affected.

Oh, got it.

Comment 37

[Al Billings [:abillings - ex-MoCo]]

Calling this a sec-low and clearing sec-approval. Just check it into trunk.

Comment 38

[Timothy Nikkel (:tnikkel)]

Actually, I'll probably just let this land when the next version of libpng lands in our repo, it should have this fix.

Comment 39

[Glenn Randers-Pehrson]

I expect to release libpng-1.6.23 on June 9th and I'll prepare a patch the same day.

Comment 40

[Glenn Randers-Pehrson]

Comment on attachment 8757146 [details] [diff] [review]
patch

This patch was included in bug #1275901 (update to libpng-1.6.23) which has landed.

Comment 41

[Timothy Nikkel (:tnikkel)]

Fixed by landing this patch via bug 1275901.