Closed Bug 772998 Opened 12 years ago Closed 12 years ago

Persistent (stored) XSS in profile allowing client side code to be executed via private Messages.

Categories

(support.mozilla.org :: General, defect)

defect
Not set
critical

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: cocking70, Assigned: jsocol)

Details

(Keywords: sec-critical, wsec-xss)

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/535.11 (KHTML, like Gecko) Chrome/17.0.963.79 Safari/535.11

Steps to reproduce:

After creating an account and going to the profile editor (https://support.mozilla.org/en-US/users/edit) I changed "Display name:" to:

"/><script>alert(document.cookie);</script>

I then tried to send a private message (https://support.mozilla.org/en-US/message/new) to this account, the client side code was be executed. 


Actual results:

An alert box was created showing cookie information. This persistent XSS could be used to steal sessions from other users when they attempt to private message me without any user interaction.

Proof of concept:
(using Firefox) In the "To" box (found here: https://support.mozilla.org/en-US/message/new) if you type my account name "testaudit" (without the quotes) you will see an alert box with your cookie information displayed. 


Expected results:

Username should appear in TO box allowing private message to be sent.
I would also like to include that if i created multiple accounts and
used the following display names:
a"/><script>blahblah</script>
b"/><script>blahblah</script>
...
z"/><script>blahblah</script>


As the To function searches for display names as well as user names &
the auto complete feature is enabled, it would execute my client side
script after the first character is entered. This means the session of
every single person who sends a PM would be stolen without any user
interaction.

Hopefully you agree, this bug could have been devastating.
Group: core-security → websites-security
Severity: normal → critical
Whiteboard: ws:critical
Whiteboard: ws:critical
Ricky, let's tackle this ASAP and get a fix out today if possible. Looks like we're using string concatenation[1] when we should be assigning innerHTML (or adding a new function that escapes and interpolates).

[1] https://github.com/mozilla/kitsune/blob/master/media/js/users.autocomplete.js#L40
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → james
Landed the commit to master. Will push to stage then prod shortly.

https://github.com/mozilla/kitsune/commit/9416717ed28cf553020b54976d62a8e61b5e0ff5
Just landed in prod.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Once this is verified let's remove it from the websites-security group. mfuller can you help us verify this?
Hi, I can verify this, but the page the OP listed (https://support.mozilla.org/en-US/message/new) is returning a "not found" for me.

Also, is the "display name" written to a database? If so, the correct fix for this cannot be JavaScript / blacklist-based. For example, a user can craft a JavaScript injection that does not include the "unsafe" characters (&, <, >, etc).

If the name is retrieved from a database, it must have a server-side escaping function before it can be used in JS or on the page.

Let me know and I can look at this further

Thanks!
Matt
Go here: https://support.mozilla.org/en-US/messages
then click: "New Message"

this will take you to /messages/new
(In reply to Matt Fuller :mfuller from comment #8)

> Also, is the "display name" written to a database? If so, the correct fix
> for this cannot be JavaScript / blacklist-based. For example, a user can
> craft a JavaScript injection that does not include the "unsafe" characters
> (&, <, >, etc).

Yes, of course it's written to a database. The string is never executed as JavaScript, it's always treated as a string literal. The issue here was that it was being concatenated as HTML, not as JS. Escaping all HTML special characters (which is literally all that the Python version of this function does) mitigates the HTML-injection vector.

There are certainly injections that don't use any HTML special chars, (stuff like [!{}]+[+[]])[+[]]) but those are meaningless in HTML.

> If the name is retrieved from a database, it must have a server-side
> escaping function before it can be used in JS or on the page.

It is escaped on the server insofar as it is guaranteed to be valid JSON, which means the string on the server side turns into a JS literal string before being sent to the browser.

It's entirely possible there's a vector I don't know about, so if you could explain this further, maybe give an example, that would be great. We're talking about a JSON-encoded JS string that is never evaled as JS. It was inserted into HTML, and now it's inserted via a function that escapes all HTML special chars.
(In reply to cocking70 from comment #9)
> Go here: https://support.mozilla.org/en-US/messages
> then click: "New Message"
> 
> this will take you to /messages/new

You do need to be logged in, of course.
:james - thanks for the clarification. I was asking about whether it was escaped because there may be other locations where the name is displayed as raw HTML (a classic XSS). I understand what you're referring to with the function, that is sufficient. My concern was if the following event occurred:
- User inserts malicious payload as display name
- Display name is included as part of a static HTML generated page (reflected)
- Code is executed.

That doesn't appear to be the case, as you've described.

Thanks for explaining, I've verified this fix.
Matt
Status: RESOLVED → VERIFIED
(In reply to Matt Fuller :mfuller from comment #12)
> :james - thanks for the clarification. I was asking about whether it was
> escaped because there may be other locations where the name is displayed as
> raw HTML (a classic XSS).

Output is escaped at the template level in cases like that. Most user-generated content is auto-escaped by our template layer. Interpolation in general marks a string as "unsafe" to the template layer.

Some (e.g. that which must be wrapped non-trivially in HTML, usually for L10n reasons, like 'welcome, <a href="/profile/url">username</a>') uses the |fe filter of jingo[1]. This escapes user content before interpolating and marking the whole thing as safe.

So, user data is either escaped outright or escaped before being interpolated (like this JS function now does) or passed through Bleach[2].

Not claiming that we're perfect, but we're close, and those are our mitigations for reflected XSS.

[1] https://github.com/jbalogh/jingo/blob/master/jingo/helpers.py#L30
[2] https://github.com/jsocol/bleach

The code here operates exactly like the |fe filter.
Status: VERIFIED → RESOLVED
Closed: 12 years ago12 years ago
(In reply to Ricky Rosario [:rrosario, :r1cky] from comment #6)
> Just landed in prod.

(In reply to Matt Fuller :mfuller from comment #12)
> Thanks for explaining, I've verified this fix.

Can we go ahead and remove this from the websites-security group? I'd like to use it as an example.
Thanks everyone - I've removed it from websites-security.
Group: websites-security
Flags: sec-bounty+
The reporter would prefer to be credited in the HoF as "Daniel Cocking" (per mail to security@)
You need to log in before you can comment on or make changes to this bug.