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

RESOLVED FIXED in Bugzilla 3.6

Status

()

RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: mockodin, Assigned: LpSolit)

Tracking

3.4.4
Bugzilla 3.6
Bug Flags:
approval +
blocking3.6 +

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

9 years ago
Created attachment 415727 [details]
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.

Updated

9 years ago
Keywords: qawanted
Whiteboard: DUPME?
(Assignee)

Comment 1

9 years ago
Unless I miss something, I'm unable to reproduce this issue with 3.5.2 and 3.4.4. Do you have a URL?
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Keywords: qawanted
Resolution: --- → WORKSFORME
(Reporter)

Comment 2

9 years ago
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.
(Assignee)

Comment 3

9 years ago
Ah ok, I didn't try this testcase. Reopening for further testing when I have some time.
Status: RESOLVED → REOPENED
Keywords: qawanted
Resolution: WORKSFORME → ---
(Assignee)

Comment 4

9 years ago
I can reproduce the issue. I'm surprised attachment.cgi didn't redirect to correct_urlbase first before throwing the error.
Assignee: user-accounts → attach-and-request
Status: REOPENED → NEW
Component: User Accounts → Attachments & Requests
Keywords: qawanted
Whiteboard: DUPME?
(Assignee)

Comment 5

9 years ago
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.
Assignee: attach-and-request → LpSolit
Severity: trivial → normal
Status: NEW → ASSIGNED
Depends on: 38862
Flags: blocking3.6+
Summary: Attachment Access Denied page for displays user as being logged out → Credentials are not checked correctly when viewing one attachment from another bug's alternate host
Target Milestone: --- → Bugzilla 3.4
(Assignee)

Comment 6

9 years ago
Created attachment 419417 [details] [diff] [review]
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.
Attachment #415727 - Attachment is obsolete: true
Attachment #419417 - Flags: review?(mkanat)

Comment 7

9 years ago
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.
Attachment #419417 - Flags: review?(mkanat) → review+
(Assignee)

Comment 8

9 years ago
(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.
Flags: approval3.4+
Flags: approval+

Comment 9

9 years ago
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.
(Assignee)

Comment 10

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

Comment 11

9 years ago
Yeah, I think it would help with readability, though. Usually negative conditions are hard to read and positive conditions are easy to read.
(Assignee)

Comment 12

9 years ago
Created attachment 419566 [details] [diff] [review]
alternate patch (reversed logic), v1.1

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

Comment 13

9 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago9 years ago
Flags: approval3.4+
Resolution: --- → FIXED
Target Milestone: Bugzilla 3.4 → Bugzilla 3.6

Comment 14

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

That looks way better, thank you. :-)
Attachment #419566 - Flags: review+
You need to log in before you can comment on or make changes to this bug.