Re: [webkit-dev] RenderArena: Teaching an old dog new tricks
On Wed, Nov 14, 2012 at 11:37 PM, Chris Evans cev...@chromium.org wrote: On Wed, Nov 14, 2012 at 10:29 PM, Ryosuke Niwa rn...@webkit.org wrote: In other words, why are you interested in using the proposed allocation mechanism for only DOM nodes/objects instead of everything in WebCore/WebKit? The challenge is to add partitioning without going too crazy on number of partitions, whilst maximizing benefit (i.e minimizing the options the attacker has.) If we stuff every virtual class in WebCore/WebKit together, the attacker has just tons of options for overlap of freed object and attacker-chosen object. The situation is complicated by the presence of multiple vtables in some classes. Right. That kind of trade off makes sense. But there’s also a danger of making our codebase too complex. As you’re well aware, increasing code complexity will lead to more bugs including but not limited to security bugs. I’ve been seen many new and even some experienced contributors introducing memory management bugs because they’re not used to either RefCounted objects or RenderArena allocated objects because many people tend to work on either rendering code or DOM code but not both. If we’re deploying RenderArea for DOM nodes, I’d like to make sure we have a solid API that’s extremely hard to misuse (in addition to making sure adopting a node from one document to another doesn’t result in pathological performance, etc…). To come up with the suggestion of partitioning off the Node hierarchy, I actually did a blind analysis against the Pinkie Pie exploit. i.e. I picked the best defense idea we had without fully dissecting the exploit and then ran an analysis of how the defense would have impacted the exploitability. With the defense in place, the attacker's options for this particular case turned out to be severely limited and I couldn't find a good place to start. I even managed to locate and chat to Pinkie Pie who described the proposed defense as quite useful. Are there other exploits we can get data from? Getting statistics from one exploit results in a strong sampling bias so ideally we can assess whether slab memory allocation is a good defense against other types of exploits. - R. Niwa ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] RenderArena: Teaching an old dog new tricks
On Wed, Nov 14, 2012 at 11:32 PM, Maciej Stachowiak m...@apple.com wrote: On Nov 14, 2012, at 11:09 PM, Chris Evans cev...@chromium.org wrote: On Wed, Nov 14, 2012 at 8:59 PM, Ryosuke Niwa rn...@webkit.org wrote: On Wed, Nov 14, 2012 at 8:52 PM, Elliott Sprehn espr...@chromium.orgwrote: I was present for one of the discussions about the exploit and how an arena like allocator could have helped at Google. One proposed solution was to allocate all the JS typed buffers in an arena. Is there a reason we can't just do that? It's much less intrusive to allocate ArrayBuffer in an arena than to allocate all DOM objects in one. I don’t think allocating all JS objects in an arena is good enough because attackers can inject nearly arbitrary sequence of bytes into DOM objects (e.g. text node). Yeah, pretty much this. The worry is that it's very hard to be sure you've identified all cases / classes where the attacker has reasonable control over the size and exact content of the allocation. You have to start looking at the buffers backing ArrayBuffers, the buffers backing WTF::Vectors, the buffers backing the (multitude of) string classes, and even then, you're left worrying about objects that simply have a bunch of consecutive ints that the attacker can set as properties, etc. And even in the unlikely event you catch everything in WebKit, you still have other very attacker-controllable allocations going on in the same process such as audio and video packets; canvas buffers; rendered image buffers; the list goes on. I don't think it's a battle than can be won. So we think the problem is best approached from another observation: - Use-after-free of certain classes is either very lethal, or very common, or both. Use-after-free is pretty common for the RenderObject hierarchy and the Node hierarchy. Inferno ran a quick script for use-after-free stats in automated ClusterFuzz reports and it was approximately 332 Render and 134 Node. Due to historical accident, we already have a protection for RenderObject. The most lethal use-after-frees, though, are DOM objects. A freed DOM object is often wired directly into Javascript and the attacker can prod all sorts of methods and properties on the freed object in order to cause a chosen set of accesses (corresponding to reads, writes and vtable usages under the covers) in a chosen order. I'm not comfortable sharing it verbatim on a public list, but happy to send you a copy of the Pinkie Pie exploit if you're interested. It relies on a lethal DOM use-after-free. Because use-after-free in the Node hierarchy is both common and lethal, a separate allocation area seems a profitable path forward. It still seems to me like the key difference is vtable vs no vtable, It's an important difference, but if we partitioned in to two based on that difference alone, we'd have the following issues: 1) Classes with multiple vtables would spoil our day. The sheer number of classes in the vtable partition would practically ensure that an attacker could overlap data of their choosing on top of a secondary vtable. Overlap of attacker data onto a secondary vtable is also a concern with the DOM partition approach, but the chances of it being possible are much much lower. In the Pinkie Pie exploit case, the DOM arena solution made it impossible. 2) The Pinkie Pie exploit wasn't exclusively about the vtable. Before you can abuse an overlap with a freed vtable pointer, you need to defeat ASLR. This was partly achieved by overlapping the pointer and size of a freed WTF::Vector member with arbitrary data. (This is highly dangerous for DOM objects as it can lead to a direct arbitrary memory read or write primitive from Javascript!) Again, the sheer number of classes in a vtable partition would make a collision profitable to the attacker a statistical likelihood. Again, with a strict DOM arena, possibilities are really shut down. Back to the Pinkie Pie case, there wasn't any immediately useful overlap in either the 32-bit or 64-bit memory layout. rather than DOM vs. not DOM. Also having a per-document arena for DOM nodes (as is done for render objects via RenderArena) seems irrelevant to the security goal and likely to cause bad memory fragmentation. My read on the Arena is that it's fragmentation resistant (i.e. it will not repurpose a larger free chunk to satisfy a smaller allocation.) However, memory usage at any given time is defined by peak usage since it cannot release pages back to the system without ruining its security guarantee. Interestingly, it can't be super bad: we already bite this bullet for RenderArena as used by RenderObjects. The RenderArena lifetime is the same as the document / DOM and I was surprised to recently be told that we don't throw away the RenderArena on a full layout. Cheers Chris Allocating everything with a vtable separately seems like it would achieve the security goals, avoid the need to thread arena
Re: [webkit-dev] RenderArena: Teaching an old dog new tricks
On Nov 15, 2012, at 12:02 AM, Ryosuke Niwa rn...@webkit.org wrote: On Wed, Nov 14, 2012 at 11:37 PM, Chris Evans cev...@chromium.org wrote: On Wed, Nov 14, 2012 at 10:29 PM, Ryosuke Niwa rn...@webkit.org wrote: In other words, why are you interested in using the proposed allocation mechanism for only DOM nodes/objects instead of everything in WebCore/WebKit? The challenge is to add partitioning without going too crazy on number of partitions, whilst maximizing benefit (i.e minimizing the options the attacker has.) If we stuff every virtual class in WebCore/WebKit together, the attacker has just tons of options for overlap of freed object and attacker-chosen object. The situation is complicated by the presence of multiple vtables in some classes. Right. That kind of trade off makes sense. But there’s also a danger of making our codebase too complex. As you’re well aware, increasing code complexity will lead to more bugs including but not limited to security bugs. I’ve been seen many new and even some experienced contributors introducing memory management bugs because they’re not used to either RefCounted objects or RenderArena allocated objects because many people tend to work on either rendering code or DOM code but not both. If we’re deploying RenderArea for DOM nodes, I’d like to make sure we have a solid API that’s extremely hard to misuse (in addition to making sure adopting a node from one document to another doesn’t result in pathological performance, etc…). You can achieve all this by having only one global custom allocator for all objects inheriting from Node, instead of one per Document based on RenderArena. They could stay RefCounted and would just need to overload operator new and operator delete at the Node level. There would be no need to pass around arena pointers and clients of Node and its subclasses would not even have to be aware of the difference. Still not convince that just targeting Node subclasses is a rational solution. To come up with the suggestion of partitioning off the Node hierarchy, I actually did a blind analysis against the Pinkie Pie exploit. i.e. I picked the best defense idea we had without fully dissecting the exploit and then ran an analysis of how the defense would have impacted the exploitability. With the defense in place, the attacker's options for this particular case turned out to be severely limited and I couldn't find a good place to start. I even managed to locate and chat to Pinkie Pie who described the proposed defense as quite useful. Are there other exploits we can get data from? Getting statistics from one exploit results in a strong sampling bias so ideally we can assess whether slab memory allocation is a good defense against other types of exploits. I tend to agree that if we make a memory allocation architecture change, it should be based on sample size 1. Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] RenderArena: Teaching an old dog new tricks
On Thu, Nov 15, 2012 at 3:22 AM, Chris Evans cev...@chromium.org wrote: ... My read on the Arena is that it's fragmentation resistant (i.e. it will not repurpose a larger free chunk to satisfy a smaller allocation.) However, memory usage at any given time is defined by peak usage since it cannot release pages back to the system without ruining its security guarantee. Interestingly, it can't be super bad: we already bite this bullet for RenderArena as used by RenderObjects. The RenderArena lifetime is the same as the document / DOM and I was surprised to recently be told that we don't throw away the RenderArena on a full layout. My understanding is that the render tree is rather small compared to the DOM in terms of memory usage on most pages so not releasing it may not be so bad, but never releasing the memory used from DOM nodes seems like it'd be bad for large web apps. - E ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] RenderArena: Teaching an old dog new tricks
On Thu, Nov 15, 2012 at 12:22 AM, Chris Evans cev...@chromium.org wrote: On Wed, Nov 14, 2012 at 11:32 PM, Maciej Stachowiak m...@apple.com wrote: rather than DOM vs. not DOM. Also having a per-document arena for DOM nodes (as is done for render objects via RenderArena) seems irrelevant to the security goal and likely to cause bad memory fragmentation. My read on the Arena is that it's fragmentation resistant (i.e. it will not repurpose a larger free chunk to satisfy a smaller allocation.) However, memory usage at any given time is defined by peak usage since it cannot release pages back to the system without ruining its security guarantee. Interestingly, it can't be super bad: we already bite this bullet for RenderArena as used by RenderObjects. The RenderArena lifetime is the same as the document / DOM and I was surprised to recently be told that we don't throw away the RenderArena on a full layout. Not really. Render tree is really small. It's in the order of a few megabytes on most websites. On the other hand, DOM tree and CSS objects can consume as much as tens, if not hundreds, of megabytes because there are many DOM nodes that are not displayed on the screen. Also, a large proportion of render objects tend to be allocated and deallocated at the same time while DOM nodes tend to be created and deleted at different times on many script heavy page. These two characteristics of render tree makes it particularly attractive to make use of memory management strategies like the one used in RenderArena. I'm not convinced that using the same strategy for DOM nodes is a good idea. - R. Niwa ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] RenderArena: Teaching an old dog new tricks
On Nov 15, 2012, at 12:22 AM, Chris Evans cev...@chromium.org wrote: On Wed, Nov 14, 2012 at 11:32 PM, Maciej Stachowiak m...@apple.com wrote: On Nov 14, 2012, at 11:09 PM, Chris Evans cev...@chromium.org wrote: On Wed, Nov 14, 2012 at 8:59 PM, Ryosuke Niwa rn...@webkit.org wrote: On Wed, Nov 14, 2012 at 8:52 PM, Elliott Sprehn espr...@chromium.org wrote: I was present for one of the discussions about the exploit and how an arena like allocator could have helped at Google. One proposed solution was to allocate all the JS typed buffers in an arena. Is there a reason we can't just do that? It's much less intrusive to allocate ArrayBuffer in an arena than to allocate all DOM objects in one. I don’t think allocating all JS objects in an arena is good enough because attackers can inject nearly arbitrary sequence of bytes into DOM objects (e.g. text node). Yeah, pretty much this. The worry is that it's very hard to be sure you've identified all cases / classes where the attacker has reasonable control over the size and exact content of the allocation. You have to start looking at the buffers backing ArrayBuffers, the buffers backing WTF::Vectors, the buffers backing the (multitude of) string classes, and even then, you're left worrying about objects that simply have a bunch of consecutive ints that the attacker can set as properties, etc. And even in the unlikely event you catch everything in WebKit, you still have other very attacker-controllable allocations going on in the same process such as audio and video packets; canvas buffers; rendered image buffers; the list goes on. I don't think it's a battle than can be won. So we think the problem is best approached from another observation: - Use-after-free of certain classes is either very lethal, or very common, or both. Use-after-free is pretty common for the RenderObject hierarchy and the Node hierarchy. Inferno ran a quick script for use-after-free stats in automated ClusterFuzz reports and it was approximately 332 Render and 134 Node. Due to historical accident, we already have a protection for RenderObject. The most lethal use-after-frees, though, are DOM objects. A freed DOM object is often wired directly into Javascript and the attacker can prod all sorts of methods and properties on the freed object in order to cause a chosen set of accesses (corresponding to reads, writes and vtable usages under the covers) in a chosen order. I'm not comfortable sharing it verbatim on a public list, but happy to send you a copy of the Pinkie Pie exploit if you're interested. It relies on a lethal DOM use-after-free. Because use-after-free in the Node hierarchy is both common and lethal, a separate allocation area seems a profitable path forward. It still seems to me like the key difference is vtable vs no vtable, It's an important difference, but if we partitioned in to two based on that difference alone, we'd have the following issues: 1) Classes with multiple vtables would spoil our day. The sheer number of classes in the vtable partition would practically ensure that an attacker could overlap data of their choosing on top of a secondary vtable. Overlap of attacker data onto a secondary vtable is also a concern with the DOM partition approach, but the chances of it being possible are much much lower. In the Pinkie Pie exploit case, the DOM arena solution made it impossible. 2) The Pinkie Pie exploit wasn't exclusively about the vtable. Before you can abuse an overlap with a freed vtable pointer, you need to defeat ASLR. This was partly achieved by overlapping the pointer and size of a freed WTF::Vector member with arbitrary data. (This is highly dangerous for DOM objects as it can lead to a direct arbitrary memory read or write primitive from Javascript!) Again, the sheer number of classes in a vtable partition would make a collision profitable to the attacker a statistical likelihood. Again, with a strict DOM arena, possibilities are really shut down. Back to the Pinkie Pie case, there wasn't any immediately useful overlap in either the 32-bit or 64-bit memory layout. I would probably need to know more about it to comment intelligently. Maybe we could discuss offline or on the security list. rather than DOM vs. not DOM. Also having a per-document arena for DOM nodes (as is done for render objects via RenderArena) seems irrelevant to the security goal and likely to cause bad memory fragmentation. My read on the Arena is that it's fragmentation resistant (i.e. it will not repurpose a larger free chunk to satisfy a smaller allocation.) However, memory usage at any given time is defined by peak usage since it cannot release pages back to the system without ruining its security guarantee. I'm talking about external fragmentation, not internal fragmentation. RenderArena is not resistant to it in any way. Interestingly,
[webkit-dev] PAN(flick) to left operation collapse screen in Webkit(Safari) on window7 tablet
Hi, PAN(flick) to left operation collapse screen in Webkit(Safari) on window7 tablet. Version:Safari 5.1.7(7534.57.2) Does anyone know this problem and the patch to resolve? How to reproduce. 1) Launch Safai on Windows tablet. Set its window's width less than 960px. 2) Open http://jquerymobile.com/demos/1.1.1/docs/forms/docs-forms.html with it. 3) On the right pane, do PAN(flick) to left, then the whole contents in the window move horizontally to left by about 30px to 100px. This contents itself is not designed to move horizontally. The move does not occur on other browsers such as Chrome and IE. This problem may not occur depending on window's width size and zoom scale. After my team analyzed this problem, we figured out that, when RenderLayer's m_scrollSize.m_width is greather than m_LayerSize.m_width at the beginning of scrollByRecursively in RederLayer.cpp which is called just before PAN processing, this problem occurs. Hideki *** Hideki Yoshida Embedded Software Division NEC System Technologies, Ltd. E-MAIL:yoshida-...@necst.nec.co.jp *** ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] RenderArena: Teaching an old dog new tricks
On Thu, Nov 15, 2012 at 12:34 AM, Elliott Sprehn espr...@chromium.orgwrote: On Thu, Nov 15, 2012 at 3:22 AM, Chris Evans cev...@chromium.org wrote: ... My read on the Arena is that it's fragmentation resistant (i.e. it will not repurpose a larger free chunk to satisfy a smaller allocation.) However, memory usage at any given time is defined by peak usage since it cannot release pages back to the system without ruining its security guarantee. Interestingly, it can't be super bad: we already bite this bullet for RenderArena as used by RenderObjects. The RenderArena lifetime is the same as the document / DOM and I was surprised to recently be told that we don't throw away the RenderArena on a full layout. My understanding is that the render tree is rather small compared to the DOM in terms of memory usage on most pages This does not seem to necessarily be the case. Loading Gmail to the inbox view, sizes in bytes for all live allocations: - 541k in the DOM arena - At least a 918k plus a 49k render arena. May have failed to count a few smaller ones that are still live. Would you like similar measurements for a simpler page? Or some other complicated page you think might be instructive? Cheers Chris so not releasing it may not be so bad, but never releasing the memory used from DOM nodes seems like it'd be bad for large web apps. - E ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Porting WebKit To a new graphic backend.
On 11/14/2012 07:24 AM, ZhangJiPeng wrote: The idea came from an embedded browser development project. Benjamin I want to porting WebKit to a new platform, the platform can only provide video address programming interface. So I need to porting DirectFB, Cairo, GTK and so on. However the hardware resources are limited, can't running so much software, I need to find a more lightweight and high quality solutions, so I develop the picasso library. The same code base can run on different graphics system does not depend on the features of graphic system, this is the goal of Picasso. To experiment with this, you could create your own git branch and try implementing a GraphicsContextPicasso (see Source/WebCore/platform/graphics/cairo | skia) - similar to GraphicsContextSkia and GraphicsContextCairo. Then you need to typedef the WebKit GraphicsContext to the Picasso graphics context and instruct your browser app to do all WebKit rendering into the Picasso GraphicsContext. Gettings simple web pages to render with a new backend, somewhat close to the other backends might not even take that long. However, getting every detail right, like layers with opacity, rounded corners, correct clipping, full SVG support etc. is a rahter large amount of work. Regards, Dominik ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Porting WebKit To a new graphic backend.
, Dominik Röttsches wrote: On 11/14/2012 07:24 AM, ZhangJiPeng wrote: The idea came from an embedded browser development project. Benjamin I want to porting WebKit to a new platform, the platform can only provide video address programming interface. So I need to porting DirectFB, Cairo, GTK and so on. However the hardware resources are limited, can't running so much software, I need to find a more lightweight and high quality solutions, so I develop the picasso library. The same code base can run on different graphics system does not depend on the features of graphic system, this is the goal of Picasso. To experiment with this, you could create your own git branch and try implementing a GraphicsContextPicasso (see Source/WebCore/platform/graphics/cairo | skia) - similar to GraphicsContextSkia and GraphicsContextCairo. Then you need to typedef the WebKit GraphicsContext to the Picasso graphics context and instruct your browser app to do all WebKit rendering into the Picasso GraphicsContext. Gettings simple web pages to render with a new backend, somewhat close to the other backends might not even take that long. However, getting every detail right, like layers with opacity, rounded corners, correct clipping, full SVG support etc. is a rahter large amount of work. Regards, Dominik Thank you for your suggestion. I have submitted a patch for these changes. https://bugs.webkit.org/show_bug.cgi?id=102063 It will be actively maintained. Regards jpzhang ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] PAN(flick) to left operation collapse screen in Webkit(Safari) on window7 tablet
It sounds like you meant to go to https://bugs.webkit.org/ and file a bug. Thanks, ~Brady On Nov 15, 2012, at 1:13 AM, HIDEKI YOSHIDA yoshida-...@necst.nec.co.jp wrote: Hi, PAN(flick) to left operation collapse screen in Webkit(Safari) on window7 tablet. Version:Safari 5.1.7(7534.57.2) Does anyone know this problem and the patch to resolve? How to reproduce. 1) Launch Safai on Windows tablet. Set its window's width less than 960px. 2) Open http://jquerymobile.com/demos/1.1.1/docs/forms/docs-forms.html with it. 3) On the right pane, do PAN(flick) to left, then the whole contents in the window move horizontally to left by about 30px to 100px. This contents itself is not designed to move horizontally. The move does not occur on other browsers such as Chrome and IE. This problem may not occur depending on window's width size and zoom scale. After my team analyzed this problem, we figured out that, when RenderLayer's m_scrollSize.m_width is greather than m_LayerSize.m_width at the beginning of scrollByRecursively in RederLayer.cpp which is called just before PAN processing, this problem occurs. Hideki *** Hideki Yoshida Embedded Software Division NEC System Technologies, Ltd. E-MAIL:yoshida-...@necst.nec.co.jp *** ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] RenderArena: Teaching an old dog new tricks
Since a common theme people are bringing up is vtable overrides, I do recall reading about vtable masking being available in some compilers. I'm wondering if we should push for support for such in compilers we use - I'm not sure what the vcall perf hit is in such cases, but it would knock kill off vtable overwrites as an attack vector. We can also trivially add vtable poisoning to dealloc if we felt it necessary - i'm not sure here if the problem is specifically use-after-free, use-after-free-and-overwritten-with-evil-data, or use-after-free-and-overwritten-with-another-object-with-a-different-type. --Oliver On Nov 15, 2012, at 2:08 AM, Chris Evans cev...@chromium.org wrote: On Thu, Nov 15, 2012 at 12:34 AM, Elliott Sprehn espr...@chromium.org wrote: On Thu, Nov 15, 2012 at 3:22 AM, Chris Evans cev...@chromium.org wrote: ... My read on the Arena is that it's fragmentation resistant (i.e. it will not repurpose a larger free chunk to satisfy a smaller allocation.) However, memory usage at any given time is defined by peak usage since it cannot release pages back to the system without ruining its security guarantee. Interestingly, it can't be super bad: we already bite this bullet for RenderArena as used by RenderObjects. The RenderArena lifetime is the same as the document / DOM and I was surprised to recently be told that we don't throw away the RenderArena on a full layout. My understanding is that the render tree is rather small compared to the DOM in terms of memory usage on most pages This does not seem to necessarily be the case. Loading Gmail to the inbox view, sizes in bytes for all live allocations: - 541k in the DOM arena - At least a 918k plus a 49k render arena. May have failed to count a few smaller ones that are still live. Would you like similar measurements for a simpler page? Or some other complicated page you think might be instructive? Cheers Chris so not releasing it may not be so bad, but never releasing the memory used from DOM nodes seems like it'd be bad for large web apps. - E ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] RenderArena: Teaching an old dog new tricks
On Nov 14, 2012, at 3:19 PM, Chris Evans cev...@chromium.org wrote: A first step might be to make it a platform define. For the Chromium platform we'd leave the define on -- there are some nice security properties we get from having the RenderObjects in their own spot. I'm happy to go in to more details if you want, but it's similar (although not identical) to the blog post linked by Brendan regarding Firefox. Not all WebKit consumers need weight things the same way as the Chromium port of course, but at least for us, the security win outweighs any quirks of RenderArena. r- Don't do this. I have removed the please to aid clarity. Geoff ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] RenderArena: Teaching an old dog new tricks
On Nov 14, 2012, at 3:27 PM, Ojan Vafai o...@chromium.org wrote: As someone outside all these discussions, this seems like a completely unfair characterization of what happened. Sam voiced an objection, then there was a bunch of discussion in which Chris made an argument that Eric found compelling. Many days passed, then Eric r+'ed. Unless there was other discussion not on the bug that I missed, your objection came after Eric's r+. I don't see the process problem here. I want to call this issue out specifically, because it has come up more than once now. The burden of proof for a patch is on the submitter, not on the reviewer. If a reviewer says a patch is a bad idea, it's up to the submitter to convince the reviewer otherwise. Saying more things and then letting time pass is not the same as convincing the reviewer, and it does not give one automatic rights to commit code, just because the reviewer has not kept up with line-by-line refutation. It is an impossible burden for good reviewers to argue, line-by-line, against all bad ideas. I tried to do this in just one bug (http://webkit.org/b/88834), saying no to O(N^2) algorithms, followed by leaky algorithms, followed by use-after-free algorithms, with specific details, and the full process took three months. It is simply not practical to assume that reviewers can do this for all patches. During those three months, one of the reasons I kept up with line-by-line refutation was that I feared that another reviewer might use the same logic you've used here in order to r+ a bad patch. This has to stop. The process problem here was three-fold: (1) Chris argued with Sam's objection, but did not resolve it (2) Eric r+ed a patch with an unresolved objection from a reviewer (3) Eric announced a new direction for WebKit, after two reviewers had objected to it On Nov 14, 2012, at 3:18 PM, Adam Barth aba...@webkit.org wrote: One thing that I notice about these two comments is that they say please don't make this change but they don't explain why. I realize that it takes more time and energy to explain the why, but it's helpful for folks like me (and I suspect Chris and Cris as well) who don't know these issues as well as you. Thanks for taking the time to explain the issue in your previous email. That helped me understand this issue much better than before. I'm happy to participate, and say why, when I have time. I don't always have time. Please don't take my participation as consent to the crazy notion that line-by-line refutation is required to r- a bad patch. Geoff ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] RenderArena: Teaching an old dog new tricks
On Thu, Nov 15, 2012 at 11:49 AM, Geoffrey Garen gga...@apple.com wrote: On Nov 14, 2012, at 3:19 PM, Chris Evans cev...@chromium.org wrote: A first step might be to make it a platform define. For the Chromium platform we'd leave the define on -- there are some nice security properties we get from having the RenderObjects in their own spot. I'm happy to go in to more details if you want, but it's similar (although not identical) to the blog post linked by Brendan regarding Firefox. Not all WebKit consumers need weight things the same way as the Chromium port of course, but at least for us, the security win outweighs any quirks of RenderArena. r- Don't do this. Ok, no platform define for RenderArena. There's also an implicit r- on removing the thing, though, as we'd regress security(!!) and performance. Seems we're stuck with the thing. Cheers Chris I have removed the please to aid clarity. Geoff ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] RenderArena: Teaching an old dog new tricks
On Nov 15, 2012, at 2:16 PM, Chris Evans cev...@chromium.org wrote: On Thu, Nov 15, 2012 at 11:49 AM, Geoffrey Garen gga...@apple.com wrote: On Nov 14, 2012, at 3:19 PM, Chris Evans cev...@chromium.org wrote: A first step might be to make it a platform define. For the Chromium platform we'd leave the define on -- there are some nice security properties we get from having the RenderObjects in their own spot. I'm happy to go in to more details if you want, but it's similar (although not identical) to the blog post linked by Brendan regarding Firefox. Not all WebKit consumers need weight things the same way as the Chromium port of course, but at least for us, the security win outweighs any quirks of RenderArena. r- Don't do this. Ok, no platform define for RenderArena. There's also an implicit r- on removing the thing, though, as we'd regress security(!!) and performance. Seems we're stuck with the thing. I don't think anyone is asking for immediate removal. At the very least we'd need a way to get the same performance - this has also been clear. Your new info also highlights the security benefits, and we'd have to address that too. Perhaps as we explore ways to improve robustness against use-after-free attacks for other, non-render-tree objects, we will find a solution that would be as effective as RenderArena even for renderers. Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] RenderArena: Teaching an old dog new tricks
On Thu, Nov 15, 2012 at 2:16 PM, Chris Evans cev...@chromium.org wrote: On Thu, Nov 15, 2012 at 11:49 AM, Geoffrey Garen gga...@apple.com wrote: On Nov 14, 2012, at 3:19 PM, Chris Evans cev...@chromium.org wrote: A first step might be to make it a platform define. For the Chromium platform we'd leave the define on -- there are some nice security properties we get from having the RenderObjects in their own spot. I'm happy to go in to more details if you want, but it's similar (although not identical) to the blog post linked by Brendan regarding Firefox. Not all WebKit consumers need weight things the same way as the Chromium port of course, but at least for us, the security win outweighs any quirks of RenderArena. r- Don't do this. Ok, no platform define for RenderArena. There's also an implicit r- on removing the thing, though, as we'd regress security(!!) and performance. Seems we're stuck with the thing. While I don’t want to further agitate the issue or go off on a tangent, and agree that we must address the security aspect before getting rid of RenderArena, only WebKit reviewers can r- patches written by other contributors. You’re not even supposed to set r- on your own patches. See http://www.webkit.org/coding/commit-review-policy.html - R. Niwa ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] RenderArena: Teaching an old dog new tricks
I had a few more thoughts on this email besides the fragmentation aspect. On Nov 15, 2012, at 12:22 AM, Chris Evans cev...@chromium.org wrote: It still seems to me like the key difference is vtable vs no vtable, It's an important difference, but if we partitioned in to two based on that difference alone, we'd have the following issues: 1) Classes with multiple vtables would spoil our day. The sheer number of classes in the vtable partition would practically ensure that an attacker could overlap data of their choosing on top of a secondary vtable. Overlap of attacker data onto a secondary vtable is also a concern with the DOM partition approach, but the chances of it being possible are much much lower. In the Pinkie Pie exploit case, the DOM arena solution made it impossible. Are you thinking about classes with multiple inheritence? A couple of thoughts on this: a) I think the majority of uses of multiple inheritence from classes with virtual methods (thereby leading to multiple vtables) is in subclasses of Node, e.g. in SVG. I don't see how this is much more a problem for broader-scope segregated allocation. b) This could be addressed by allocating classes with multiple vtables separately from ones with single vtables, if the cost is worth it. That seems to address the issue more directly than making Node subclasses the cutoff. Your comments on this make me worry that this mitigation might be overly tailored to the one specific exploit. 2) The Pinkie Pie exploit wasn't exclusively about the vtable. Before you can abuse an overlap with a freed vtable pointer, you need to defeat ASLR. This was partly achieved by overlapping the pointer and size of a freed WTF::Vector member with arbitrary data. (This is highly dangerous for DOM objects as it can lead to a direct arbitrary memory read or write primitive from Javascript!) Again, the sheer number of classes in a vtable partition would make a collision profitable to the attacker a statistical likelihood. Again, with a strict DOM arena, possibilities are really shut down. Back to the Pinkie Pie case, there wasn't any immediately useful overlap in either the 32-bit or 64-bit memory layout. Is defeating ASLR via this type of information disclosure useful in ways other than following up with an attack on a vtable? It seems like a lot of Node subclasses Another thought is that the CSSOM is likely to be pretty vulnerable to these kinds of attacks too. So a DOM-specific solution may be too narrow, but at the same time, adding more and more different arenas is likely to cause bad fragmentation in pathological cases. This is why I think doing the segregation at a coarser granularity is a promosing approach. Cheers, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] RenderArena: Teaching an old dog new tricks
On Nov 15, 2012, at 2:59 PM, Ryosuke Niwa rn...@webkit.org wrote: On Thu, Nov 15, 2012 at 2:16 PM, Chris Evans cev...@chromium.org wrote: On Thu, Nov 15, 2012 at 11:49 AM, Geoffrey Garen gga...@apple.com wrote: On Nov 14, 2012, at 3:19 PM, Chris Evans cev...@chromium.org wrote: A first step might be to make it a platform define. For the Chromium platform we'd leave the define on -- there are some nice security properties we get from having the RenderObjects in their own spot. I'm happy to go in to more details if you want, but it's similar (although not identical) to the blog post linked by Brendan regarding Firefox. Not all WebKit consumers need weight things the same way as the Chromium port of course, but at least for us, the security win outweighs any quirks of RenderArena. r- Don't do this. Ok, no platform define for RenderArena. There's also an implicit r- on removing the thing, though, as we'd regress security(!!) and performance. Seems we're stuck with the thing. While I don’t want to further agitate the issue or go off on a tangent, and agree that we must address the security aspect before getting rid of RenderArena, only WebKit reviewers can r- patches written by other contributors. You’re not even supposed to set r- on your own patches. See http://www.webkit.org/coding/commit-review-policy.html I think Chris merely meant that removing RenderArena without fixing those problems would appear to violate the project goals, and therefore a responsible reviewer would likely r- it. I don't think he intended to literally r- a patch. Cheers, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
[webkit-dev] r- your own patches [was: Re: RenderArena: Teaching an old dog new tricks]
On 16 November 2012 09:59, Ryosuke Niwa rn...@webkit.org wrote: While I don’t want to further agitate the issue or go off on a tangent, and agree that we must address the security aspect before getting rid of RenderArena, only WebKit reviewers can r- patches written by other contributors. You’re not even supposed to set r- on your own patches. See http://www.webkit.org/coding/commit-review-policy.html I see that page says 'Note that you should not put r+ nor r- on patches in such unofficial reviews' with respect to a non-reviewer doing a shadow review. I can't see the extrapolation from that to 'you can't r- your own patches'. I thought r-'ing your own patch was a relatively common practice when uploading a WIP patch, as a signal that 'I have no intention of landing this patch', and as a courtesy so a reviewer will not waste any time looking at it (unless specifically asked). I don't see why I wouldn't be allowed to r- my own patch? mike ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] r- your own patches [was: Re: RenderArena: Teaching an old dog new tricks]
On Thu, Nov 15, 2012 at 4:28 PM, Mike Lawther mikelawt...@google.com wrote: On 16 November 2012 09:59, Ryosuke Niwa rn...@webkit.org wrote: While I don’t want to further agitate the issue or go off on a tangent, and agree that we must address the security aspect before getting rid of RenderArena, only WebKit reviewers can r- patches written by other contributors. You’re not even supposed to set r- on your own patches. See http://www.webkit.org/coding/commit-review-policy.html I see that page says 'Note that you should not put r+ nor r- on patches in such unofficial reviews' with respect to a non-reviewer doing a shadow review. I can't see the extrapolation from that to 'you can't r- your own patches'. I thought r-'ing your own patch was a relatively common practice when uploading a WIP patch, as a signal that 'I have no intention of landing this patch', and as a courtesy so a reviewer will not waste any time looking at it (unless specifically asked). I don't see why I wouldn't be allowed to r- my own patch? It seems fine to r- your own patch. You can also clear the review flag if you're not interested in having someone review your patch. If you want to upload a work-in-progress patch, one pattern I use is the following: webkit-patch upload --no-review -m Work-in-progress That will avoid setting the review flag and will label the patch as a work in progress. Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] r- your own patches [was: Re: RenderArena: Teaching an old dog new tricks]
On Thu, Nov 15, 2012 at 4:28 PM, Mike Lawther mikelawt...@google.comwrote: On 16 November 2012 09:59, Ryosuke Niwa rn...@webkit.org wrote: While I don’t want to further agitate the issue or go off on a tangent, and agree that we must address the security aspect before getting rid of RenderArena, only WebKit reviewers can r- patches written by other contributors. You’re not even supposed to set r- on your own patches. See http://www.webkit.org/coding/commit-review-policy.html I see that page says 'Note that you should not put r+ nor r- on patches in such unofficial reviews' with respect to a non-reviewer doing a shadow review. I can't see the extrapolation from that to 'you can't r- your own patches'. I thought r-'ing your own patch was a relatively common practice when uploading a WIP patch, as a signal that 'I have no intention of landing this patch', and as a courtesy so a reviewer will not waste any time looking at it (unless specifically asked). r+ and r- flags are supposed to be set only by reviewers. If you wanted to withdraw your patch from the review queue, then you should be clearing r? flag, instead of setting r-. If you’re uploading a WIP patch, then it should not bear either r?, r-, or r+ flags. You can accomplish this by either not setting the flag when you upload a patch on Bugzilla, clearing flag on the Bugzilla, or using --no-review option on webkit-patch. A patch with r- should be a patch rejected by a reviewer, not a WIP patch the contributor decided not to proceed with. Otherwise, reviewers can’t differentiate the patches rejected by other reviewers and patches authors didn’t like (unless you recognize author’s IRC nickname). - R. Niwa ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] RenderArena: Teaching an old dog new tricks
On Thu, Nov 15, 2012 at 3:05 PM, Maciej Stachowiak m...@apple.com wrote: I had a few more thoughts on this email besides the fragmentation aspect. On Nov 15, 2012, at 12:22 AM, Chris Evans cev...@chromium.org wrote: It still seems to me like the key difference is vtable vs no vtable, It's an important difference, but if we partitioned in to two based on that difference alone, we'd have the following issues: 1) Classes with multiple vtables would spoil our day. The sheer number of classes in the vtable partition would practically ensure that an attacker could overlap data of their choosing on top of a secondary vtable. Overlap of attacker data onto a secondary vtable is also a concern with the DOM partition approach, but the chances of it being possible are much much lower. In the Pinkie Pie exploit case, the DOM arena solution made it impossible. Are you thinking about classes with multiple inheritence? A couple of thoughts on this: a) I think the majority of uses of multiple inheritence from classes with virtual methods (thereby leading to multiple vtables) is in subclasses of Node, e.g. in SVG. I don't see how this is much more a problem for broader-scope segregated allocation. b) This could be addressed by allocating classes with multiple vtables separately from ones with single vtables, if the cost is worth it. That seems to address the issue more directly than making Node subclasses the cutoff. One reason to make Node the cutoff is that UAF of DOM objects is often more dangerous because of the rich JS API the attacker can play with to take the effect of the memory corruption in so many different ways. So the non-vtable aspects of the exploitation get defended a little better due to fewer attacker opportunities. That said, I like the elegance of your single vtable suggestion. Let me think about it. Your comments on this make me worry that this mitigation might be overly tailored to the one specific exploit. It's a fair comment; and I did see your request for other examples in a previous mail. Actual UAF exploits are hard to come by because many people who write them are not incentivized to ever make them public. And the great example I do have cost $60k ;-) I'll work on getting more. Or, send me a bug which leaves a JS handle pointing to a freed underlying DOM node and I could see about weaponizing it. There are bunch of historical examples such as https://code.google.com/p/chromium/issues/detail?id=118273 which show attackers are going after freed DOM node pointers, though. We only have the bug and not an exploit. 2) The Pinkie Pie exploit wasn't exclusively about the vtable. Before you can abuse an overlap with a freed vtable pointer, you need to defeat ASLR. This was partly achieved by overlapping the pointer and size of a freed WTF::Vector member with arbitrary data. (This is highly dangerous for DOM objects as it can lead to a direct arbitrary memory read or write primitive from Javascript!) Again, the sheer number of classes in a vtable partition would make a collision profitable to the attacker a statistical likelihood. Again, with a strict DOM arena, possibilities are really shut down. Back to the Pinkie Pie case, there wasn't any immediately useful overlap in either the 32-bit or 64-bit memory layout. Is defeating ASLR via this type of information disclosure useful in ways other than following up with an attack on a vtable? Yeah. Once you have a valid address, you can use that (e.g. set the appropriate ArrayBuffer bytes at the right index) to be the basis for e.g. a WTF::Vector. And set the length to 0x. Now, assuming there's a JS API to make queries that are backed by the Vector, you get to read and possibly write arbitrary memory. If you can read arbitrary memory, you have a UXSS-class of vulnerability in most setups. If you can read or write arbitrary memory, you can target vtables, function pointers, stacks... bad news at this stage. The DOM arena approach does lead to less liklihood of being able to cause useful overlap to do any of the above steps. If you like, you could imagine the probability of useful attack for each step as being some function of the number of different classes in the same partition. The vision behind the DOM arena is to take the most dangerous UAFs and put them in a small partition. Cheers Chris It seems like a lot of Node subclasses Another thought is that the CSSOM is likely to be pretty vulnerable to these kinds of attacks too. So a DOM-specific solution may be too narrow, but at the same time, adding more and more different arenas is likely to cause bad fragmentation in pathological cases. This is why I think doing the segregation at a coarser granularity is a promosing approach. Cheers, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] r- your own patches [was: Re: RenderArena: Teaching an old dog new tricks]
On Nov 15, 2012, at 4:56 PM, Ryosuke Niwa rn...@webkit.org wrote: On Thu, Nov 15, 2012 at 4:28 PM, Mike Lawther mikelawt...@google.com wrote: On 16 November 2012 09:59, Ryosuke Niwa rn...@webkit.org wrote: While I don’t want to further agitate the issue or go off on a tangent, and agree that we must address the security aspect before getting rid of RenderArena, only WebKit reviewers can r- patches written by other contributors. You’re not even supposed to set r- on your own patches. See http://www.webkit.org/coding/commit-review-policy.html I see that page says 'Note that you should not put r+ nor r- on patches in such unofficial reviews' with respect to a non-reviewer doing a shadow review. I can't see the extrapolation from that to 'you can't r- your own patches'. I thought r-'ing your own patch was a relatively common practice when uploading a WIP patch, as a signal that 'I have no intention of landing this patch', and as a courtesy so a reviewer will not waste any time looking at it (unless specifically asked). r+ and r- flags are supposed to be set only by reviewers. If you wanted to withdraw your patch from the review queue, then you should be clearing r? flag, instead of setting r-. If you’re uploading a WIP patch, then it should not bear either r?, r-, or r+ flags. You can accomplish this by either not setting the flag when you upload a patch on Bugzilla, clearing flag on the Bugzilla, or using --no-review option on webkit-patch. A patch with r- should be a patch rejected by a reviewer, not a WIP patch the contributor decided not to proceed with. Otherwise, reviewers can’t differentiate the patches rejected by other reviewers and patches authors didn’t like (unless you recognize author’s IRC nickname). Personally I think it is ok to r- your own patch (it doesn't really violate the spirit of the rules about setting the review flag), but it's probably better practice to unset the review flag for the sake of clarity. Regards, Maciej___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
