532518 - Credentials are not checked correctly when viewing one attachment from another bug's alternate host

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, defect)

Description

[Michael Thomas (Mockodin)]

Attachment: Access Denied screen shot

If a user attempts to access an attachment for which they does not have permission to view the resulting error screen will display the Log In and Forgotten password links in the header and footer

This can be see on the BMO site (version 3.4.4+)

To replicate find an attachment you are denied access to and attempt to display it.

Comment 1

[Frédéric Buclin]

Unless I miss something, I'm unable to reproduce this issue with 3.5.2 and 3.4.4. Do you have a URL?

Comment 2

[Michael Thomas (Mockodin)]

I attempted to view an attachment from irc, since I had a attachment already up I had just replaced the attach_id. What I failed to notice was the url itself had redirected originally to include the bug id at the beginning the url. Like this:

https://bug<bugid>.bugzilla.mozilla.org/attachment.cgi?id=<attachmentid>

I did not replace the bugid. So if the bug_id is left incorrect, but the attach id is change it will try and open the attach but throw the access denied error as it should and references the bug actually associated with the attachment. Which is where the screen shot came from on BMO.

So in summary, this shouldn't occur under normal conditions but if someone manually enters the attachment id and has no permissions to view it they will get the incorrectly displayed page, though still correctly denying access.

If the bug_id is also changed the page will display normally.

Comment 3

[Frédéric Buclin]

Ah ok, I didn't try this testcase. Reopening for further testing when I have some time.

Comment 4

[Frédéric Buclin]

I can reproduce the issue. I'm surprised attachment.cgi didn't redirect to correct_urlbase first before throwing the error.

Comment 5

[Frédéric Buclin]

In bug 38862, I assumed that if you were not on the bug's alternate host, you were on the main host, from where we could safely check your credentials. This is a good assumption most of the time, except when each bug has its own host for attachments and you hack the URL to view one attachment from another bug's alternate host, as described in comment 2. In that case, we need to redirect you to the main host to check your credentials first.

I don't think that's exploitable as the login form points to the main host, but I want to handle this edge-case correctly anyway for 3.4.x and tip.

Comment 6

[Frédéric Buclin]

Attachment: patch, v1

As said in my previous comment, we need to redirect the user to the main host to check his credentials when the alternate host mismatches the expected host.

Comment 7

[Max Kanat-Alexander]

Comment on attachment 419417 [details] [diff] [review]
patch, v1

This logic is getting really confusing. Could we possibly reverse it all so that the viewing only happens in the $bug_id case? (That is, reverse all the if, elsif, else?)

In any case, this looks OK.

Comment 8

[Frédéric Buclin]

(In reply to comment #7)
> that the viewing only happens in the $bug_id case? (That is, reverse all the
> if, elsif, else?)

An alternative I had in mind was:

if (!$cgi->url_is_attachment_base($bug_id)) {
    if ($cgi->url_is_attachment_base) {
        $cgi->redirect_to_urlbase;
    }
    ....
}
else {
    ....
}

Does it look more understandable to you? Personally, I found it more confusing than my current patch.

Comment 9

[Max Kanat-Alexander]

Yeah, I find that more confusing, too.

I was thinking of making the very top condition "if ($cgi->url_is_attachment_base($bug_id)" and putting what's currently in the "else" into there, and then reversing the logic in general.

Comment 10

[Frédéric Buclin]

Ah, I see. Hum, I'm not sure the benefit is real, though. :)

Comment 11

[Max Kanat-Alexander]

Yeah, I think it would help with readability, though. Usually negative conditions are hard to read and positive conditions are easy to read.

Comment 12

[Frédéric Buclin]

Attachment: alternate patch (reversed logic), v1.1

Here is the patch with the logic reversed. That's the one I'm going to commit.

Comment 13

[Frédéric Buclin]

The patch doesn't apply cleanly on the 3.4 branch as bug 523495 didn't land on this branch, and so $cgi->url_is_attachment_base is not available there. As this bug is not too severe, I'm only committing this patch for 3.6.

Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.167; previous revision: 1.166
done

Comment 14

[Max Kanat-Alexander]

Comment on attachment 419566 [details] [diff] [review]
alternate patch (reversed logic), v1.1

That looks way better, thank you. :-)