Bugzilla

Comment 2

•

12 years ago

While we should definitely add better validation to sslbase (and lots of other options), I think it's reasonable to say that if a user has tweakparams, he/she effectively has admin access already. Similar to how editusers is effectively admin, even without the bit explicitly set. As such, I don't consider this a valid security issue.

Comment 3

•

12 years ago

The string entered for the sslbase parameter must start with https:// and so you cannot type the string specified in point 2.

Moreover, we trust parameters set by administrators. We don't do a full validation on purpose. If you cannot trust your admins, then you are in trouble. This is not a bug.

Assignee: rforbes → administration

Group: bugzilla-security

Status: UNCONFIRMED → RESOLVED

Closed: 12 years ago

Component: Bugzilla-General → Administration

Resolution: --- → INVALID

Whiteboard: [verif?]

Comment 4

•

12 years ago

(In reply to Mario Gomes from comment #0)
> PoC: https://j6j9djxoqkc0.demo.bugzilla.org/

I don't seem to be getting any type of XSS warning on the above URL. Can you show me exactly how you were getting it?

Reporter

Comment 5

•

12 years ago

try it again. Anyway it can be exploited by the old clickjacking in attachment "Details" page.

(In reply to Reed Loden [:reed] from comment #4)
> (In reply to Mario Gomes from comment #0)
> > PoC: https://j6j9djxoqkc0.demo.bugzilla.org/
> 
> I don't seem to be getting any type of XSS warning on the above URL. Can you
> show me exactly how you were getting it?

Comment 6

•

12 years ago

  <body onload=""
        class="j6j9djxoqkc0-demo-bugzilla-org-aaaaa"'><img src=x onerror=confirm('XSS');>- yui-skin-sam">

Reporter

Comment 7

•

12 years ago

Yep, that's it.

(In reply to Reed Loden [:reed] from comment #6)
>   <body onload=""
>         class="j6j9djxoqkc0-demo-bugzilla-org-aaaaa"'><img src=x
> onerror=confirm('XSS');>- yui-skin-sam">

Comment 8

•

12 years ago

  <body onload="[% onload %]"
        class="[% urlbase.replace('^https?://','').replace('/$','').replace('[-~@:/.]+','-') %]
               [% FOREACH class = bodyclasses %]
                 [% ' ' %][% class FILTER css_class_quote %]
               [% END %] yui-skin-sam">

Doesn't look like we're filtering it.

Comment 9

•

12 years ago

(In reply to Reed Loden [:reed] from comment #8)
> urlbase.replace('^https?://','').replace('/$','').replace('[-~@:/.]+','-') %]
> 
> Doesn't look like we're filtering it.

t/008filter.t doesn't complain because the regexp contains the +, - and / characters, which are seen as mathematical operators:

    return 1 if $directive =~ /[+\-*\/]/;

This looks like the single place where urlbase is not filtered. As it's used to define a CSS class name, it should probably use FILTER css_class_quote (which is a ugly function, btw, see bug 474877).

Depends on: 224242

Reporter

Comment 10

•

12 years ago

"security issue" or not I can exploit it with only a click(using clickjacking on attachment details page). And nobody here takes this seriously. So, what do you think about a flaw that result in persistent xss with only a user click? Is it not a security issue? 

(In reply to Reed Loden [:reed] from comment #2)
> I don't consider this a valid security issue.

Reporter

Comment 11

•

12 years ago

Yeah that isn't a bug, that's a "feature" + a "clickjacking" = that becomes persistent XSS. That's why I love you buddies. You always try minimize the bug that is easily exploitable. So, is it really "INVALID"?

(In reply to Frédéric Buclin from comment #3)
> Moreover, we trust parameters set by administrators. We don't do a full
> validation on purpose. If you cannot trust your admins, then you are in
> trouble. This is not a bug.

Dave Miller [:justdave]

Comment 12

•

12 years ago

(In reply to Frédéric Buclin from comment #9)
> This looks like the single place where urlbase is not filtered. As it's used
> to define a CSS class name, it should probably use FILTER css_class_quote
> (which is a ugly function, btw, see bug 474877).

Which makes this sound like a real bug which should be fixed, to me.  Failure to filter.

That said, I'm 100% in agreement here that the security impact is non-existant here, because it requires admin privs to exploit, and would have to be intentionally exploited by an admin (we have CSRF protection in place on the params page). If you have an admin that's willing to do something like that, you have much bigger problems than XSS on your hands.

Which of course, does not mean we shouldn't fix it.  I think resolving it invalid just because it's not a security issue when it was initially filed as one is a little hasty.

Status: RESOLVED → REOPENED

Ever confirmed: true

Resolution: INVALID → ---

Comment 13

•

12 years ago

(In reply to Mario Gomes from comment #11)
> Yeah that isn't a bug, that's a "feature" + a "clickjacking" = that becomes
> persistent XSS. That's why I love you buddies. You always try minimize the
> bug that is easily exploitable. So, is it really "INVALID"?

Yes, your XSS issue is really invalid, because it's not exploitable, despite what you try to say. We *trust* parameters entered by admins, period! Also, no Javascript code can steal login cookies, because they are protected against such attacks, so your last comment in comment 0 is wrong too.

Summary: Persistent XSS in /editparams.cgi → The urlbase field in global/header.html.tmpl must be filtered

Dave Miller [:justdave]

Comment 14

•

12 years ago

There is also a degree of "minimizing attack surface" here.  There's always the possibility that an administrator's account could be compromised in some other way (someone steals their password via a keylogger or social-engineers someone to do a password reset, etc).  If you have illicit access to an admin account there are far worse things than this that you could do with that access.  But the more you can limit the damage to things that are easy to detect/easily noticed, the better.

Matt Selsky [:selsky]

Assignee

Comment 15

•

11 years ago

Attached patch v1 — Details — Splinter Review

Assignee: administration → selsky

Status: REOPENED → ASSIGNED

Attachment #697200 - Flags: review?(LpSolit)

Comment 16

•

11 years ago

Comment on attachment 697200 [details] [diff] [review]
v1

r=LpSolit

Attachment #697200 - Flags: review?(LpSolit) → review+

Updated

•

11 years ago

Severity: normal → minor

Flags: approval4.4+

Flags: approval4.2+

Flags: approval4.0+

Flags: approval+

Target Milestone: --- → Bugzilla 4.0