Closed
Bug 1184009
(CVE-2015-4491)
Opened 9 years ago
Closed 9 years ago
gdk-pixbuf heap overflow and DoS affecting Firefox
Categories
(Core :: Widget: Gtk, defect)
Tracking
()
VERIFIED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox39 | --- | wontfix |
firefox40 | - | wontfix |
firefox41 | + | verified |
firefox42 | + | fixed |
firefox43 | --- | fixed |
firefox-esr31 | --- | wontfix |
firefox-esr38 | 41+ | fixed |
b2g-v2.0 | --- | unaffected |
b2g-v2.0M | --- | unaffected |
b2g-v2.1 | --- | unaffected |
b2g-v2.1S | --- | unaffected |
b2g-v2.2 | --- | unaffected |
b2g-v2.2r | --- | unaffected |
b2g-master | --- | unaffected |
People
(Reporter: gustavo.grieco, Assigned: lsalzman)
References
Details
(Keywords: csectype-bounds, sec-high, Whiteboard: [adv-main40+][adv-esr38.2+][adv-main41-][adv-esr38.3-])
Attachments
(4 files, 1 obsolete file)
(deleted),
application/x-tar
|
Details | |
(deleted),
patch
|
acomminos
:
review+
lmandel
:
approval-mozilla-aurora+
lmandel
:
approval-mozilla-beta+
lmandel
:
approval-mozilla-esr38+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
(deleted),
application/x-gzip
|
Details | |
(deleted),
patch
|
acomminos
:
review+
RyanVM
:
checkin+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:39.0) Gecko/20100101 Firefox/39.0
Build ID: 20150629114848
Steps to reproduce:
I'm forwarding this bug because it clearly affects last versions of Firefox. This was reported to upstream here (https://bugzilla.gnome.org/show_bug.cgi?id=752297) and it is supposed to be fixed (i haven't tested the patch yet).
Requirements:
* Linux x86_64
* gdk-pixbuf 2.31 or newer (you can upgrade only this package in Ubuntu/Debian download and installing from here: https://packages.debian.org/sid/libgdk-pixbuf2.0-0 or http://packages.ubuntu.com/vivid/libgdk-pixbuf2.0-0). Older versions are more difficult to exploit, but the issue is still there.
* At least 10GB of real/virtual memory (a big swapfile will do the trick if you don't have enough physical memory).
Steps to reproduce:
1. Download and decompress firefox-overflow.bmp.gz (it is compressed to avoid crashing my own browser!)
2. Try to open (ctrl+O) or attach firefox-overflow.bmp
3. Boom
Actual results:
Firefox crashes inside gdk-pixbuf code. Details:
Program received signal SIGSEGV, Segmentation fault.
make_filter_table (filter=0x7ffffffead10) at pixops.c:1294
1294 pixops.c: No such file or directory.
(gdb) info registers
rax 0x0 0
rbx 0x2 2
rcx 0x0 0
rdx 0x0 0
rsi 0x7fffbae00000 140736328630272
rdi 0x7fffbcfc8d00 140736364055808
rbp 0x7ffffffead10 0x7ffffffead10
rsp 0x7ffffffeab20 0x7ffffffeab20
r8 0x2b0000 2818048
r9 0x0 0
r10 0x4000010 67108880
r11 0x7fffbeb5000c 140736392921100
r12 0x8 8
r13 0x1 1
r14 0x1000003 16777219
r15 0x7fffbc436c00 140736351923200
rip 0x7fffedf57239 0x7fffedf57239 <pixops_process+681>
eflags 0x10246 [ PF ZF IF RF ]
cs 0x33 51
ss 0x2b 43
ds 0x0 0
es 0x0 0
fs 0x0 0
gs 0x0 0
(gdb) x/i $rip
=> 0x7fffedf57239 <pixops_process+681>: mov %edx,(%rsi,%rax,4)
Here %rsi depens on the dimension of the malformed bmp.
Expected results:
Of course, Firefox shouldn't so easily be exploitable. Nevertheless, it is not clear how Firefox can prevent this kind of vulnerabilities in the future (unless your re-implement gdk-pixbuf functions)
Reporter | ||
Updated•9 years ago
|
OS: Unspecified → Linux
Hardware: Unspecified → x86_64
Updated•9 years ago
|
Component: Untriaged → Widget: Gtk
Product: Firefox → Core
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → lsalzman
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee | ||
Comment 2•9 years ago
|
||
This patch does some overflow checking to verify that the filter table created within gdk-pixbuf won't overflow on allocation when trying to scale the image. Due to the fact that the upstream "fix" for the issue merely makes the allocation always fail in such cases anyway, it is sufficient for us to merely do the check on our end so that we're not dependent on users having the upstream library fixed.
[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
The patch doesn't explicit mention the issue with the overflow, but just does the size check, which by itself shouldn't highlight the possible exploit.
> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No. However, there is an upstream bug report filed on the issue, so that this issue is unfortunately known beyond our bug reporting system. I would recommend we patch this ASAP.
> Which older supported branches are affected by this flaw?
This affects branches as far back as at least ESR 31, that I can see. Pretty much anything that uses "gdk_pixbuf_new_from_file_at_size". The problem is not in our code, per se, but in vulnerabilities introduced in gdk-pixbuf, so even our code which may have previously been safe became unsafe due to the upstream issue.
> If not all supported branches, which bug introduced the flaw?
We appear to have started using this gdk-pixbuf feature in this changeset: https://hg.mozilla.org/releases/mozilla-esr31/rev/e92be8bbe5ef
> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
This patch should work with older branches. At worst, it will disable previews on large images, but only in cases where crashes/overflows would have normally occurred.
> How likely is this patch to cause regressions; how much testing does it need?
This shouldn't cause any notable regressions other than as noted - in cases where a crash or exploit might otherwise be observed, instead a lack of a preview image will be observed. This change at least blocks the reported upstream vulnerability as far as my own testing indicated.
Attachment #8636664 -
Flags: sec-approval?
Attachment #8636664 -
Flags: review?(acomminos)
Comment 3•9 years ago
|
||
Comment on attachment 8636664 [details] [diff] [review]
limit image preview sizes
Review of attachment 8636664 [details] [diff] [review]:
-----------------------------------------------------------------
::: widget/gtk/nsFilePicker.cpp
@@ +105,4 @@
> // Only scale down images that are too big
> if (preview_width > MAX_PREVIEW_SIZE || preview_height > MAX_PREVIEW_SIZE) {
> + if (ceil(preview_width / double(MAX_PREVIEW_SIZE) + 1.0) *
> + ceil(preview_height / double(MAX_PREVIEW_SIZE) + 1.0) < 0x7FFFFF) {
It's probably a good idea to add a comment briefly explaining why this is done. I'm okay with the magic number use as it's not really an intuitive quantity.
A check for affected GDK pixbuf versions using gdk_pixbuf_*_version may make sense as well here, and provide a bit of context.
Attachment #8636664 -
Flags: review?(acomminos) → review+
Assignee | ||
Comment 4•9 years ago
|
||
(In reply to Andrew Comminos [:acomminos] from comment #3)
> Comment on attachment 8636664 [details] [diff] [review]
> limit image preview sizes
>
> Review of attachment 8636664 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: widget/gtk/nsFilePicker.cpp
> @@ +105,4 @@
> > // Only scale down images that are too big
> > if (preview_width > MAX_PREVIEW_SIZE || preview_height > MAX_PREVIEW_SIZE) {
> > + if (ceil(preview_width / double(MAX_PREVIEW_SIZE) + 1.0) *
> > + ceil(preview_height / double(MAX_PREVIEW_SIZE) + 1.0) < 0x7FFFFF) {
>
> It's probably a good idea to add a comment briefly explaining why this is
> done. I'm okay with the magic number use as it's not really an intuitive
> quantity.
>
> A check for affected GDK pixbuf versions using gdk_pixbuf_*_version may make
> sense as well here, and provide a bit of context.
I was afraid to add details like this because it gives too much information about how to exploit the issue.
Comment 5•9 years ago
|
||
sec-approval+ for trunk. We should have branch patches for Aurora, Beta, and ESR38 created and nominated so they can go in once trunk is green.
status-firefox39:
--- → wontfix
status-firefox40:
--- → affected
status-firefox41:
--- → affected
status-firefox42:
--- → affected
status-firefox-esr31:
--- → wontfix
status-firefox-esr38:
--- → affected
tracking-firefox40:
--- → +
tracking-firefox41:
--- → +
tracking-firefox42:
--- → +
tracking-firefox-esr38:
--- → 40+
Updated•9 years ago
|
Attachment #8636664 -
Flags: sec-approval? → sec-approval+
Assignee | ||
Comment 6•9 years ago
|
||
Try results for patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7236168282db
Keywords: checkin-needed
Comment 7•9 years ago
|
||
Keywords: checkin-needed
Comment 8•9 years ago
|
||
(In reply to Al Billings [:abillings] from comment #5)
> sec-approval+ for trunk. We should have branch patches for Aurora, Beta, and
> ESR38 created and nominated so they can go in once trunk is green.
I don't see any Aurora, Beta, or ESR38 patches here. Is there a reason they weren't made and nominated?
Flags: needinfo?(lsalzman)
Assignee | ||
Comment 9•9 years ago
|
||
(In reply to Al Billings [:abillings] from comment #8)
> (In reply to Al Billings [:abillings] from comment #5)
> > sec-approval+ for trunk. We should have branch patches for Aurora, Beta, and
> > ESR38 created and nominated so they can go in once trunk is green.
>
> I don't see any Aurora, Beta, or ESR38 patches here. Is there a reason they
> weren't made and nominated?
The patch should work for all of them.
Flags: needinfo?(lsalzman)
Comment 10•9 years ago
|
||
Ok. Well, generally, you, as the assignee, need to nominate them for inclusion or they won't get approved and go in on the branches.
Updated•9 years ago
|
Attachment #8636664 -
Flags: approval-mozilla-esr38?
Attachment #8636664 -
Flags: approval-mozilla-beta?
Attachment #8636664 -
Flags: approval-mozilla-aurora?
Updated•9 years ago
|
Flags: sec-bounty?
Comment 11•9 years ago
|
||
Comment on attachment 8636664 [details] [diff] [review]
limit image preview sizes
This has already landed on inbound. Better get it on the branches. Beta+ Aurora+ ESR38+
Attachment #8636664 -
Flags: approval-mozilla-esr38?
Attachment #8636664 -
Flags: approval-mozilla-esr38+
Attachment #8636664 -
Flags: approval-mozilla-beta?
Attachment #8636664 -
Flags: approval-mozilla-beta+
Attachment #8636664 -
Flags: approval-mozilla-aurora?
Attachment #8636664 -
Flags: approval-mozilla-aurora+
Comment 12•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/43e50f129af4
https://hg.mozilla.org/releases/mozilla-aurora/rev/e67096569716
https://hg.mozilla.org/releases/mozilla-beta/rev/9b861360b297
https://hg.mozilla.org/releases/mozilla-esr38/rev/c9d5dc392dde
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-b2g-v2.1S:
--- → unaffected
status-b2g-v2.2:
--- → unaffected
status-b2g-v2.2r:
--- → unaffected
status-b2g-master:
--- → unaffected
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Updated•9 years ago
|
Alias: CVE-2015-4491
Whiteboard: [adv-main40+][adv-esr38.2+]
Updated•9 years ago
|
Flags: qe-verify+
Updated•9 years ago
|
Flags: sec-bounty? → sec-bounty+
Comment 13•9 years ago
|
||
While following STR via comment 0, I got stuck at installing gdk-pixbuf 2.31 toolkit; encountered various errors, including to install some circular dependency and this broke the OS (Ubuntu 14.04 x64). Since I am not able to reproduce/verify this issue here, Gustavo Grieco could you please confirm if this issue is fixed on 40.0 RC [1] and ESR 38.1.1 [2] builds? Thanks in advance!
[1] http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/40.0-candidates/build2/
[2] http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/38.1.1esr-candidates/build2/
Flags: needinfo?(gustavo.grieco)
Reporter | ||
Comment 14•9 years ago
|
||
(In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #13)
> While following STR via comment 0, I got stuck at installing gdk-pixbuf 2.31
> toolkit; encountered various errors, including to install some circular
> dependency and this broke the OS (Ubuntu 14.04 x64).
I know, it almost happened to me.
> Since I am not able to reproduce/verify this issue here, Gustavo Grieco could you please confirm if
> this issue is fixed on 40.0 RC [1] and ESR 38.1.1 [2] builds? Thanks in
> advance!
Sure thing. I have the vulnerable packages installed at my laptop at home. I will be able to test it but later today. Is it ok?
>
> [1]
> http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/40.0-candidates/
> build2/
> [2]
> http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/38.1.1esr-candidates/
> build2/
Flags: needinfo?(gustavo.grieco)
Reporter | ||
Comment 15•9 years ago
|
||
I'm affraid it is still aborting:
Program received signal SIGTRAP, Trace/breakpoint trap.
0x00007fffef212c13 in g_logv () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
(gdb) bt
#0 0x00007fffef212c13 in g_logv () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#1 0x00007fffef212d72 in g_log () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#2 0x00007fffef211644 in g_malloc () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#3 0x00007fffedf1f0ff in make_filter_table (filter=0x7ffffffead20) at pixops.c:1275
#4 pixops_process (dest_buf=0x7fffc58a3800 'Z' <repeats 200 times>..., render_x0=0, render_y0=0, render_x1=180, render_y1=1, dest_rowstride=540,
dest_channels=3, dest_has_alpha=0, src_buf=0x7fff38f00000 "", src_width=2097181, src_height=341, src_rowstride=6291544, src_channels=3,
src_has_alpha=0, scale_x=<optimized out>, scale_y=<optimized out>, check_x=0, check_y=0, check_size=0, color1=0, color2=0, filter=0x7ffffffead20,
line_func=0x7fffedf1dcf0 <scale_line>, pixel_func=0x7fffedf1eac0 <scale_pixel>) at pixops.c:1351
#5 0x00007fffedf1fe17 in _pixops_scale_real (interp_type=PIXOPS_INTERP_BILINEAR, scale_y=0,0029325513196480938, scale_x=8,5829501602389108e-05,
src_has_alpha=0, src_channels=3, src_rowstride=6291544, src_height=341, src_width=2097181, src_buf=0x7fff38f00000 "", dest_has_alpha=0,
dest_channels=3, dest_rowstride=540, render_y1=1, render_x1=0, render_y0=0, render_x0=0, dest_buf=0x7fffc58a3800 'Z' <repeats 200 times>...)
at pixops.c:2299
#6 _pixops_scale (dest_buf=dest_buf@entry=0x7fffc58a3800 'Z' <repeats 200 times>..., dest_width=<optimized out>, dest_height=<optimized out>,
dest_rowstride=540, dest_channels=3, dest_has_alpha=0, src_buf=0x7fff38f00000 "", src_width=2097181, src_height=341, src_rowstride=6291544,
src_channels=3, src_has_alpha=0, dest_x=0, dest_y=0, dest_region_width=180, dest_region_height=1, offset_x=offset_x@entry=0,
offset_y=offset_y@entry=0, scale_x=scale_x@entry=8,5829501602389108e-05, scale_y=scale_y@entry=0,0029325513196480938,
interp_type=PIXOPS_INTERP_BILINEAR) at pixops.c:2354
#7 0x00007fffedf17bb9 in gdk_pixbuf_scale (src=0x7fffd889ce40, dest=0x7fffd889b9e0, dest_x=0, dest_y=0, dest_width=180, dest_height=1, offset_x=0,
offset_y=0, scale_x=8,5829501602389108e-05, scale_y=0,0029325513196480938, interp_type=GDK_INTERP_BILINEAR) at gdk-pixbuf-scale.c:156
#8 0x00007fffedf18176 in gdk_pixbuf_scale_simple (src=src@entry=0x7fffd889ce40, dest_width=180, dest_height=dest_height@entry=1,
interp_type=interp_type@entry=GDK_INTERP_BILINEAR) at gdk-pixbuf-scale.c:344
#9 0x00007fffedf194cd in get_scaled_pixbuf (scaled=0x7fffcacf0ec0, pixbuf=0x7fffd889ce40) at gdk-pixbuf-scaled-anim.c:138
#10 0x00007fffedf14d94 in gdk_pixbuf_new_from_file_at_scale (filename=0x7fffc258e640 "/home/g/ready/a/boom.bmp", width=<optimized out>,
height=<optimized out>, preserve_aspect_ratio=<optimized out>, error=0x0) at gdk-pixbuf-io.c:1401
#11 0x00007ffff2a43113 in ?? () from /home/g/Apps/firefox/libxul.so
#12 0x00007fffef4db3b8 in g_closure_invoke () from /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#13 0x00007fffef4ecd3d in ?? () from /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#14 0x00007fffef4f4a29 in g_signal_emit_valist () from /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#15 0x00007fffef4f5212 in g_signal_emit_by_name () from /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#16 0x00007fffef4db3b8 in g_closure_invoke () from /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#17 0x00007fffef4ecd3d in ?? () from /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#18 0x00007fffef4f4a29 in g_signal_emit_valist () from /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#19 0x00007fffef4f5212 in g_signal_emit_by_name () from /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#20 0x00007fffef4db3b8 in g_closure_invoke () from /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#21 0x00007fffef4ecd3d in ?? () from /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#22 0x00007fffef4f4a29 in g_signal_emit_valist () from /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#23 0x00007fffef4f5212 in g_signal_emit_by_name () from /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#24 0x00007fffeec5d0e7 in ?? () from /usr/lib/x86_64-linux-gnu/libgtk-x11-2.0.so.0
---Type <return> to continue, or q <return> to quit---
#25 0x00007fffeec603f0 in ?? () from /usr/lib/x86_64-linux-gnu/libgtk-x11-2.0.so.0
#26 0x00007fffef4db5e7 in ?? () from /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#27 0x00007fffef4f4088 in g_signal_emit_valist () from /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#28 0x00007fffef4f4ce2 in g_signal_emit () from /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#29 0x00007fffeeda82be in ?? () from /usr/lib/x86_64-linux-gnu/libgtk-x11-2.0.so.0
#30 0x00007fffeedabb86 in gtk_tree_view_set_cursor_on_cell () from /usr/lib/x86_64-linux-gnu/libgtk-x11-2.0.so.0
#31 0x00007fffeec61fb6 in ?? () from /usr/lib/x86_64-linux-gnu/libgtk-x11-2.0.so.0
#32 0x00007fffef4de372 in g_cclosure_marshal_VOID__POINTERv () from /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#33 0x00007fffef4db5e7 in ?? () from /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#34 0x00007fffef4f4088 in g_signal_emit_valist () from /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#35 0x00007fffef4f4ce2 in g_signal_emit () from /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#36 0x00007fffeec7361a in ?? () from /usr/lib/x86_64-linux-gnu/libgtk-x11-2.0.so.0
#37 0x00007fffee63f6e7 in ?? () from /usr/lib/x86_64-linux-gnu/libgio-2.0.so.0
#38 0x00007fffee6728db in ?? () from /usr/lib/x86_64-linux-gnu/libgio-2.0.so.0
#39 0x00007fffee6728f9 in ?? () from /usr/lib/x86_64-linux-gnu/libgio-2.0.so.0
#40 0x00007fffef20bce5 in g_main_context_dispatch () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#41 0x00007fffef20c048 in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#42 0x00007fffef20c0ec in g_main_context_iteration () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#43 0x00007ffff175238f in ?? () from /home/g/Apps/firefox/libxul.so
#44 0x00007ffff175196d in ?? () from /home/g/Apps/firefox/libxul.so
#45 0x00007ffff169f390 in ?? () from /home/g/Apps/firefox/libxul.so
#46 0x00007ffff16a71a9 in ?? () from /home/g/Apps/firefox/libxul.so
#47 0x00007ffff16be8f3 in ?? () from /home/g/Apps/firefox/libxul.so
#48 0x00007ffff1bc9fd2 in ?? () from /home/g/Apps/firefox/libxul.so
#49 0x00007ffff2a25655 in ?? () from /home/g/Apps/firefox/libxul.so
#50 0x00007ffff2f248f3 in ?? () from /home/g/Apps/firefox/libxul.so
#51 0x00007ffff2f58645 in ?? () from /home/g/Apps/firefox/libxul.so
#52 0x00007ffff2f5accb in ?? () from /home/g/Apps/firefox/libxul.so
#53 0x00007ffff2f5af66 in XRE_main () from /home/g/Apps/firefox/libxul.so
#54 0x00000000004085eb in _start ()
Reporter | ||
Comment 16•9 years ago
|
||
Assignee | ||
Comment 17•9 years ago
|
||
That crash is not happening in the same place for the same reason. It's just signaling an OOM now instead of an overflow.
Reporter | ||
Comment 18•9 years ago
|
||
(In reply to Lee Salzman [:eihrul] from comment #17)
> That crash is not happening in the same place for the same reason. It's just
> signaling an OOM now instead of an overflow.
Yes, but i thought the fix will avoid such abort too.
Assignee | ||
Updated•9 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 19•9 years ago
|
||
After digging further into Gustavo's info, I found that my calculations of the scaled size were off, due to severely non-obvious control flow in gdk-pixbuf's scale calculation. The aspect ratio calculations were actually being triggered in the at_scale_size_prepared_cb in gdk-pixbuf-io.c, which could let an image be constructed that could still trigger the SIGSEGV based on my old patch.
I've adjusted the calculations to take this into account, so that we now explicitly pass in the desired scaled size including aspect ratio and no longer let gdk have control of that. This way we can with certainty predict if an integer overflow would occur.
Anyway, this patch overlays my previous patch to get rid of the corner case that was left.
However, my other finding is that despite this, any OOMs that happen will and indeed must cause an abort like Gustavo noticed in his recent post. The gdk-pixbuf code always assumes that either the malloc returns non-null, or has aborted, so there are no null checks in the code anywhere. Thus, our only option is to leave the abort behavior in place. This is still much safer than leaving the SIGSEGV from wild memory accesses.
An alternative mitigation strategy to also catch the abort would be to further have a maximum preview image size as well, which still would not guarantee aborts wouldn't occur in memory constrained scenarios, but at least would allow reasonable cases to pass through and filter out possibly malicious cases.
Flags: needinfo?(abillings)
Attachment #8644743 -
Flags: review?(acomminos)
Comment 20•9 years ago
|
||
(In reply to Lee Salzman [:eihrul] from comment #19)
>
> However, my other finding is that despite this, any OOMs that happen will
> and indeed must cause an abort like Gustavo noticed in his recent post. The
> gdk-pixbuf code always assumes that either the malloc returns non-null, or
> has aborted, so there are no null checks in the code anywhere. Thus, our
> only option is to leave the abort behavior in place. This is still much
> safer than leaving the SIGSEGV from wild memory accesses.
Does this change the security implications of the original problem at all?
Do you think we should wait to disclose this (write an advisory) until your corner cases are fixed or should I go ahead and disclose this next week when we release 40?
Flags: needinfo?(abillings) → needinfo?(lsalzman)
Assignee | ||
Comment 21•9 years ago
|
||
(In reply to Al Billings [:abillings] from comment #20)
> (In reply to Lee Salzman [:eihrul] from comment #19)
> >
> > However, my other finding is that despite this, any OOMs that happen will
> > and indeed must cause an abort like Gustavo noticed in his recent post. The
> > gdk-pixbuf code always assumes that either the malloc returns non-null, or
> > has aborted, so there are no null checks in the code anywhere. Thus, our
> > only option is to leave the abort behavior in place. This is still much
> > safer than leaving the SIGSEGV from wild memory accesses.
>
> Does this change the security implications of the original problem at all?
>
> Do you think we should wait to disclose this (write an advisory) until your
> corner cases are fixed or should I go ahead and disclose this next week when
> we release 40?
You can still construct images that force gdk-pixbuf to allocate 2GB or more
of memory without causing the original overflow, and such allocations are almost
guaranteed to OOM on the systems most users have, thus causing Firefox to abort.
This is probably not desirable, since while it's not a threat of leaking user
information anymore, it can still deny service.
If we were to limit maximum image preview sizes to, say, no bigger than 4096
on either dimension, then the maximum possible allocation would end up being
only 4096*4096*4, or about 64MB, which is unlikely to cause an OOM, and it would
also have the side-effect of preventing the original overflow from occurring
since the scale ratios would never exceed what causes the scaling code go bug out.
We'd just unfortunately be stuck with that limit until we replaced gdk-pixbuf
in the future.
That might be the best/easiest overall mitigation and could be done easily before
next week if you're okay with that proposal... Then it would be safe enough to
disclose since we'd have a fairly decent kluge-around in place.
Flags: needinfo?(lsalzman)
Comment 22•9 years ago
|
||
We've already built release candidates (for the third time!) so we're not going to take more fixes. I think we should fix this in 41 with your suggestions and not disclose this yet next week.
Dan, do you concur?
Flags: needinfo?(dveditz)
Updated•9 years ago
|
Attachment #8644743 -
Flags: review?(acomminos) → review+
Assignee | ||
Comment 23•9 years ago
|
||
Okay, here's the simpler/stupider/more fool-proof alternate approach that just limits the source size to less than 4096 on any dimension. The maximum buffer it will thus allocate in this case is going to be around 4*4096^2, so 64MB, and thus not terribly likely to OOM.
As a side-effect, this will also prevent the scale ratio filtering overflow without having to explicitly check for it.
Kill two vulnerabilities with one patch.
Attachment #8644743 -
Attachment is obsolete: true
Attachment #8645021 -
Flags: review?(acomminos)
Updated•9 years ago
|
Attachment #8645021 -
Flags: review?(acomminos) → review+
Assignee | ||
Updated•9 years ago
|
Attachment #8645021 -
Flags: checkin?
Comment 24•9 years ago
|
||
Comment on attachment 8645021 [details] [diff] [review]
limit gtk file picker preview source size
https://hg.mozilla.org/integration/mozilla-inbound/rev/10e77092a656
So I guess this is going to need Aurora/Beta/esr38 approval requests as well? Note that this push is highly unlikely to be merged to m-c until after today's uplifts. Whatever that means for the esr38 et al tracking status.
Attachment #8645021 -
Flags: checkin? → checkin+
Comment 25•9 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
status-b2g-v2.0:
--- → unaffected
status-b2g-v2.0M:
--- → unaffected
status-b2g-v2.1:
--- → unaffected
status-firefox43:
--- → fixed
Resolution: --- → FIXED
Target Milestone: mozilla42 → mozilla43
Comment 26•9 years ago
|
||
We should have cloned this bug for the new patch once we knew it was a different crash that wasn't the same security bug, and especially when it had to land in a different release. Status here is messy! Since the follow-up won't need a new security advisory I guess we're OK.
Flags: needinfo?(dveditz)
Comment 27•9 years ago
|
||
Comment 28•9 years ago
|
||
Comment 29•9 years ago
|
||
I'm trying to decide whether to take this bug, and also bug 1031145 (which I am not authorized to view) in Thunderbird 38.2.0 Normally we would to match our code as closely as possible to the Firefox esr38.2.0 release, which has already shipped.
Is there any expectation of doing a FF 38.2.1 due to this or the other bug? Thoughts on whether this might be important for Thunderbird?
Comment 30•9 years ago
|
||
Removing qe-verify+ flag since we were unable to setup the test environment for this (see comment 13).
Flags: qe-verify+
Comment 31•9 years ago
|
||
Hello Gustavo, would you mind taking a look at this using a recent build of Fx41 to see if we've fixed this issue for your scenario? Thank you.
Flags: needinfo?(gustavo.grieco)
Reporter | ||
Comment 32•9 years ago
|
||
Sure. I will test it as soon as possible, later today. Do you have a link to a Firefox 41 Linux build?
Flags: needinfo?(gustavo.grieco)
Comment 33•9 years ago
|
||
Reporter | ||
Comment 34•9 years ago
|
||
I'm affraid that build is too unstable. It keep crashing as soon as i try to browse.
Here you have the stack trace:
Program received signal SIG38, Real-time event 38.
0x00007ffff09a80d0 in ?? () from /usr/lib/x86_64-linux-gnu/libfreetype.so.6
(gdb) bt
#0 0x00007ffff09a80d0 in ?? () from /usr/lib/x86_64-linux-gnu/libfreetype.so.6
#1 0x00007ffff096484c in FT_Load_Glyph () from /usr/lib/x86_64-linux-gnu/libfreetype.so.6
#2 0x00007ffff307ad83 in vpx_get_mb_ss_mmx () from /home/g/Apps/firefox/libxul.so
#3 0x00007ffff16b926f in ?? () from /home/g/Apps/firefox/libxul.so
#4 0x00007ffff3093f4b in vpx_get_mb_ss_mmx () from /home/g/Apps/firefox/libxul.so
#5 0x00007ffff30b429e in vpx_get_mb_ss_mmx () from /home/g/Apps/firefox/libxul.so
#6 0x00007ffff30a1448 in vpx_get_mb_ss_mmx () from /home/g/Apps/firefox/libxul.so
#7 0x00007ffff30b631f in vpx_get_mb_ss_mmx () from /home/g/Apps/firefox/libxul.so
#8 0x00007ffff158e87f in ?? () from /home/g/Apps/firefox/libxul.so
#9 0x00007ffff1e72731 in ?? () from /home/g/Apps/firefox/libxul.so
#10 0x00007ffff15a7544 in ?? () from /home/g/Apps/firefox/libxul.so
...
It looks completely unrelated. I guess we should wait for a better build..
Updated•9 years ago
|
Group: core-security → core-security-release
Comment 35•9 years ago
|
||
(In reply to Gustavo Grieco from comment #34)
> I'm affraid that build is too unstable. It keep crashing as soon as i try to
> browse.
Gustavo, I'm not sure why the build was unstable for you, but could you also try the following builds?
- 41 beta 7 - http://ftp.mozilla.org/pub/mozilla.org/firefox/candidates/41.0b7-candidates/build1/
- latest ESR - http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-esr38-linux64/latest/
If you can't get the time or still have issues, then we'll also give it a try.
Flags: needinfo?(gustavo.grieco)
Reporter | ||
Comment 36•9 years ago
|
||
I can tested early next week. If i'm getting the same crashes as my previous comment, i will open a new bug.
Flags: needinfo?(gustavo.grieco)
Comment 37•9 years ago
|
||
(In reply to Gustavo Grieco from comment #36)
> I can tested early next week. If i'm getting the same crashes as my previous
> comment, i will open a new bug.
Sounds perfect, thanks!
Assignee | ||
Comment 38•9 years ago
|
||
(In reply to Gustavo Grieco from comment #34)
> I'm affraid that build is too unstable. It keep crashing as soon as i try to
> browse.
> Here you have the stack trace:
>
> Program received signal SIG38, Real-time event 38.
> 0x00007ffff09a80d0 in ?? () from /usr/lib/x86_64-linux-gnu/libfreetype.so.6
> (gdb) bt
> #0 0x00007ffff09a80d0 in ?? () from
> /usr/lib/x86_64-linux-gnu/libfreetype.so.6
> #1 0x00007ffff096484c in FT_Load_Glyph () from
> /usr/lib/x86_64-linux-gnu/libfreetype.so.6
> #2 0x00007ffff307ad83 in vpx_get_mb_ss_mmx () from
> /home/g/Apps/firefox/libxul.so
> #3 0x00007ffff16b926f in ?? () from /home/g/Apps/firefox/libxul.so
> #4 0x00007ffff3093f4b in vpx_get_mb_ss_mmx () from
> /home/g/Apps/firefox/libxul.so
> #5 0x00007ffff30b429e in vpx_get_mb_ss_mmx () from
> /home/g/Apps/firefox/libxul.so
> #6 0x00007ffff30a1448 in vpx_get_mb_ss_mmx () from
> /home/g/Apps/firefox/libxul.so
> #7 0x00007ffff30b631f in vpx_get_mb_ss_mmx () from
> /home/g/Apps/firefox/libxul.so
> #8 0x00007ffff158e87f in ?? () from /home/g/Apps/firefox/libxul.so
> #9 0x00007ffff1e72731 in ?? () from /home/g/Apps/firefox/libxul.so
> #10 0x00007ffff15a7544 in ?? () from /home/g/Apps/firefox/libxul.so
> ...
>
> It looks completely unrelated. I guess we should wait for a better build..
SIG38 does not mean a crash; it's a signal uses inside Firefox for internal communication. Just have GDB ignore it or continue past it.
Reporter | ||
Comment 39•9 years ago
|
||
It works great now. I tested with all the broken images, and i got no crash. Even with new heap overflow i reported today (https://bugzilla.mozilla.org/show_bug.cgi?id=1203078). The only crash i could produce was using the DoS with the TARGA file, because it is affecting the parsing of the image itself in gdk (so, there is no way to prevent it unless you disable the TARGA loader).
Reporter | ||
Comment 41•9 years ago
|
||
I think this fix should be pushed as fast as possible. More vulnerabilities are waiting inside gdk-pixbuf, even some are public like this one: https://bugzilla.gnome.org/show_bug.cgi?id=734556
Updated•9 years ago
|
Updated•9 years ago
|
Whiteboard: [adv-main40+][adv-esr38.2+] → [adv-main40+][adv-esr38.2+][adv-main41-]
Updated•9 years ago
|
Whiteboard: [adv-main40+][adv-esr38.2+][adv-main41-] → [adv-main40+][adv-esr38.2+][adv-main41-][adv-esr38.3-]
Updated•8 years ago
|
Group: core-security-release
Updated•8 years ago
|
Updated•8 years ago
|
Keywords: csectype-bounds
Updated•8 years ago
|
Attachment #8643255 -
Attachment description: gustavo.grieco@imag.fr,4000?,2015-07-15,2015-07-30,2015-08-04,true,,, → gustavo.grieco@imag.fr,4000,2015-07-15,2015-07-30,2015-08-04,true,,,
Updated•5 years ago
|
Attachment #8643255 -
Attachment description: gustavo.grieco@imag.fr,4000,2015-07-15,2015-07-30,2015-08-04,true,,, → gustavo.grieco@imag.fr,4000,2015-07-15,2015-07-30,2015-08-04,true,Gustavo Grieco,,
You need to log in
before you can comment on or make changes to this bug.
Description
•