759802 - (CVE-2012-2806) Null-pointer execution/null out of bounds write at libjpeg/jdmarker.c

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[Atte Kettunen]

Attachment: Repro-file

Repro-file as attachment.

Crash-report:

https://crash-stats.mozilla.com/report/index/bp-7fbc775d-2fc4-46ff-9af8-b8a0f2120530

This issue seems to affect also Google Chrome and I have already reported it to Google. They have given me permission to report it also for Mozilla.

Comment 1

[Atte Kettunen]

Also the guys at Google would like to know, is your embedded libjpeg the turbo variety or plain variety and if so which version, 6 or 8?

Comment 2

[Joe Drew (not getting mail)]

Comment on attachment 628366 [details]
Repro-file

It's way too easy to crash Firefox when we load this image.

Comment 3

[Joe Drew (not getting mail)]

We use jpeg-turbo in Firefox, version v1.2.0.

Comment 4

[Justin Lebar (not reading bugmail)]

> This issue seems to affect also Google Chrome and I have already reported it to Google. 

Is there a Google bug report you can link us to?

cc DRC, the libjpeg-turbo maintainer.

Comment 5

[Atte Kettunen]

The bug at Chrome issue-list is currently restricted view SecurityTeam. I relay your message to there.

Comment 6

[Justin Lebar (not reading bugmail)]

> Attachment mime type: image/gif → application/octet-stream

In case it's not clear to others, STR is: save this file to disk, give it a .jpg file extension, and open it in Firefox.

Comment 7

[DRC]

This is reproducible in libjpeg-turbo at the low level, and it seems to be specific to -turbo (libjpeg just reports "unsupported data precision 128" and dies gracefully.)

Investigating.

Comment 8

[Atte Kettunen]

DRC: I said to Chris Evans that he should CC you into the Chrome Issue. He thinks that you should co-ordinate with this one.

Some one could also CC Chris Evans into here.

Comment 9

[Justin Lebar (not reading bugmail)]

There are a bunch of Chris Evans'es in bugzilla.  I think the one we want is scarybeasts, based on the fact that [1] links to [2].

Can anyone confirm that this is the person we want?  If not does anyone mind if we cc this potentially-random-but-probably-not person to this bug?

[1] http://www.google.com/about/company/rewardprogram.html
[2] http://scarybeastsecurity.blogspot.com/2010/01/logout-xsrf-significant-web-app-bug.html

Comment 10

[Atte Kettunen]

:D Sorry if I was unclear. scarybeasts([e-mail]) is the one.

Comment 11

[Justin Lebar (not reading bugmail)]

