GNOME Bugzilla – Bug 699752
GNOME Shell D-Bus interface
Last modified: 2013-11-04 15:30:55 UTC
Created attachment 243378 [details] Test1 results I tested the GNOME Shell 3.6.3.1 D-Bus methods from: name: org.gnome.Shell object path: /org/gnome/Shell interface: org.gnome.Shell with my fuzzer (https://github.com/matusmarhefka/dfuzzer). I did two tests through all of the GNOME Shell D-Bus methods. First testing managed to crash the GNOME Shell on the following call of the method ScreenshotArea: ScreenshotArea( 2147483647, 2147483647, 2147483647, -2147483648, "\fR3~,jc5?\_FzmxB%NaO?HmnW)3e+LBq~Rn,=R>]/z!(iyJnE);dQ*P'0cl;0,*-_x6|HQWULX]6O%"t/;V`E(dRkQmy^w';n:87kmi8CDLsJNeVi" ) (see the attachment 'Test1 results') Second testing crashed the GNOME Shell on calling the method ScreenshotArea again
Created attachment 243379 [details] Test2 results
Second testing crashed the GNOME Shell on calling the method ScreenshotArea again: ScreenshotArea( -2147483648, 2147483647, 2147483647, 2147483647, true, "PQbnvwe2FJY4UQ_E2Ei!O g"\0i"TTk%'Kp}CV-j}c~S2\vD"`EO]-N::8:nm&ptQaoq5|Y4`Vdo0[4/9V[v`+2zCLi1OZ#}9roNlG_JzE:+!N:;%v/b">]BgDQ7{T55E#d2GD]BfwMhDd!F[0*ZOd}7*LO&!a<Ca}V)") Immediately after the crash, the GNOME Shell restarted and the method ScreenshotWindow was called: ScreenshotWindow( false, true, false, "G1C!HhBR}Id{mEU~C(8\tR)='~.|6%`Z6"[^j}.eGr_5851X;I60x<N}8YzN^Y)t[aPC_^*'Of:d{" ) which caused another crash of the GNOME Shell from which it did not recover, only logout -> login helped. (see the attachment 'Test2 results')
This is interesting, but I'm not sure it's really a gnome-shell bug here. I mean, 2147483647 is obviously not a valid area size (65536 is the maximum the X protocol allows for screens and windows), so it almost legit that the shell crashes in this case, and making the common case more complex just to appease an automatic tester does not seem a good idea to me.
So if 65536 is maximum value, why it is not validated inside that method ? Or why you even used 32 bit integer type ?
a) Because we don't need to. It's a private interface used by gnome-screenshot and gnome-settings-daemon, and we can make assumptions on those (or fix the bugs there if they arise) b) Probably because everyone remembers 'i' and 'u', but less so for smaller types. After all, they're all doubles in JS, so it doesn't really matter.
Are you suggesting there's a security issue that can arise by crashing the shell?
I don't think there's a security issue, but I still think its good practice to validate arguments, and return a meaningful error instead of just crashing the desktop shell.
Jasper St. Pierre> No I am not aware of any security issues, but a robust application should handle such inputs and not crash, definitely not the core user interface like the GNOME Shell. Giovanni Campagna> I disagree it's a private interface, as any application connected to a session bus can call these methods. Imagine someone write his own application which will be using the ScreenshotArea method.
Created attachment 258919 [details] [review] screenshot: Extend ScreenshotArea parameter validation We currently only ensure that width and height are positive, so it is still possible to pass in values that don't make any sense at all (which may even result in a crash when exceeding limits imposed by X11). There is nothing to screenshot outside the actual screen area, so restrict the parameters to that.
Created attachment 258920 [details] [review] screencast: Fix disabling screencasts via session mode If screencasts are disabled, we return a DBus error, but still start the recording happily - add early returns in that case.
Created attachment 258921 [details] [review] screencast: Validate parameters of ScreencastArea ... just as we do for screenshots.
Review of attachment 258920 [details] [review]: Sure.
Review of attachment 258919 [details] [review]: ::: js/ui/screenshot.js @@ +83,3 @@ + x >= global.screen_width || y >= global.screen_height || + width <= 0 || height <= 0 || + width > global.screen_width - x || height > global.screen_height - y) { I prefer (x + width) > global.screen_width, which is more obvious that with a positive x and width, it also handles the (x >= global.screen_width) case above.
Review of attachment 258921 [details] [review]: same comments as first patch, and: ::: js/ui/screencast.js @@ +142,3 @@ + width <= 0 || height <= 0 || + width > global.screen_width - x || height > global.screen_height - y) { + extra whitespace @@ +147,3 @@ + "Invalid params"); + return; + extra whitespace
Attachment 258919 [details] pushed as f9f5004 - screenshot: Extend ScreenshotArea parameter validation Attachment 258920 [details] pushed as 9520e87 - screencast: Fix disabling screencasts via session mode Attachment 258921 [details] pushed as e8d9a4b - screencast: Validate parameters of ScreencastArea