Launchpad has imported 13 comments from the remote bug at https://bugs.freedesktop.org/show_bug.cgi?id=98165.
If you reply to an imported comment from within Launchpad, your comment will be sent to the remote bug automatically. Read more about Launchpad's inter-bugtracker facilities at https://help.launchpad.net/InterBugTracking. ------------------------------------------------------------------------ On 2016-10-08T17:16:15+00:00 Jbowler wrote: This is in cairo-1.14.6 This has already been reported on oss-security, although there is no analysis there and as yet there is no CVE: http://www.openwall.com/lists/oss-security/2016/10/06/1 The repro uses: rsvg-convert -o crash.png crash.svg The crash happens because write_png passes invalid (off by 4GByte) pointers to libpng. The bug is in the declaration of _cairo_image_surface which obviously won't work on a machine with a 64-bit address space and 32-bit (int) values. The crash is 'just' a read from the invalid pointer inside libpng, however there is at least one other case of the loop in read_png where the crash would be a memory overwrite with data from the PNG; that version has been semi-fixed. I'm not posting a detailed analysis because I'm not sure how many places the bug is exposed and it is pretty clear given the fact that the loop in read_png is different that you already know about one instance of this bug. The libpng maintainer has a copy of my complete analysis and the original SVG, I suggest not posting it at the moment because it took me about 4 minutes to find the problem given the SVG. I also suspect it isn't specific to SVG; I assume the read_png change came from test jockeys hitting Cairo with various obvious PNG files, they tend to not test SVG anywhere near as much. The fix is to change 'stride' in the surface to (size_t), and preferably width/height to (uint32_t) and depth to (unsigned). Doing that will reveal all cases of the bug given a sufficiently high warning level. Reply at: https://bugs.launchpad.net/ubuntu/+source/cairo/+bug/1639372/comments/0 ------------------------------------------------------------------------ On 2016-10-11T07:30:52+00:00 Jbowler wrote: This bug is also reported here: https://bugzilla.redhat.com/show_bug.cgi?id=1382656 The referenced bug: http://seclists.org/oss-sec/2016/q4/44 isn't up to date but is, unfortunately, publicly readable. Reply at: https://bugs.launchpad.net/ubuntu/+source/cairo/+bug/1639372/comments/1 ------------------------------------------------------------------------ On 2016-10-11T12:38:11+00:00 Adrian Johnson wrote: Created attachment 127211 fix integer overflow Reply at: https://bugs.launchpad.net/ubuntu/+source/cairo/+bug/1639372/comments/2 ------------------------------------------------------------------------ On 2016-10-11T16:43:50+00:00 Jbowler wrote: Well, yes, stride should be (size_t), but there may be other instances of this. If you change the type of stride in the struct to (unsigned int), from (int) and run with the correct compiler warning options it will warn about: (int) * (unsigned int) because the (int) gets converted silently to (unsigned int). GCC probably ignores this by default, but the -Wconversion stuff is meant to detect it. Coverity certainly can. Doing the above temporarily will tell you if any other code in libcairo does this. It doesn't catch all the potential problems; for example read_png already has 'i' as (unsigned int) and does (IRC): i * stride That still overflows on a 64-bit system, it just requires a bigger SVG and it is a 'safe' overflow because all the pointers are still inside the image buffer. This is why I suggested changing the struct member; it is difficult to detect potential 32-bit overflow. I don't think even Coverity warns about 32-bit arithmetic being used inside a 64-bit address calculation and it is extremely common and normally safe. The other approach you could use is to check when the cairo surface is created to make sure it doesn't require more than a 31, or 32-bit sized buffer. However there are some devices out there which can exceed a 4GByte image; think of a 72" poster printer running at 1200dpi. That has 86400 dots (bytes) per row so a 42" high printout would exceed the limit. Reply at: https://bugs.launchpad.net/ubuntu/+source/cairo/+bug/1639372/comments/3 ------------------------------------------------------------------------ On 2016-10-13T11:36:08+00:00 Adrian Johnson wrote: I don't like the idea of making stride unsigned. Maybe ptrdiff_t would be a better type for stride. Reply at: https://bugs.launchpad.net/ubuntu/+source/cairo/+bug/1639372/comments/4 ------------------------------------------------------------------------ On 2016-10-13T14:54:46+00:00 Jbowler wrote: If cairo does support bottom-up surfaces, as are typically used in engineering analysis (where 'z' comes out of the page) then that is the correct solution. Indeed, the change made to write_png (the cast to (size_t)) does not work because the surface is not made inside cairo- png.c (as in read_png). Internally libpng uses ptrdiff_t because the libpng "simplified API" accepts an image buffer with a negative stride; stride is 31-bit signed in the API but the local variables initialized using it are ptrdiff_t. With hindsight it would have been better to use ptrdiff_t in the API, but the CVEs only started rolling in after the API had been in use for a while. Reply at: https://bugs.launchpad.net/ubuntu/+source/cairo/+bug/1639372/comments/5 ------------------------------------------------------------------------ On 2016-10-20T11:06:45+00:00 Adrian Johnson wrote: Created attachment 127421 prevent invalid ptr access Reply at: https://bugs.launchpad.net/ubuntu/+source/cairo/+bug/1639372/comments/6 ------------------------------------------------------------------------ On 2016-11-10T00:35:26+00:00 Seth Arnold wrote: Is this patch sufficient? There's more cases of 'int stride', including e.g.: static cairo_surface_t * _cairo_boilerplate_image_create_similar (cairo_surface_t *other, cairo_content_t content, int width, int height) { cairo_format_t format; cairo_surface_t *surface; int stride; void *ptr; switch (content) { case CAIRO_CONTENT_ALPHA: format = CAIRO_FORMAT_A8; break; case CAIRO_CONTENT_COLOR: format = CAIRO_FORMAT_RGB24; break; case CAIRO_CONTENT_COLOR_ALPHA: default: format = CAIRO_FORMAT_ARGB32; break; } stride = cairo_format_stride_for_width(format, width); ptr = malloc (stride* height); surface = cairo_image_surface_create_for_data (ptr, format, width, height, stride); cairo_surface_set_user_data (surface, &key, ptr, free); return surface; } I know the malloc (stride * height) looks unsafe, but I think cairo_format_stride_for_width() will return -1 in cases that might cause the stride*height parameter to overflow, which causes the malloc() to fail, and the subsequent functions fail 'safely' in that case. In any event, this code looks fairly closely tied to stride being an 'int' rather than ptrdiff_t. Here's another example: static cairo_status_t _cairo_image_spans_and_zero (void *abstract_renderer, int y, int height, const cairo_half_open_span_t *spans, unsigned num_spans) { cairo_image_span_renderer_t *r = abstract_renderer; uint8_t *mask; int len; mask = r->u.mask.data; if (y > r->u.mask.extents.y) { len = (y - r->u.mask.extents.y) * r->u.mask.stride; memset (mask, 0, len); mask += len; } The u.mask.stride would be a ptrdiff_t after this patch, but 'len' is still only an 'int'. Any advice appreciated. Thanks Reply at: https://bugs.launchpad.net/ubuntu/+source/cairo/+bug/1639372/comments/11 ------------------------------------------------------------------------ On 2016-11-10T01:33:33+00:00 Jbowler wrote: (In reply to Seth Arnold from comment #7) > stride = cairo_format_stride_for_width(format, width); I think that function should return a ptrdiff_t. If it does so the compiler will start issuing warnings on 64-bit builds where the result is truncated to 32 bits, as in the above assignment. I believe that in general it is ok for 'width' and 'height' to be (int), assuming you don't support 16-bit systems, but any multiplication has to be done with care: > ptr = malloc (stride* height); That strikes me as bogus. Images with rows as long as 2^31 bytes are rare; pretty much limited to people trying to break libpng! However images with 2^31 or more bytes in total can happen and it is perfectly possible on a modern desktop/laptop/tablet to end up with one in memory. (Maybe it's a little dumb, but it is possible.) In any case if cairo_format_stride_for_width uses (-1) as an error return (I'm not that was what you are saying) the callers need to check for it. > surface = cairo_image_surface_create_for_data (ptr, format, > width, height, stride); The last (stride) parameter should have type ptrdiff_t. I think the point I was trying to make before is that if the change is done in the struct and places that use the 'stride' member it will force a lot of other changes (int to ptrdiff_t) but a GNU 64-bit compiler with -Wall -Wextra should show the vast majority of the issues. > Here's another example: > > static cairo_status_t > _cairo_image_spans_and_zero (void *abstract_renderer, > int y, int height, > const cairo_half_open_span_t *spans, > unsigned num_spans) > { > cairo_image_span_renderer_t *r = abstract_renderer; > uint8_t *mask; > int len; > > mask = r->u.mask.data; > if (y > r->u.mask.extents.y) { > len = (y - r->u.mask.extents.y) * r->u.mask.stride; > memset (mask, 0, len); > mask += len; > } > > > The u.mask.stride would be a ptrdiff_t after this patch, but 'len' is still > only an 'int'. Right; that should result in a compiler warning for the assignment to 'len', so then the next step is to change 'len' to ptrdiff_t. Fixing the warnings forces the change through to all the required places. The only way to stop the warning without fixing the problem is to use an explicit cast; a static_cast in C++ terms. casts are evil ;-) Reply at: https://bugs.launchpad.net/ubuntu/+source/cairo/+bug/1639372/comments/12 ------------------------------------------------------------------------ On 2017-01-05T14:49:28+00:00 zhouzhen1 wrote: It's been a while since this thread was last updated. Is there any plan to fix this and make a new release? Reply at: https://bugs.launchpad.net/ubuntu/+source/cairo/+bug/1639372/comments/14 ------------------------------------------------------------------------ On 2017-11-08T01:09:59+00:00 Bryce Harrington wrote: Yes agreed, this fix looks ok, and this is already being carried by Debian Sid. Carrying this in the devel tree seems like the next logical step, and if no issues arise from the extra testing and review, it looks suitable for landing in 1.14 stable too. Landed: To ssh://git.freedesktop.org/git/cairo 35fccff..38fbe62 master -> master Given the feedback in comments 7 & 8 I'm going to leave this report open for now as reminder to investigate further, although it might be worthwhile to break those out as a separate bug report or two so this one can be closed. Reply at: https://bugs.launchpad.net/ubuntu/+source/cairo/+bug/1639372/comments/17 ------------------------------------------------------------------------ On 2018-08-25T13:35:06+00:00 Gitlab-migration wrote: -- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/cairo/cairo/issues/81. Reply at: https://bugs.launchpad.net/ubuntu/+source/cairo/+bug/1639372/comments/18 ------------------------------------------------------------------------ On 2019-07-20T00:47:28+00:00 Emadyassen1998 wrote: great post. http://www.winmilliongame.com http://www.gtagame100.com http://www.subway-game.com http://www.zumagame100.com Reply at: https://bugs.launchpad.net/ubuntu/+source/cairo/+bug/1639372/comments/19 ** Changed in: cairo Status: In Progress => Unknown ** Bug watch added: Red Hat Bugzilla #1382656 https://bugzilla.redhat.com/show_bug.cgi?id=1382656 ** Bug watch added: gitlab.freedesktop.org/cairo/cairo/issues #81 https://gitlab.freedesktop.org/cairo/cairo/issues/81 -- You received this bug notification because you are a member of Ubuntu Touch seeded packages, which is subscribed to cairo in Ubuntu. https://bugs.launchpad.net/bugs/1639372 Title: CVE-2016-9082: DOS attack in converting SVG to PNG Status in cairo: Unknown Status in cairo package in Ubuntu: Fix Released Status in cairo source package in Precise: Confirmed Status in cairo source package in Trusty: Confirmed Status in cairo source package in Xenial: Confirmed Status in cairo source package in Yakkety: Confirmed Status in cairo package in Debian: Fix Released Bug description: I'm attaching debdiffs for trusty, xenial and yakkety. Zesty is already fixed by syncing cairo 1.14.6-1.1 from Debian. Maybe someone else can work on the precise update. Proof of Concept at http://seclists.org/oss-sec/2016/q4/44 I didn't get gdb to work, but when I tried to convert the file, I got a crash report named /var/crash/_usr_bin_rsvg-convert.1000.crash . After the update, no crash happened. I reproduced the crash and verified that the new package doesn't crash on yakkety. In xenial I wasn't able to reproduce the crash. I did not test on trusty. To manage notifications about this bug go to: https://bugs.launchpad.net/cairo/+bug/1639372/+subscriptions -- Mailing list: https://launchpad.net/~touch-packages Post to : touch-packages@lists.launchpad.net Unsubscribe : https://launchpad.net/~touch-packages More help : https://help.launchpad.net/ListHelp
