ah, i forgot to mention: this machine doesn't have a physical "PrtSc" button, so i set up one of the function keys to trigger the "Save a screenshot to Pictures" action via Preferences » Keyboard » Shortcuts » Screenshots

I suspect the attack would work just as well on a machine (or USB kbd) that has a PrtSc button without making the preferences change, but i haven't tested that.

Ouch, this is a serious issue indeed.

So what we have here is basically two issues:
1) Taking a screenshot causes a memory leak
2) Crashing the shell in the lock screen gives you access to the locked session.

Both don't sound easy to fix at all, but #2 is the more critical one, I think.

A workaround would be disabling the option to take screenshots in the lock screen, but it's fragile because if someone finds some other way to crash the shell when locked (and I don't doubt that they will), we'll have the same issue again.

2) has already been taken care of. It was #691987.

The issue is that for a short period of time, while gnome-shell is restarting, we show the raw X windows.

Created attachment 287237 [details] [review]
Only allow screenshots in the user session

We currently disallow screencasts in all session types but the user session to
prevent someone with access to the machine from filling up the disk with
screencasts. Screenshots can be abused the same way so restrict them as well.

(In reply to comment #2)
> Ouch, this is a serious issue indeed.
> 
> So what we have here is basically two issues:
> 1) Taking a screenshot causes a memory leak

Doesn't look like a leak. What happens is that when we take a screenshot we create a thread that writes the data to disk (which holds a reference on the data). So if you flood the system with screenshot requests you end up with tons of threads that holds the screenshot data. 

There aren't infinitely number of threads that can be run at once and the disk writes take time. Both together cause that "leak" ... we probably should rate limit the screenshot requests to avoid that.

> 2) Crashing the shell in the lock screen gives you access to the locked
> session.

It doesn't until the restart takes a lot of time there is a time window where the windows are accessible. If you hit the OOM killer lots of stuff needs to get paged in and thus that window becomes bigger. So a crash by itself is not enough to access things. 

Not sure what crazy side effects that might have if any but ... Jasper can we simply unmap all windows when we lock and map them on unlock?

