New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can't make clickable workspaces with stripActions active #125

Closed
00dani opened this Issue Aug 16, 2013 · 20 comments

Comments

Projects
None yet
3 participants
@00dani
Copy link
Contributor

00dani commented Aug 16, 2013

xmobar currently strips out any <action> invocations found in the string passed through either StdinReader or XMonadPropLog, the two main ways to send workspace info from XMonad.

Unfortunately, this setup makes it impossible to implement what's probably the number one use of clickable-area support (judging from dzen-based XMonad configs): clickable workspaces, such that clicking on a workspace in the statusbar switches to it.

So why does xmobar do that? What's the rationale behind 85fdac2 ? Security is one possibility, but practically all usage of these two plugins will involve only a string controlled by the end-user, i.e., the log output of their window manager. Are there other reasons to be stripping actions?

@jaor

This comment has been minimized.

Copy link
Owner

jaor commented Aug 16, 2013

The reason we strip actions is that it's trivial to create a web page
whose title has an embedded action that would be interpreted in the
title bar displayed for a web browser and can cause arbitrary harm when
clicked, because actions execute arbitrary shell code.

A possibility would be to provide a plugin argument and let the user
decide in the configuration whether actions should be stripped. I think
that the flag should still default to false, because most people don't
worry about filtering the input right now.

We discussed options involving escaping of characters, but they also
require user intervention massaging the input string and again there
will be users that won't change their configurations (or write the
amount of code required for their window manager).

The discussion was on the mailing list:
http://projects.haskell.org/pipermail/xmobar/2013-June/000063.html

(As an aside, i've never been able to reproduce the bug described
there and the reporter dropped the ball, so i didn't strip that markup.)

I'm inclined to providing a configuration flag just for XMonadPropLog
that activates actions and then people with the specific need will find
out examples in the documentation of what to do, while the rest will be
safe by default. Or even adding an ActionablePropLog plugin...

Thoughts?

@00dani

This comment has been minimized.

Copy link
Contributor

00dani commented Aug 16, 2013

Hmm. I honestly don't see "Web pages with embedded malicious <action>s in their titles" to be a particularly significant attack vector, since xmobar isn't on the whole actually used that much and nothing else uses the same syntax for actions. Still, I suppose it is an attack vector.

Providing flags for the relevant plugins to disable action-stripping on a case-by-case basis seems the safest solution, preferably coupled with documentation on how to go about securing the window-titles part of the log when actions are enabled.

@00dani

This comment has been minimized.

Copy link
Contributor

00dani commented Aug 18, 2013

Is there an intended way to secure the window-titles part of the log, apart from the current "welp, no actions are allowed at all in the stdin"? For instance, does HTML-escaping the title work?

@jaor

This comment has been minimized.

Copy link
Owner

jaor commented Aug 18, 2013

No, there's currently no escaping implemented. I'm surely missing
something here, but i'm not sure how escaping avoids the (admittedly
unlikely, but oh well) attack: what prevents a window from having a
tittle that reads as the escaped characters that will then be converted
to an action by xmobar?

@00dani

This comment has been minimized.

Copy link
Contributor

00dani commented Aug 18, 2013

Well, the title would necessarily be escaped in the window manager, probably by encoding it in HTML entities. xmobar would then have to render &lt; as a literal < instead of interpreting it as the start of an <action>; this might be done by unencoding HTML entities in the string to be displayed after the parsing for <action>s and other formatting codes.

Does dzen2 have any way to check for or avoid this particular security hole, which would I imagine be equally applicable to its formatting language? Edit: Turns it it does. There's a dzenEscape function packaged with XMonad that's applied by default to the window title when using dzenPP. So is there any particular reason escaping couldn't be introduced for xmobar and included in the default setups in the same way?

@jaor

This comment has been minimized.

Copy link
Owner

jaor commented Aug 18, 2013

Well, the title would necessarily be escaped in the window manager

The bit that worries me about that is that some people won't take the
time to modify their window manager configurations to do the escaping
and, perhaps more importantly, that that escaping is not a trivial
operation (we can provide example code for doing that in xmonad, or in
sawfish because the latter is my window manager of choice, but not for
every window manager out there... and i've been trying hard to make
xmobar window manager-agnostic).

But perhaps we could have an EscapedStdinReader plugin that behaves
the way you describe, or a configuration flag for StdinReader that
activates HTML-escaping parsing... does that make sense?