His bugmail account is at gmail (I didn't want to type the full e-mail in here for spambots to feast upon), but that's good enough for me!

Comment 12

[DRC]

Fix checked into trunk and branches/1.2.x in libjpeg-turbo SVN repository.

Comment 13

[Atte Kettunen]

From Chrome Issue-list:

Comment 20 by [e-mail], Today (19 minutes ago)
"CVE-2012-2806

If you could share the CVE with Mozilla, that'd help prevent confusion."

Comment 14

[Atte Kettunen]

Attachment: Second repro-file

Adding second repro-file. This repro-file doesn't cause null-exec instead it is heap-buffer-overflow - Write.

I think that this crash is because of the same bug because the two frames get_sos and consume_markers are the same I catched previously from Chrome with the first repro-file

==30145== ERROR: AddressSanitizer heap-buffer-overflow on address 0x7f19a0217d48 at pc 0x7f19b997e4b0 bp 0x7fffb009d3d0 sp 0x7fffb009d3c8
WRITE of size 8 at 0x7f19a0217d48 thread T0
    #0 0x7f19b997e4b0 in get_sos /home/attekett/firefox/src/media/libjpeg/jdmarker.c:327
    #1 0x7f19b99775b5 in consume_markers /home/attekett/firefox/src/media/libjpeg/jdinput.c:386
0x7f19a0217d48 is located 0 bytes to the right of 1224-byte region [0x7f19a0217880,0x7f19a0217d48)
allocated by thread T0 here:
    #0 0x41b732 in malloc ??:0
    #1 0x7f19be3d5398 in moz_xmalloc /home/attekett/firefox/src/memory/mozalloc/mozalloc.cpp:54
    #2 0x7f19b703aadf in mozilla::image::RasterImage::RequestDecode() /home/attekett/firefox/src/image/src/RasterImage.cpp:2468
==30145== ABORTING
Stats: 237M malloced (219M for red zones) by 335211 calls
Stats: 53M realloced by 14995 calls
Stats: 154M freed by 180561 calls
Stats: 19M really freed by 29317 calls
Stats: 480M (122954 full pages) mmaped in 111 calls
  mmaps   by size class: 8:262128; 9:57337; 10:20475; 11:18423; 12:3072; 13:2048; 14:1536; 15:512; 16:896; 17:160; 18:192; 19:64; 20:20; 21:8; 22:4; 23:3; 24:2;
  mallocs by size class: 8:239994; 9:54474; 10:16246; 11:16819; 12:2449; 13:1971; 14:1524; 15:445; 16:840; 17:147; 18:197; 19:70; 20:19; 21:7; 22:4; 23:3; 24:2;
  frees   by size class: 8:112131; 9:38221; 10:11931; 11:13397; 12:1454; 13:1037; 14:1270; 15:348; 16:494; 17:120; 18:75; 19:61; 20:14; 21:4; 23:2; 24:2;
  rfrees  by size class: 8:21204; 9:2221; 10:2095; 11:3157; 12:163; 13:205; 14:120; 15:41; 16:62; 17:21; 18:17; 19:10; 20:1;
Stats: malloc large: 449 small slow: 2070
Shadow byte and word:
  0x1fe334042fa9: fb
  0x1fe334042fa8: 00 fb fb fb fb fb fb fb
More shadow bytes:
  0x1fe334042f88: 00 00 00 00 00 00 00 00
  0x1fe334042f90: 00 00 00 00 00 00 00 00
  0x1fe334042f98: 00 00 00 00 00 00 00 00
  0x1fe334042fa0: 00 00 00 00 00 00 00 00
=>0x1fe334042fa8: 00 fb fb fb fb fb fb fb
  0x1fe334042fb0: fa fa fa fa fa fa fa fa
  0x1fe334042fb8: fa fa fa fa fa fa fa fa
  0x1fe334042fc0: fa fa fa fa fa fa fa fa
  0x1fe334042fc8: fa fa fa fa fa fa fa fa

Comment 15

[DRC]

Does the patch I committed fix the second problem as well?

Comment 16

[Atte Kettunen]

I can't verify if the patch fixes the second files issue. It should. I can verify it monday at best.

Comment 17

[Atte Kettunen]

Attachment: Real second repro-file

Seems like I uploaded wrong file by accident. This should be the correct file.

Comment 18

[Justin Lebar (not reading bugmail)]

I've posted patches in bug 759891.  Assuming they look good on try, we should be able to get this in tomorrow.  (It's not like anybody is reviewing this code.)

Comment 19

[Justin Lebar (not reading bugmail)]

(In reply to Atte Kettunen from comment #16)
> I can't verify if the patch fixes the second files issue. It should. I can
> verify it monday at best.

Would definitely appreciate this.

Comment 20

[Atte Kettunen]

Oh. Sorry. I'll build new version now. Hanging on laptop still so can take some time.

Comment 21

[DRC]

Note: pending resolution of this, I intend to release the current 1.2.x branch as 1.2.1.

Would be nice to get some confirmation of the fix from Chrome as well.  They haven't added me to the bug on their end, so I have no idea what the status is.

Comment 22

[Atte Kettunen]

They pulled newest version of libjpeg-turbo and the bug is marked as Fixed. I haven't seen any crashes related to this issue on Chrome since the fix was release.

Comment 23

[Atte Kettunen]

The faulty version was not been in the Chrome source long so the bug was only in dev-channel.

Comment 24

[Atte Kettunen]

I'm unable to build ASAN-firefox atm. I get odd "Bus error" from clang. I'll try to investigate it further.

Comment 25

[Atte Kettunen]

Seems to work for me. Repro-files I uploaded cause no error on ASAN-build with updated libjpeg-turbo downloaded with "svn co https://libjpeg-turbo.svn.sourceforge.net/svnroot/libjpeg-turbo/trunk"

Comment 26

[DRC]

You should be using branches/1.2.x, not trunk, since trunk contains evolving (not necessarily well-tested) code.

Comment 27

[Atte Kettunen]

I'll try with that one too.

Comment 28

[Atte Kettunen]

Seems to work also with "svn co https://libjpeg-turbo.svn.sourceforge.net/svnroot/libjpeg-turbo/branches/1.2.x"

Comment 29

[Justin Lebar (not reading bugmail)]

Attachment: Just the r830 --> r831 change.

Comment 30

[Justin Lebar (not reading bugmail)]

Comment on attachment 630252 [details] [diff] [review]
Just the r830 --> r831 change.

This looks pretty safe to take on branches to me.  That is, after looking at the diff, I think my concerns in bug 759891 comment 11 were unfounded.

Comment 31

[Joe Drew (not getting mail)]

Comment on attachment 630252 [details] [diff] [review]
Just the r830 --> r831 change.

Review of attachment 630252 [details] [diff] [review]:
-----------------------------------------------------------------

shipit

Comment 32

[Justin Lebar (not reading bugmail)]

Comment on attachment 630252 [details] [diff] [review]
Just the r830 --> r831 change.

[Approval Request Comment]
Security fix.  No string changes.

I don't know what the process is for landing changes on the release channel, and I'm not familiar with the bar we set for those changes.  But this looks like a relatively easily exploitable vulnerability to me, so I'd like us to consider it for the release channel.

Comment 33

[Daniel Veditz [:dveditz]]

Rated this "critical" assuming the worst, but are the values ever going to be anything other than null? That wouldn't be so bad.

Comment 34

[Daniel Veditz [:dveditz]]

Rated this "critical" assuming the worst, but are the values ever non-null? executing null itself isn't so bad.

Comment 35

[DRC]

The specifics of this bug are that jdmarker.c:326 was attempting to zero out the current component info pointers:

  for (i = 0; i < cinfo->num_components; i++)
    cinfo->cur_comp_info[i] = NULL;

Due to a corrupt header in the JPEG file, num_components was being set to a fairly ridiculous value (73, in the case of one of the examples.)  Unfortunately, the section of code that checks for said ridiculous value is executed after the code in jdmarker.c, so what was happening was that memory was being zeroed out to the end of the jpeg_decompress_struct and beyond.  It didn't cause an access violation, perhaps because there was another structure allocated immediately following on the heap.  However, all of the decompression subobject pointers in jpeg_decompress_struct were now set to NULL.

Following the above code is a loop:

  for (i = 0; i < n; i++) {
    INPUT_BYTE(cinfo, cc, return FALSE);
    INPUT_BYTE(cinfo, c, return FALSE);
    
    for (ci = 0, compptr = cinfo->comp_info; ci < cinfo->num_components;
         ci++, compptr++) {
      if (cc == compptr->component_id && !cinfo->cur_comp_info[ci])
        goto id_found;
    }

    ERREXIT1(cinfo, JERR_BAD_COMPONENT_ID, cc);

  id_found:

    cinfo->cur_comp_info[i] = compptr;
    compptr->dc_tbl_no = (c >> 4) & 15;
    compptr->ac_tbl_no = (c     ) & 15;
    
    TRACEMS3(cinfo, 1, JTRC_SOS_COMPONENT, cc,
	     compptr->dc_tbl_no, compptr->ac_tbl_no);
  }

n is guaranteed to be < MAX_COMP_SCANS, due to a check earlier in the function.  Thus, although the above loop will erroneously try to read past the cur_comp_info pointers and past the end of the structure, it will not write past the boundaries of the cur_comp_info pointers, and thus the NULL subobject pointers will still be NULL.

To summarize, in my opinion, the bug will not ever cause a non-NULL pointer to be executed.

Comment 36

[Alex Keybl [:akeybl]]

(In reply to Justin Lebar [:jlebar] from comment #32)
> Comment on attachment 630252 [details] [diff] [review]
> Just the r830 --> r831 change.
> 
> [Approval Request Comment]
> Security fix.  No string changes.
> 
> I don't know what the process is for landing changes on the release channel,
> and I'm not familiar with the bar we set for those changes.  But this looks
> like a relatively easily exploitable vulnerability to me, so I'd like us to
> consider it for the release channel.

Dan wontfix'd this for 13, but we'll track nonetheless in case we take ride-alongs with a possible 13.0.1. Approving for Aurora 15, Beta 14 and the ESR, however.

Comment 37

[Justin Lebar (not reading bugmail)]

Landed on Aurora for FF15: https://hg.mozilla.org/releases/mozilla-aurora/rev/95cbdaa73a16

Comment 38

[Daniel Veditz [:dveditz]]

(In reply to DRC from comment #35)
> To summarize, in my opinion, the bug will not ever cause a non-NULL pointer
> to be executed.

That's what it looked like to me, too. Although in at least one case I've seen the ability to write a single null on a heap structure pointer led to code execution when the heap buffer was freed.

Comment 39

[Atte Kettunen]

moderate?

According to https://wiki.mozilla.org/Security_Severity_Ratings

sec-critical Examples:
"Any crash where random memory or NULL is executed (the top of the stack is not a function) "

Old information or wrong interpretation?

Comment 40

[Justin Lebar (not reading bugmail)]

Well, comment 14 demonstrates that they can write 0 onto the heap, right?  Heap overflow is potentially critical (although it's surely difficult to exploit!).

Comment 41

[Justin Lebar (not reading bugmail)]

Comment on attachment 630252 [details] [diff] [review]
Just the r830 --> r831 change.

Landed on beta for FF14: https://hg.mozilla.org/releases/mozilla-beta/rev/d835833d44a0

Unfortunately ESR is still running libjpeg-turbo v1.1.0 (r469), and this patch does not apply cleanly.

I *think* I see how to modify the patch, but it's not clear to me that v1.1.0 doesn't have other unfixed security vulnerabilities.

Comment 42

[Justin Lebar (not reading bugmail)]

For ESR, I think we'd need

r740: http://libjpeg-turbo.svn.sourceforge.net/viewvc/libjpeg-turbo/trunk/jdmarker.c?view=patch&r1=740&r2=739&pathrev=740

r751: http://libjpeg-turbo.svn.sourceforge.net/viewvc/libjpeg-turbo/branches/1.2.x/jdmarker.c?view=patch&r1=751&r2=750&pathrev=751

r831: (patch in this bug)

But it would be good to know if there are other known security vulnerabilities in 1.1.0; otherwise we're not accomplishing much by taking this one change.

Comment 43

[Justin Lebar (not reading bugmail)]

Attachment: Patch for ESR

Comment 44

[DRC]

Hi, folks.  Please note that this security vulnerability did not exist in 1.1.0.  It was introduced by r740 in 1.2.0.  Thus, all you're doing above is introducing the vulnerability into a code base that was already secure, then you're introducing a new patch to fix the vulnerability that you just introduced.  In other words, you're swallowing the spider to catch the fly, but I don't know why you swallowed the fly.

The only issue I see in the changelog that you might want to look at for patching 1.1.0 is this one:
http://crbug.com/72399
I'm not sure if it's a security vulnerability or not.  It never created a segfault or any visible run-time issue.  It was mainly just that it made valgrind unhappy.

You might want to scan the changelog yourself and double-check to see if any other post-1.1.0 changes stand out as possible security vulnerabilities, but I just scanned it myself and didn't notice anything.

Comment 45

[Justin Lebar (not reading bugmail)]

Comment on attachment 631069 [details] [diff] [review]
Patch for ESR

Thanks, DRC.

Comment 46

[Alex Keybl [:akeybl]]

(In reply to DRC from comment #44)
> Hi, folks.  Please note that this security vulnerability did not exist in
> 1.1.0.  It was introduced by r740 in 1.2.0.  Thus, all you're doing above is
> introducing the vulnerability into a code base that was already secure, then
> you're introducing a new patch to fix the vulnerability that you just
> introduced.  In other words, you're swallowing the spider to catch the fly,
> but I don't know why you swallowed the fly.
> 
> The only issue I see in the changelog that you might want to look at for
> patching 1.1.0 is this one:
> http://crbug.com/72399
> I'm not sure if it's a security vulnerability or not.  It never created a
> segfault or any visible run-time issue.  It was mainly just that it made
> valgrind unhappy.
> 
> You might want to scan the changelog yourself and double-check to see if any
> other post-1.1.0 changes stand out as possible security vulnerabilities, but
> I just scanned it myself and didn't notice anything.

Jason - Given this, can we back out the patch in attachment 630252 [details] [diff] [review]? It appears to offer little benefit on FF14/FF15, and only additional risk.

Comment 47

[Daniel Veditz [:dveditz]]

(In reply to Atte Kettunen from comment #39)
> According to https://wiki.mozilla.org/Security_Severity_Ratings
> 
> sec-critical Examples:
> "Any crash where random memory or NULL is executed (the top of the stack is
> not a function) "
> 
> Old information or wrong interpretation?

a little of both. /Some/ crashes where null is executed can be proved to always be null, and those are not sec-critical. Many crashes where null is executed is because memory is messed up, and maybe in other conditions it could be an exploitable non-null. So "Any" is incorrect, but it's common.

In other stacks "random" addresses on the stack are due to JS JITted code, it's not really random so we have to distinguish those cases (but, crashing JITted code is often bad anyway). That's not the case here.

> moderate?

No, if this is exploitable it's sec-critical, if it's not then it's a DoS. We don't use a "Schrodinger's Cat Moderate" indeterminate state.

Comment 48

[DRC]

> Jason - Given this, can we back out the patch in attachment 630252 [details] [diff] [review]
> [diff] [review]? It appears to offer little benefit on FF14/FF15, and only
> additional risk.

It offers no benefit to Firefox AFAIK.  r740 was introduced in response to a request from the SumatraPDF developers, because apparently PDF files generated by a certain popular content creation application had incorrectly-encoded CMYK JPEG files embedded in them.  Adobe Acrobat rendered the PDF's correctly, so it was desirable for SumatraPDF to also render the PDF's correctly so users would not accuse them of incompatibility.  I introduced the "fix" in libjpeg-turbo (which is really less of a fix and more of a relaxing of the rules) in order to lend my support to another open source project, but the "fix" created the regression that led to this bug report.

Comment 49

[Justin Lebar (not reading bugmail)]

> Jason - Given this, can we back out the patch in attachment 630252 [details] [diff] [review] [diff] [review]? It 
> appears to offer little benefit on FF14/FF15, and only additional risk.

FF14/FF15 are both on libjpeg 1.2.x branch, and were vulnerable to this exploit.  ESR is on the 1.1.x branch and is not vulnerable.

Comment 50

[Justin Lebar (not reading bugmail)]

FF13 is running the 1.1.x branch, so is unaffected.

Comment 51

[Daniel Veditz [:dveditz]]

According to Atte, Chris Evans commented in the Chrome bug "You could corrupt any heap object with NULLs, by arranging for a heap object of your choice to appear after the overflowed jpeg_decompress_struct. There's a huge number of classes to choose from and I'm sure one of them does not react well to have its state corrupted with NULLs."  -- True enough. Wouldn't be easy, but we've seen it done when the motivation is high enough.

Comment 54

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

This is marked fixed for 14 and 15 but not mozilla-central? I assume an oversight here?

Comment 55

[Justin Lebar (not reading bugmail)]

(In reply to Al Billings [:abillings] from comment #54)
> This is marked fixed for 14 and 15 but not mozilla-central? I assume an
> oversight here?

Bug 759891 fixed this issue on m-c, so yes, we should mark this as fixed.

Comment 56

[u279076]

Crash reproduced with the attached testcases using Firefox 14.0a2 2012-05-21.

Firefox 14.0b11 does not crash.
Firefox 15.0a2 2012-07-11 does not crash.
Firefox 16.0a1 2012-07-11 does not crash.

Comment 57

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

Does anyone know if Chrome has shipped this fix yet or do we need to embargo disclosure until a later date (when they do)?

Comment 58

[Atte Kettunen]

The Chrome issue is marked as fixed. The issue was catched at dev-channel and never reached stable on Chrome.

Comment 59

[Atte Kettunen]

The Chrome issue is marked as fixed. The issue was catched at dev-channel and never reached stable on Chrome.

Comment 60

[Daniel Veditz [:dveditz]]

See comment 50, this was caught (thanks to you!) in Firefox never reached our "stable", either, although the bug did make it to the Beta channel.

Comment 61

[Raymond Forbes[:rforbes]]

rforbes-bugspam-for-setting-that-bounty-flag-20130719