> A workaround would be disabling the option to take screenshots in the lock
> screen, but it's fragile because if someone finds some other way to crash the
> shell when locked (and I don't doubt that they will), we'll have the same issue
> again.

We should do that anyway (see commit message).

Created attachment 287244 [details] [review]
shell-screenshot: Add rate limiting

We currently allow infinite number of screenshot requests to be active at
the same time, which can "dos" the system and cause OOM.

So limit the number of screenshots requests and simply fail if the user tries
to do more at once.

Review of attachment 287244 [details] [review]:

Wouldn't it be simpler to just allow one screenshot at a time?  Just have a boolean "screenshot_running" or something.

(In reply to comment #7)
> Review of attachment 287244 [details] [review]:
> 
> Wouldn't it be simpler to just allow one screenshot at a time?  Just have a
> boolean "screenshot_running" or something.

Indeed that makes sense.

Created attachment 287248 [details] [review]
shell-screenshot: Only allow one screenshot request at a time

We currently allow infinite number of screenshot requests to be active at
the same time, which can "dos" the system and cause OOM.

So fail subsequent requests when a screenshot operation is already running.

(In reply to comment #5)
> we probably should rate limit the screenshot requests to avoid that.

Yes. gnome-settings-daemon used to disable auto-repeat for all but a select few keybindings (volume/brightness being the only ones I recall off-hand), we should probably start doing the same.


> > A workaround would be disabling the option to take screenshots in the lock
> > screen, but it's fragile because if someone finds some other way to crash the
> > shell when locked (and I don't doubt that they will), we'll have the same issue
> > again.
> 
> We should do that anyway (see commit message).

No strong opinion, but allowing screenshots when locked was not accidental - there are cases where it comes in handy (reviews, bug reports, ...), and the potential for filling up the disc is much smaller than with screencasting (depending on the max-screencast-length setting, starting recording once vs. hitting PrintScrn for hours).

Review of attachment 287248 [details] [review]:

::: src/shell-screenshot.c
@@ +43,3 @@
 
+/* Used to rate limit the screenshot requests */
+static gboolean _screenshot_running = FALSE;

I don't see why this needs to be separate from the instance struct - in fact, with screenshots being limited to one at a time, there's no need anymore to keep screenshot_data separate either ...

(In reply to comment #11)
> Review of attachment 287248 [details] [review]:
> 
> ::: src/shell-screenshot.c
> @@ +43,3 @@
> 
> +/* Used to rate limit the screenshot requests */
> +static gboolean _screenshot_running = FALSE;
> 
> I don't see why this needs to be separate from the instance struct - in fact,
> with screenshots being limited to one at a time, there's no need anymore to
> keep screenshot_data separate either ...

Well done it that way because ShellScreenshot is not a singleton and the shell uses a new instance for each request. But sure we could change that as well.

(In reply to comment #10)
> (In reply to comment #5)
> > we probably should rate limit the screenshot requests to avoid that.
> 
> Yes. gnome-settings-daemon used to disable auto-repeat for all but a select few
> keybindings (volume/brightness being the only ones I recall off-hand), we
> should probably start doing the same.

Restricting screenshots to one at a time makes sense to me, however we should still look into addressing the auto-repeat issue(*) - users can set up shortcuts for arbitrary commands(**), and we don't have a way to filter out the potentially expensive ones (opening 10 gimp instances or so) ...


(*)  not necessarily for 3.14.1 though
(**) at least those are disabled on the lock screen

(In reply to comment #12)
> Well done it that way because ShellScreenshot is not a singleton and the shell
> uses a new instance for each request. But sure we could change that as well.

Oh, missed that - patch looks good to me then. Though maybe we want to do what we do for Screencasting, and limit screenshots to one per sender at any given time?

(In reply to comment #13)
> (In reply to comment #10)
> > (In reply to comment #5)
> > > we probably should rate limit the screenshot requests to avoid that.
> > 
> > Yes. gnome-settings-daemon used to disable auto-repeat for all but a select few
> > keybindings (volume/brightness being the only ones I recall off-hand), we
> > should probably start doing the same.
> 
> Restricting screenshots to one at a time makes sense to me, however we should
> still look into addressing the auto-repeat issue(*) - users can set up
> shortcuts for arbitrary commands(**), and we don't have a way to filter out the
> potentially expensive ones (opening 10 gimp instances or so) ...

There are almost infinite ways for a user to screw himself when the session is active. So I am not too worried about this case (unless it can happen on accident). The look screen is a different story though because it supposed to provide some kind of "protection".

(In reply to comment #15)
> So I am not too worried about this case (unless it can happen on
> accident). The look screen is a different story though because it supposed to
> provide some kind of "protection".

I agree that the lock screen is different, in the session the issue becomes just a polish thing - repeating a shortcut action when the key is not released quickly enough is just wrong for 99.9% of all shortcuts.

(In reply to comment #16)
> (In reply to comment #15)
> > So I am not too worried about this case (unless it can happen on
> > accident). The look screen is a different story though because it supposed to
> > provide some kind of "protection".
> 
> I agree that the lock screen is different, in the session the issue becomes
> just a polish thing - repeating a shortcut action when the key is not released
> quickly enough is just wrong for 99.9% of all shortcuts.

Oh yeah I don't  disagree with you. That comment meant to say "yes does not have to happen for 3.14.1".

(In reply to comment #14)
> (In reply to comment #12)
> > Well done it that way because ShellScreenshot is not a singleton and the shell
> > uses a new instance for each request. But sure we could change that as well.
> 
> Oh, missed that - patch looks good to me then. Though maybe we want to do what
> we do for Screencasting, and limit screenshots to one per sender at any given
> time?

Yeah can do that. We should be consistent here.

Created attachment 287251 [details] [review]
shell-screenshot: Only allow one screenshot request at a time per sender

We currently allow infinite number of screenshot requests to be active at
the same time, which can "dos" the system and cause OOM.

So fail subsequent requests for the same sender when a screenshot operation
is already running.

(In reply to comment #3)
> 2) has already been taken care of. It was #691987.
> 
> The issue is that for a short period of time, while gnome-shell is restarting,
> we show the raw X windows.

Yes, and for that short period of time those windows are not only shown (which is a bad enough privacy issue on it's own), but also accept input (which makes the already-bad issue even worse). Considering the fact that we can't never be 100% sure that the shell will never crash during the lock screen, it would be nice if you could hide those windows even if the shell crashes when the screen is locked.

Created attachment 287270 [details] [review]
shell-screenshot: Only allow one screenshot request at a time per sender

We currently allow infinite number of screenshot requests to be active at
the same time, which can "dos" the system and cause OOM.

So fail subsequent requests for the same sender when a screenshot operation
is already running.

---

Moved the screenshot_data stuff into ShellScreenshotPrivate

One other hardening measure would be to forcefully terminate the entire user session if the gnome-shell exits abnormally while serving as the lockscreen.

I don't know exactly how to do this, though, or whether this usability/security tradeoff is acceptable to the GNOME project.

(In reply to comment #22)
> One other hardening measure would be to forcefully terminate the entire user
> session if the gnome-shell exits abnormally while serving as the lockscreen.
> 
> I don't know exactly how to do this, though, or whether this usability/security
> tradeoff is acceptable to the GNOME project.

I really rather if we didn't. Losing your work due to a shell crash would not be nice at all.

drago01 suggested a more interesting solution in comment 5, but I don't know how feasible it is. (would the shell be able to restore the windows if they were unmaped and then it crashed in the lock screen, for example?)

(In reply to comment #23)
> I really rather if we didn't. Losing your work due to a shell crash would not
> be nice at all.

To be clear, i'm talking about terminating the session on gnome-shell abnormal failure *only* when when it's serving as the lockscreen.

A gnome-shell crash during a normal session shouldn't be affected.

(In reply to comment #24)
> To be clear, i'm talking about terminating the session on gnome-shell abnormal
> failure *only* when when it's serving as the lockscreen.

Still, that would only change
  "expose open windows until the shell restarts"
to
  "expose open windows until the session is terminated"

There is nothing preventing users from locking the screen when they have unsaved work around, so there is indeed the potential to with users' data, for the little gain of making the window in which information is exposed a bit smaller.

This is being discussed on oss-security for a possible CVE assignment:

 http://www.openwall.com/lists/oss-security/2014/09/29/17
 http://www.openwall.com/lists/oss-security/2014/10/02/35
 http://www.openwall.com/lists/oss-security/2014/10/02/38

Review of attachment 287270 [details] [review]:

I can still DOS the shell :-(
Not sure that's due to the logic bug pointed out below, or whether we need to limit screenshots a bit more ...

::: js/ui/screenshot.js
@@ +72,3 @@
 
+    _ensureShooterForSender: function(sender) {
+        let shooter = this._screenShooter.get(sender);

I wonder if it'd be worth handling uniqueness also in JS (something like:

  _createScreenshot(invocation) {
    let sender = invocation.get_sender();
    if (this._shooters.has(sender) {
        invocation.return_value(...);
        return null;
    }
    shooter = ...;
    return shooter;
  };

  let screenshot = this._createShooter(invocation);
  if (!screenshot)
      return;

That would make it easier to limit screenshots further in the future if it turns out to be desirable (just do GLib.timeout_add(DEFAULT, 200, _removeShooter) in _onScreenshotComplete).

@@ +97,3 @@
+        this._screenShooter.delete(sender);
+
+        return true;

Unused return value (I know this is following the screencast code, but it's not unused there)

@@ +149,1 @@
         screenshot.screenshot_area (x, y, width, height, filename,

So this doesn't quite work:

 (1) call Screenshot
    -> create object, start taking screenshot
 (2) call Screenshot
    -> screenshot returns error, is removed from map
 (3) call Screenshot
    -> create object, start taking 2nd screenshot
  ...

In other words, rather than limiting screenshots to one-per-sender, we just filter out every other invocation.

::: src/shell-screenshot.c
@@ +50,2 @@
 {
+  g_type_class_add_private (klass, sizeof (ShellScreenshotPrivate));

We could start using G_DEFINE_TYPE_WITH_PRIVATE

@@ +433,3 @@
+  ShellScreenshotPrivate *priv = screenshot->priv;
+
+  if (priv->is_running) {

Or maybe if (priv->filename != NULL) ?

@@ +535,3 @@
+  if (priv->is_running) {
+    if (callback)
+      callback (screenshot, FALSE, NULL, "");

So this is a bit weird: if we're already taking a screenshot, we return directly ...

@@ +539,3 @@
+  }
+
+  priv->filename = g_strdup (filename);

... otherwise we set up our data ...

@@ +546,3 @@
     {
+      priv->filename_used = g_strdup ("");
+      result = g_simple_async_result_new (G_OBJECT (screenshot), on_screenshot_written, NULL, shell_screenshot_screenshot_window);

... and do an early return (but going through on_screenshot_written this time, to free the data) in case there's no focus window.
Those should really use the same "if (priv->is_running || !window)" code path (I don't see why we should go through GAsyncResult here, but don't really mind either way)

(In reply to comment #27)
> Review of attachment 287270 [details] [review]:
> 
> I can still DOS the shell :-(
> Not sure that's due to the logic bug pointed out below, or whether we need to
> limit screenshots a bit more ...

From a quick test, this snippet appears to do the job for me:

--- a/js/ui/screenshot.js
+++ b/js/ui/screenshot.js
@@ -107,13 +107,14 @@ const ScreenshotService = new Lang.Class({
     },

     _onScreenshotComplete: function(obj, result, area, filenameUsed, flash, invocation) {
-        if (flash && result) {
-            let flashspot = new Flashspot(area);
-            flashspot.fire();
+        if (result) {
+            if (flash) {
+                let flashspot = new Flashspot(area);
+                flashspot.fire();
+            }
+            this._removeShooterForSender(invocation.get_sender());
         }

-        this._removeShooterForSender(invocation.get_sender());
-
         let retval = GLib.Variant.new('(bs)', [result, filenameUsed]);
         invocation.return_value(retval);
     },

Created attachment 287647 [details] [review]
shell-screenshot: Only allow one screenshot request at a time per sender

We currently allow infinite number of screenshot requests to be active at
the same time, which can "dos" the system and cause OOM.

So fail subsequent requests for the same sender when a screenshot operation
is already running.

---

Adressed review comments.

Review of attachment 287647 [details] [review]:

'needs-work' because some rebase broke it, the comments are really just nits

::: js/ui/screenshot.js
@@ +92,3 @@
+        }
+
+        return this. _ensureShooterForSender(sender);

The function is only called here, and the code now boils down to:

  if (!shooters.has(sender)) {
      s = shooters.get(sender);
      if (!s)
          s = new Screenshot();
  }

which is a bit silly. But more importantly, the sneaky whitespace breaks everything :-)

::: src/shell-screenshot.c
@@ +53,3 @@
 shell_screenshot_init (ShellScreenshot *screenshot)
 {
+  screenshot->priv = SHELL_SCREENSHOT_GET_PRIVATE (screenshot);

shell_screenshot_get_instance_private ()

Created attachment 287671 [details] [review]
shell-screenshot: Only allow one screenshot request at a time per sender

We currently allow infinite number of screenshot requests to be active at
the same time, which can "dos" the system and cause OOM.

So fail subsequent requests for the same sender when a screenshot operation
is already running.

---

<drago01> fmuellner: http://paste.fedoraproject.org/138866/34036314/ I have added another change
<drago01> fmuellner: currently when I try to "dos" I get screens of the flash so I changed it to wait for the flash to finish as well
<drago01> not sure if we want that but screenshooting the flash seems wrong

Review of attachment 287671 [details] [review]:

(In reply to comment #31)
> <drago01> not sure if we want that but screenshooting the flash seems wrong

Makes sense to me, but then you need to also call removeShooter() for "flash-less" screenshots ...

::: src/shell-screenshot.h
@@ +22,3 @@
 #define SHELL_IS_SCREENSHOT_CLASS(klass)   (G_TYPE_CHECK_CLASS_TYPE ((klass), SHELL_TYPE_SCREENSHOT))
 #define SHELL_SCREENSHOT_GET_CLASS(obj)    (G_TYPE_INSTANCE_GET_CLASS ((obj), SHELL_TYPE_SCREENSHOT, ShellScreenshotClass))
+#define SHELL_SCREENSHOT_GET_PRIVATE(obj)  (G_TYPE_INSTANCE_GET_PRIVATE ((obj), SHELL_TYPE_SCREENSHOT, ShellScreenshotPrivate))

Unused

Comment on attachment 287671 [details] [review]
shell-screenshot: Only allow one screenshot request at a time per sender

Attachment 287671 [details] pushed as f02b007 - shell-screenshot: Only allow one screenshot request at a time per sender

Pushed with the "flashless" case fixed and without the unused macro.

Review of attachment 287237 [details] [review]:

We agreed on IRC that the best compromise here is to simply only allow screenshots to be saved to the clipboard while the screen is locked.
That way taking screenshots is not impossible but someone with access to the machine can't simply abuse it to fill the disk.

Interesting, thanks for reporting back to those of us who weren't on IRC.

I just tested on my debian sid system, and it looks to me like the clipboard is normally cleared across screenlock/unlock.

i did: 
 * select some text with the mouse
 * copy it to the clipboard
 * paste it to convince myself that it was in the clipboard
 * wait for the screen to lock
 * unlock the screen
 * try to paste again: nothing is produced

I don't know where the clipboard is cleared -- at screenlock, or at unlock -- if it is cleared at unlock, the screenshotting to the clipboard won't work.

If it is cleared at lock, then this proposal means that an unauthenticated person can inject the screenshot into the clipboard during a screenlock, which seems a little weird.  Can an unauthenticated person inject anything else into the clipboard while the screen is locked?

(In reply to comment #35)
> I just tested on my debian sid system, and it looks to me like the clipboard is
> normally cleared across screenlock/unlock.
> 
> i did: 
>  * select some text with the mouse
>  * copy it to the clipboard
>  * paste it to convince myself that it was in the clipboard
>  * wait for the screen to lock
>  * unlock the screen
>  * try to paste again: nothing is produced

Yes, the clipboard is cleared on lock. Otherwise the password entry can leak information by unmasking it (right-click -> show text) and pasting the clipboard's content.


> If it is cleared at lock, then this proposal means that an unauthenticated
> person can inject the screenshot into the clipboard during a screenlock, which
> seems a little weird.

Weird maybe, but is it actually harmful? This is of course not the reason to allow this, but there are legitimate use cases for taking screenshots in the lock screen (illustrating a bug, showing off a feature, ...). Those are not terribly important, but if the strongest arguments against them is that it's a little weird when used by someone else, I don't see why we should not account for them :-)



>  Can an unauthenticated person inject anything else into
> the clipboard while the screen is locked?

Yes. Hit Escape to reveal the password entry, right click and select "Show Text" - after that you can enter arbitrary text and copy it to the clipboard.

This is now CVE-2014-7300

Created attachment 287941 [details] [review]
media-keys: Disallow screenshots when locked

Allowing random people to create files in the user's home folder
while the screen is locked is unexpected at best, so block the
corresponding shortcuts (while still allowing taking screenshots
to the clipboard for bug reports/reviews etc.).
While at it, adjust the shortcut for screencasts as well (though
the change their is purely informational, as screencasts are already
blocked by the shell while locked).

Review of attachment 287941 [details] [review]:

Looks fine to me

Attachment 287941 [details] pushed as e6e81ae - media-keys: Disallow screenshots when locked

Let's get this off the blocker list then ...

Hi.
Florian, you're right to entirely turn off this!

A lockscreen should allow only to authenticate as user a currently logged in user, to create a new session for a other user or or to properly shutdown the box, if no one is logged in. If the lockscreen allows to create screenshots and write data to the home-dir (or a subdir), this is a security issue.

If someone wants to create a screenshot of the lock-screen, you could use "fbgrab". Honestly, I recommend it because it offers some nice features e.g. specificy delay in seconds, device or console :-)

*** Bug 699844 has been marked as a duplicate of this bug. ***