@00dani

This comment has been minimized.

Copy link
Contributor

00dani commented Aug 18, 2013

That seems legitimate. The combination of:

  1. A flag for StdinReader, defaulting to off, that when active allows for arbitrary markup including actions in the stdin stream (or a separate plugin that allows arbitrary markup. Either works.)
  2. Documentation for common window managers indicating how to escape window titles properly when actions are enabled

should be sufficient to allow this feature without opening a security hole. (I expect xmonad-contrib would soon gain an xmobarEscape function anyway once such things are possible, which addresses my own requirement. And if the escaping needed is something generic, like using HTML entities, then implementing it for other window managers shouldn't be much of a chore.)

@jaor

This comment has been minimized.

Copy link
Owner

jaor commented Aug 18, 2013

That seems legitimate. The combination of:

  1. A flag for StdinReader, defaulting to off, that when active allows
    for arbitrary markup including actions in the stdin stream (or a
    separate plugin that allows arbitrary markup. Either works.)

I was thinking that, when the flag is off, "harmless" markup such as
setting colors or icons could be permitted, since most people use it
already with vanilla StdinReader to things like highlighting the current
workspace... it's a bit inconsistent, but less disruptive to our current
user base.

@00dani

This comment has been minimized.

Copy link
Contributor

00dani commented Aug 18, 2013

Sure, that's reasonable (and how it works currently anyway). The "unsafe" StdinReader setup would just enable <action>s (and anything else that could do dangerous stuff, if any such thing is later introduced, I suppose).

@jaor

This comment has been minimized.

Copy link
Owner

jaor commented Aug 18, 2013

A bit annoyingly, the only lightweight Haskell package for HTML-encoding
i've found, Web.Encodings, is marked as obsoleted, and all its
replacements seem rather heavy-weight.

I'm thinking of just decoding &lt; and &gt; and providing a simple
Haskell encoding function that people can cut and paste in their xmonad
configurations. We don't need full-blown HTML encoding
anyway... strictly speaking, we wouldn't even need &gt;, so an even
simpler possibility would be to only deal with &lt;...

@00dani

This comment has been minimized.

Copy link
Contributor

00dani commented Aug 18, 2013

Huh, I'd expected there to be a usable HTML-encoder available, but hadn't actually checked whether this was indeed the case.

For doing it manually, it'd be necessary to deal with both &lt; and &amp; at a minimum to ensure the original window title is preserved. &gt; indeed isn't really required.

@jaor

This comment has been minimized.

Copy link
Owner

jaor commented Aug 18, 2013

For doing it manually, it'd be necessary to deal with both < and
& at a minimum to ensure the original window title is
preserved. > indeed isn't really required.

Yes. Another possibility, if we're going to provide our own manual
solution, is that escaping consists of something even simpler, e.g.,
converting all open brackets to << in the encoding function...

@00dani

This comment has been minimized.

Copy link
Contributor

00dani commented Aug 18, 2013

Ah, yes, that'd work too. Should be easy both to encode and decode, for that matter. It'd incidentally be remarkably similar to how dzen handles this problem (escaping its ^ metacharacter by doubling to ^^).

@00dani

This comment has been minimized.

Copy link
Contributor

00dani commented Aug 20, 2013

Incidentally, for what it's worth, I just actually tried to attack myself through this security vector. I made an HTML file:

<title>&lt;action=xclock>EEEEVIL&lt;/action></title>

And found that, although other actions in my StdinReader are working (I've simply removed the stripActions call from my local xmobar build), when I opened the page up and tried clicking on the EEEEVIL in the window title it didn't start an xclock.

Thinking it might be related to the &lt; I used, I tried writing it "literally":

<title><action=xclock>EEEEVIL</action></title>

And this still didn't start an xclock when clicked!

I have no special escaping logic in my XMonad configuration; all I do for ppTitle is colour it (with xmobarColor). Note that I tried making ppTitle also wrap the whole title in the above xclock action, and that did start xclock when done, so it's not that actions aren't being recognised at all or anything.

It seems like the browsers themselves are already escaping or stripping stuff from the window title before it's passed on to X, or something along those lines? (I tried in Firefox, Chromium, and uzbl, and all had the same behaviour.) If browsers indeed are already making the title safe (somehow), does this mean there isn't really a security hole in allowing actions?

@jaor

This comment has been minimized.

Copy link
Owner

jaor commented Aug 20, 2013

On Tue, Aug 20 2013, 00Davo wrote:

Incidentally, for what it's worth, I just actually tried to attack myself through this security vector. I
made an HTML file:

<title><action=xclock>EEEEVIL</action></title>

Didn't you try to also escape the >s?

  <title>&lt;action=xclock&gt;EEEEVIL&lt;/action&gt;</title>
@00dani

This comment has been minimized.

Copy link
Contributor

00dani commented Aug 20, 2013

Didn't you try to also escape the >s?

Oh, yep. I tried that too, with identical results.

However, on further testing I've found that it's not the browsers doing whatever weird escaping is happening, because I just tried this:

mkdir -p '<action=xclock>EVIL</action>'
cd !$

The window title shows the current directory as ~/EVIL—which in itself might mean we need some kind of escaping support, since that's not the correct current directory—but clicking on the "EVIL" still doesn't start an xclock.

@00dani

This comment has been minimized.

Copy link
Contributor

00dani commented Aug 20, 2013

Oh. That's what's going on, okay.

Turns out that at least the darcs version of xmonad-contrib has another PP field called ppTitleSanitize which is run over the window title first, and that one does have escaping logic. (Specifically, it defaults to xmobarStrip . dzenEscape.) Which means there's still a security hole in the general case. It does also mean that anyone on darcs XMonad is automatically secured against this hole, though.

Sorry to have led us on that little wild-goose chase there.

@jaor

This comment has been minimized.

Copy link
Owner

jaor commented Aug 20, 2013

OK... to double check, i just tried with my setup (where the title
doesn't go through any xmonad-contrib code), and xclock is indeed
started... so back to our << plans...

@00dani

This comment has been minimized.

Copy link
Contributor

00dani commented Aug 20, 2013

Yep, guess so. Escaping < to << (and recognising such in xmobar as a literal <) seems a sufficiently lightweight and usable scheme; it's not as ubiquitous as using HTML entities but is probably simpler for window managers to implement (and only requires one magic metacharacter instead of rendering ampersands magic too, as HTML-entity encoding would).

@thiago-negri

This comment has been minimized.

Copy link
Contributor

thiago-negri commented Sep 10, 2013

I've created a pull request that creates a new plugin to address this, see #126. It was a quick hack just to get clickable workspaces working on my environment.

@jaor jaor closed this in #128 Sep 12, 2013

mamash pushed a commit to joyent/pkgsrc-wip that referenced this issue Nov 20, 2013

szptvlfn
Update to 0.19
changes from Release notes:
## Version 0.19 (October 27, 2013)

As of this release, the old bug tracker at Google code is deprecated.
Please use [Github's tracker] for new bugs.

_New features_

  - New monitor `BatteryN`, a variant of `BatteryP` that lets you
    specify the name of the monitor in the template.
  - Support for configuration file living in `XDG_CONFIG_HOME` (see
    [github #99]).
  - `Com` uses safer `runInteractiveProcess` instead of spawning a
    shell (David McLean).  If you're using shell expansion in your
    `Com` (e.g. "~/bin/script") here's a workaround: `Run Com
    "/bin/bash" ["-c", "~/bin/script"]` (cf. [github #127]).
  - New plugin `UnsafeStdinReader` that allows actions from stdin.
    Now it's possible to have clickable workspaces!
    (Thiago Negri, see [github #125]).
  - New monitor configuration option (`-x` or `--nastring`) that allows
    specifying what string to display when a monitor is not available
    (defaulting to "N/A"). Cf. [github #119].

_Bug fixes_

  - Using the width options `-w`, `-m` and `-M` in battery monitors
    watts display ([github #118]).
  - Using the `-d` option in `CoreTemp` ([github #115])
  - Fix for systems not supporting PCRE regular expressions: we use
    now BCEs, so regex-compat should be enough everywhere (see
    [github #117]).
  - Weather monitor: support for stations without name (Sergei
    Trofimovich, [issue #65]).

[Github's tracker]: https://github.com/jaor/xmobar/issues
[github #99]: jaor/xmobar#115
[github #115]: jaor/xmobar#115
[github #117]: jaor/xmobar#117
[github #125]: jaor/xmobar#125
[issue #65]: http://code.google.com/p/xmobar/issues/detail?id=65
[github #118]: jaor/xmobar#118
[github #119]: jaor/xmobar#119
[github #127]: jaor/xmobar#127
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment