1254241 - Unchecked redirect in Bugzilla auth integration

Status: RESOLVED FIXED

(MozReview Graveyard :: Review Board: Extension, defect)

Description

[Wladimir Palant]

Unchecked redirects allow hiding phishing websites behind seemingly legitimate Mozilla links. They also allow circumventing referrer checks on websites that enforce the referrer to be a Mozilla website. Yet MozReview allows redirecting to any URL after Bugzilla authentication. Consider the following URL:

> https://bugzilla.mozilla.org/auth.cgi?callback=https://reviewboard.mozilla.org/mozreview/bmo_auth_callback/%3Fredirect%3Dhttp://google.com/&description=MozReview

While this seems to point to BMO, for any logged in Bugzilla user this will redirect to reviewboard.mozilla.org automatically which will in turn redirect to any website (google.com in this case).

Comment 1

[Mark Côté [:mcote]]

Attachment: bug-1254241.diff

This should do it, I believe, by enforcing that all directs are local paths (i.e. start with '/').

Comment 2

[:glob ✱]

Comment on attachment 8730394 [details] [diff] [review]
bug-1254241.diff

Review of attachment 8730394 [details] [diff] [review]:
-----------------------------------------------------------------

r=glob  lgtm

Comment 3

[Wladimir Palant]

What about protocol-relative URLs?

> https://bugzilla.mozilla.org/auth.cgi?callback=https://reviewboard.mozilla.org/mozreview/bmo_auth_callback/%3Fredirect%3D//google.com/&description=MozReview

Comment 4

[:glob ✱]

Comment on attachment 8730394 [details] [diff] [review]
bug-1254241.diff

Review of attachment 8730394 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Wladimir Palant from comment #3)
> What about protocol-relative URLs?
> 
> > ..%3Fredirect%3D//google.com/&description=MozReview

excellent point, thanks wladimir.

mcote - how parsing the url and rejecting those that don't match the expected hostname.

Comment 5

[Mark Côté [:mcote]]

Attachment: bug-1254241.diff

This does the same thing, verifying that the redirect is only a path, by using urlparse.

I didn't want to complicate things by checking for a correct server name; all generated redirects should be local.

Comment 6

[:glob ✱]

Comment on attachment 8731395 [details] [diff] [review]
bug-1254241.diff

Review of attachment 8731395 [details] [diff] [review]:
-----------------------------------------------------------------

r=glob

Comment 7

[Wladimir Palant]

I assume that redirecting to something like a data: URL isn't possible?

Comment 8

[Mark Côté [:mcote]]

Hm I could also check that the scheme is empty as well.

Comment 9

[Mark Côté [:mcote]]

Attachment: bug-1254241.diff (v3)

Once more, with feeling.

Comment 10

[:glob ✱]

Comment on attachment 8731489 [details] [diff] [review]
bug-1254241.diff (v3)

Review of attachment 8731489 [details] [diff] [review]:
-----------------------------------------------------------------

r=glob

Comment 11

[Mark Côté [:mcote]]

https://hg.mozilla.org/hgcustom/version-control-tools/rev/415a3d6628d3

Comment 12

[Mark Côté [:mcote]]

This has been deployed.  Btw the link in comment 0 should be https://bugzilla.mozilla.org/auth.cgi?callback=https://reviewboard.mozilla.org/mozreview/bmo_auth_callback/%3Fredirect%3Dhttp://google.com/&description=mozreview to avoid generating a new API key (the description at the end should be all lower case).  I've confirmed that it did redirect to google before and no longer does.