letting attachments load from the bugzilla.mozilla.org hostname is a security problem (cookie-stealing, mostly). suggested solution: 1. make showattachment.cgi the hostname, and if it's bugzilla.mozilla.org, redirect to [bugzilla.mozilla.org's ip address]/showattachment.cgi?id=num. 2. make show_bug.cgi show the correct new url in order to avoid breaking existing two-file attachments where the first one is an image, perhaps: 3. replace "bugzilla.mozilla.org/showatt" with "[ip]/showatt" in all attachments.

i wonder if using content-disposition (mentioned in bug 20383) would fix this.

by the way, hotmail uses a similar trick to prevent *.html attachments from doing malicious scripty things. http://www.peacefire.org/security/hmattach/

moving to real milestones...

When implementing this solution make sure the redirect from [domain]/showattachment.cgi to [ip]/showattachment.cgi displays a message about the correct URL and then waits for 5-10 seconds before automatically forwarding users. Otherwise it's possible for a script like the one described in the peacefire.org article to cause the user's browser to enter an infinite redirect loop.

The attached patch also validates the value of the "attach_id" URL parameter but does NOT include the necessary "DisplayError" function which exists in patches for bug 38854 and bug 38855, only because I forgot. I can create a new patch if it matters.

It looks like what this patch does is take the Param("urlbase") and switch everything between http:// and the first / for the machines IP address. I don't think this will work for b.m.o as bugzilla is a virtual host on mothra. If you got to it's IP address, you don't get bugzilla, you get a list of links to mozilla.org resources.

that could be solved with a rewrite rule in the httpd.conf file under the mothra.mozilla.org section... RewriteRule ^/showattachment.cgi(.*)$ /path/to/bugzilla/showattachment.cgi$1

I'm not going to approve this patch because, as is, it will break bugzilla installs that are running as virtual hosts. Yes, this can be fixed by adding a rewrite rule to your apache config (as noted in my previous comment), but since not everyone has the capability to screw with their apache config, it needs to be an option. Default to ON for the obvious security reasons, and the description of the preference should indicate that it's a security risk to turn it off, but may be necessary if they are running bugzilla as a virtual host and can't add rewrite rules to their apache config.

My thoughts on this would be to create a param right below urlbase called attachmentbase (for lack of a better name :)... this can either default to %usrbase% or some arbitrary ip address (such as the IP to cvs-mirror) [my preferance would be %urlbase%]. The description for this should include a link to "docs/html/<file>.html" where <file> is documentation as to why this should be used. CC'ing barnboy 'cause I said the magic word :)

The latest patch adds an 'ipbase' parameter which contains an IP-based alternative to 'urlbase'. If the parameter is not blank it is used by globals.pl when doing linkification of "attachment xxx" in bug comments, by createattachment.cgi when linking back to a newly created attachment, and by bug_form.pl when creating the list of attachments. If 'ipbase' is set, then showattachment.cgi checks that it was requested using an IP-based URL and throws an error if it wasn't. 'ipbase' is blank by default since setting it to 'urlbase' might give a false sense of security ("It's set, the security it provides is enabled.") and because 'urlbase' initially contains only a sample value (http://cvs-mirror.mozilla.org/webtools/bugzilla/). If we want to provide a similar sample value for this parameter I suggest using the IP equivalent of the 'urlbase' sample value (http://198.186.203.47/webtools/bugzilla/). I called it 'ipbase' instead of 'attachmentbase' in case it ever gets used in a different context. Jake's patch for data validation and security in bug 70189 was applied to showattachment.cgi before I added these patches.

My obligitory two comments :) * The compare against urlbase should be done before the SQL statement (no sence running the query if the data's not gonna be used). Also, putting that check before ConnectToDatabase() would make this independant of bug 70189. * check_urlbase in defparams.pl allows ipbase to be a host name or an ip address, but showattachments.cgi won't work unless it's an IP address.

In the latest version of this patch, showattachment.cgi validates the HTTP_HOST before doing anything else, and check_ipbase makes sure value of the 'ipbase' parameter points to an IP address. I also applied my patches to showattachment.cgi against the latest version of that file after the fixes for bug 70189 were checked in. There is one more question to resolve here: Is it more important for Bugzilla, when first installed to be secure or to work? If 'ipbase' is blank by default, then the viewing of attachments will work, but the installation won't be as secure. If that parameter is set to a default value, then the installation will be more secure but viewing of attachments won't work.

I say make it work. But put a short description of the potential problem and maybe a link to the CERT advisory in the param description in defparams, and make sure it gets relnoted.

I didn't even think of this possibility before, but in the process of testing it hit me like a brick. When you log into bugzilla (using b.m.o as an example) it sends you a cookie w/the hostname 'bugzilla.mozilla.org'. On subseqent visits to bugzilla, the browser send this cookie back to bugzilla and bugzilla validates that you are who you say you are. No news here. In bug 70189, code was added to make sure that a person is authorized to view the bug in question. It does this by running quietly_check_login() which checks for the existance of the bugzilla cookie so a user can be validated. Put 2 and 2 together, and suddenly attachments don't work on secure bugs. I don't think the solution is to confirm_login(), because the whole idea of putting attachments on the IP address is so the cookie can't be stolen. So, that leaves the question... "Now what?"

It is ugly but demonstrates the concept. One question is the syntax for namespace prefixes.... am I correct in thinking they have to be alphanumeric + "_"?

The answer to that question is unfortunately very complicated... http://www.rpbourret.com/xml/NamespacesFAQ.htm#q11_2

elsif ( $thedata =~ /<[\w.-]*:?script/ ) { Is not case insensitive. Also, I think the DisplayError red box approach is quite ugly in this instance. Maybe it would look better if the options were bulleted in some way...

Yes, the red-box approach is quite ugly, although unfortunately it is the way we have chosen to display errors. Since in this case it is a warning we are trying to display, not an error, probably there is a better style available. I like the bulleted list approach and will play with that, and I'll make the regular expression case-insensitive.

<frameset><frame src=showattachment.cgi?attach_id=666666&mode=disabled></frameset> you can't use that form at all. anything that can be in a url can be spoofed. So i guess you'll have to use form submissions, since the only spoofing would first require a script.

hmmm.... so it should also warn if the document contains framesets?

Mozilla developers need to be able to view any attachment that might affect the browser. They shouldn't have to (and at least some probably won't) read through each test case looking for malicious code before loading it in a browser. If we can't figure out a real solution before 2.14, I'd rather leave things as is for now than add an ineffective and annoying warning screen.

So the javascript swipes their login cookie. Who cares? If it sends it off to someone they can't do anything with it anyway unless they happen to be at the same IP address (or spoofing it, but if they went to that much trouble you have bigger problems). It's one extra click to get the original attachment as-is. Is that *that* much of a bother if the developers just automatically go for the "view unaltered" link instead of looking over the sanitized version first? It's their own risk, and if they're that short of time that they just automatically do it, then they're no worse off than they already are with the system the way it is today with no filters in place.

Myk, this looks familiar... do you have this running on landfill under bzpacman with the attachment tracking stuff?

Jesse is right about one thing: warning messages that pop up even when problems don't exist tend to train people to ignore them, and then bugzilla is no more secure but certainly more annoying to use. Interestingly, the URL describing this exploit at Hotmail points out that stealing someone's cookies enabled the burglar to break into Hotmail. That is not the case in our situation, as several people have already pointed out, because in Bugzilla each cookie is tied to the particular IP address from which it was originally retrieved. Thus, while this exploit may not sound nice, it has no practical application in Bugzilla, and thus this bug is at least severity "minor" and shouldn't hold up the 2.14 milestone.

I agree. 2.14 has been sitting way too long, and this still has too much argument about how to actually implement it yet. Maybe the warning thing would be good, but make it a param so a site doesn't have to turn it on if they don't want it. Let's leave this here for more comments yet, if anyone can really convince me maybe we'll move it up again. In the mean time we'll re-evaluate for 2.16.

breaking dependency... based on the comments in this bug, I'm having trouble finding a way secure information on bugzilla can be compromised with this exploit. If someone disagrees, please explain it to me :-) Not to mention I basically don't like having bugs depend on bugs which are targetted at a later milestone. :-)

>Thus, while this exploit may not sound nice, it has no practical application in Bugzilla Actually, it does :) The attacker doesn't need to even see the cookie; he can use javascript and frames to direct the victim's browser to do things in bugzilla (under the victim's account). For example, the attacker might create a page that: 1. Creates a frame. 2. Loads the list of groupset=1024 bugs in that frame. The browser will send the user's bugzilla cookie to the frame, and if the user has access to Netscape-confidential bugs, the listing will succeed. 3. Hits the "long format" button at the bottom of the page in the frame. 4. Sends frame.contentDocument.body.innerHTML off to the attacker. or 3. Hits the "edit several bugs at once" button at the bottom of the page in the frame. 4. Resets the groupset on each bug to zero. This is a tough bug to solve, and I'm not against pushing it out to 2.16, but I don't think it's a minor issue.

I was about to rip you to shreds for screwing with our heads again on this. But then I figured out a way we can actually pull this off and still stay secure. I ran it past bbaetz in IRC tonight and he likes it. The primary concern with putting the attachments on a different hostname is that the cookie is required in order to view a secure attachment, and since the cookie won't be sent to the other hostname.... Well, the way we can fix this is by issuing a temporary session token that gets passed as part of the URL when redirecting to the alternate hostname. We already have token generating code and a table to store them in thanks to the recent changes in password resets. Here's how this would work: 1) All the links to attachments stay how they are. The only changes needed would be in showattachment.cgi, and defparams.pl (and probably Token.pm, to handle the new classification of tokens) 2) User clicks a link to an attachment, either in a bug or an email 3) User arrives at showattachment.cgi on the primary hostname. 4) Login checks are performed, and the user is forced to log in if the bug is secure and they haven't logged in yet. 5) A token is generated 6) A 302 redirect is issued (via the Location header returned to Apache) redirecting to the alternate hostname with the token passed as a parameter to the URL 7) User arrives at showattachment.cgi on the alternate hostname. 8) The token is checked to ensure it was issued within the last 5 minutes. This time can also be used for cleanup, and any tokens older than 5 minutes can be deleted. 9) if the token is valid, the attachment is displayed. If not, or if it's missing, an error message would be shown, with a link to showattachment.cgi on the primary hostname to get a valid token issued. My original plan here was to redirect back to the primary hostname, but that would leave it open to the infinite loop thing described several comments back if we did that. I'm still debating whether to move this back to 2.14 or not. While I'm reasonably confident that I can come up with a patch to implement this tonight yet, I'm not sure if 3 days is adaquate testing time for it.

My biggest concern with this is does lynx support 302 redirects? (for lynx -source {attachment_url} | patch ). Also, do we still want to impliment the "This attachment contains potentially harmful data" portion?

I'm pretty sure browsers are required to support 302 redirects by the RFCs and have been since close to the beginning, so I'm sure it does. We can probably put a link in the entity body just in case. Lynx supports tables and frames, and those haven't been around as long. :-) It's the META refresh stuff that half of them don't support.

-> New Bugzilla Product

We are currently trying to wrap up Bugzilla 2.16. We are now close enough to release time that anything that wasn't already ranked at P1 isn't going to make the cut. Thus this is being retargetted at 2.18. If you strongly disagree with this retargetting, please comment, however, be aware that we only have about 2 weeks left to review and test anything at this point, and we intend to devote this time to the remaining bugs that were designated as release blockers.

So, what have we decided to do with this? The token idea sounds nice (assuming that we only do it for non-groupset 0 bugs), but I don't know if its too complicated. We'd have to param the alternate name, too.

Comment on attachment 37692 [details] [diff] [review] version of patch that links to CERT advisory per comments 36/37 there's a better way to do this

Comment on attachment 39758 [details] [diff] [review] no red boxes! bullets! case-insensitive! no warnings! per comments 36/37, there's a better way to do this.

Comment 37 sounds great, apart from: > 8) The token is checked to ensure it was issued within the last 5 minutes.This > time can also be used for cleanup, and any tokens older than 5 minutes can be > deleted. The problem is that this still leaves a window of opportunity for a leak. Tokens should be single-use only. If the token is not valid or missing (e.g. if the person simply does Reload), we should redirect back to showattachment.cgi, reauthenticate, issue a new token, and come back to the secondary hostname. I can't see how this would cause an infinite loop (and I can't see any reference to infinite loops in the Peacefire article.) If the person doesn't have the creds, they stop at attachment.cgi with a permissions error. If they do, they stop at the secondary hostname with a copy of the attachment :-) Of course, all of this would only apply for attachments with group restrictions, and if the feature was turned on by defining a secondary hostname in the params. Gerv

Regarding comment 45: I haven't read the entire proposal, but my concern w/a one-time-use token is that I often grab attachments via the command line by copying the URL from my browser and pasting it into a lynx -source "<url>" | patch type command. If this proposal only requires a token for secure bugs, then this is a moot point as lynx seems to be unwilling to store a login cookie for me :)

AIUI, the auth changes bbaetz is making mean that you could put your login credentials on the command-line; and if wget respects the redirect, then you are sorted. You'd probably want a script which appended your login details automatically to save you typing, but this usage mode would still work. Gerv

Right; the Bugzilla_login form val is now honured from q_c_l and c_l. (well, there replacements, which are the same sub, which is why it woks that way) Or they will if Gerv reviews that :)

This bug is major because it leaks security-confidential bugs (comment 36).

Marking as security-sensitive, as this is a security issue, and not having it marked as such is lulling Dave into a false sense of security, ho ho, judging by his recent post to reviewers@bugzilla.org. > 6) A 302 redirect is issued (via the Location header returned to Apache) > redirecting to the alternate hostname with the token passed as a parameter to > the URL Will this run into the problem we are seeing in bug 214466, or is that just because we are setting cookies at the same time over there? Gerv

heh. yeah, that's just because we're doing cookies at the same time, over there. We have good ideas how to implement this now, there's no reason we shouldn't. Question now is... we already have an advisory ready to go for 2.17.6 because of the oops with bug 195530. Is this something we can have ready to join it? Bug 195530 is serious enough that I really don't want to wait. Maybe we can do a 2.17.7 in a couple weeks with this one...

Don't wait. This will need baking time. In fact, although I've now marked it security-sensitive, given that we and every other bug system has lived with this for years, and that this bug has been public for months, I think it's reasonable to develop a fix, check it in and bang on it, and then just mention it in the advisory for our next scheduled release. Gerv

sounds reasonable.

OK, here we go. Because of the architecture of attachment.cgi, it has no way to tell whether an attachment is secure or not, only that the current user can see it or not. Which means it has no way to tell the difference between a public attachment and a secure one that the current user happens to be able to see. So I did the redirect thing *always* instead of just on secure bugs. This actually works quite nicely. :) It's live on http://landfill.bugzilla.org/bzdave-216/ at the moment if you want to try it out.

Comment on attachment 135550 [details] [diff] [review] token+redirect v1 (2.16 branch) Why are we only matching on the host? Don't we need to match on the full host/path bit? I'll have to log in again, won't I? I can't test, because I'm not redirected at all on your test instance

Comment on attachment 135550 [details] [diff] [review] token+redirect v1 (2.16 branch) Dave asked me (on IRC) about IIS functionality on this issue. I don't believe there's an IIS problem with this (after all, bug 214466 is "only" about cookies being set while redirecting), but I'm not going to test the 2.16 patch because it's so hard to get 2.16 to even work on Win32 :-) Since 2.16 is already broken (Win32-wise) in so many other places, I've stopped caring. I'll be glad to test and review the trunk patch, though. >+# If attachmentbase is different from urlbase, then we don't want anything >+# other than attachment.cgi being accessed via attachmentbase in order to Nit: ----- "to be" might be more understandable? >+# guarantee cookies don't get set on the attachment host. If they're >+# accessing any other URL, send them back to urlbase via redirect. Nit: Who they? Cookies? Also, you're not sending them back to urlbase - you're sending them to the same page _through_ urlbase (or something like that). >+if (Param("attachmentbase") && (Param("urlbase") ne Param("attachmentbase"))) { I'd find this a lot more readable if you took the Params into variables right from the beginning instead of repeating |Param("blabla")| all the time (until you end up assigning them to local variables anyway a bit later in the code :-)). >+ if ((Param("attachmentbase") =~ /\Q$::ENV{"HTTP_HOST"}\E/i) HTTP_HOST is somebody's extension; CGI standard at <http://hoohoo.ncsa.uiuc.edu/cgi/env.html> doesn't know it. SERVER_NAME is recommended. IIS supports both, though. >+ my $url = $proto . "://" . $::ENV{HTTP_HOST} . $::ENV{REQUEST_URI}; REQUEST_URI is not standard either (and not supported by IIS). I recommend SCRIPT_NAME. Ooph, I don't have the time to go through rest of the patch... But at least you should replace those env variable names with the standard ones.

bbaetz wrote in comment 55: >Why are we only matching on the host? Don't we need to match on the full >host/path bit? The hostname is all that matters. If you use the param at all, the hostname on the alternate url is required to be different. If you don't make it different, having any other part of the URL different will be pointless, because you'll still get the cookies. >I'll have to log in again, won't I? I can't test, because I'm not >redirected at all on your test instance Shouldn't have to log in. If the bug is public, it'll redirect you with a dummy token if you're not logged in (which will just view the attachment if it's public, otherwise will get you a "you have to log in now" screen, which you would have gotten before the token was issued anyway when you hit the main url) The redirect on landfill is subtle because I'm using landfill.mozilla.org as the alternate hostname. Note the s/bugzilla/mozilla/. Jouni wrote in comment 56: >>+ if ((Param("attachmentbase") =~ /\Q$::ENV{"HTTP_HOST"}\E/i) > >HTTP_HOST is somebody's extension; CGI standard at ><http://hoohoo.ncsa.uiuc.edu/cgi/env.html> doesn't know it. SERVER_NAME is >recommended. IIS supports both, though. HTTP_HOST is part of the standard. The headers passed by the client are passed to the CGI with HTTP_ in front of them. In HTTP/1.1, the client is required to send a Host: header with the name of the host they're trying to access. We can't use SERVER_NAME because it'll always be the name, even if the client used the IP address to connect, or a name that was defined as a ServerAlias. If we use SERVER_NAME it would require two different virtual hosts to be defined rather than being able to use two different names on the same virtual host. Yes, this does mean this won't work with an HTTP/1.0 browser, but then 90% of the Bugzillas out there that are on their own domain names probably won't, anyway. bugzilla.mozilla.org is one of those. If you go to bugzilla.mozilla.org by IP address you get the webtools.mozilla.org homepage, not Bugzilla. >>+ my $url = $proto . "://" . $::ENV{HTTP_HOST} . $::ENV{REQUEST_URI}; > >REQUEST_URI is not standard either (and not supported by IIS). I recommend >SCRIPT_NAME. OK, I'll give on this one. REQUEST_URI is the same as SCRIPT_NAME + '?' + QUERY_STRING, and those two should always be available.

I wrote in comment 57: >bbaetz wrote in comment 55: > >>Why are we only matching on the host? Don't we need to match on the >>full host/path bit? > >The hostname is all that matters. If you use the param at all, the >hostname on the alternate url is required to be different. If you don't >make it different, having any other part of the URL different will be >pointless, because you'll still get the cookies. Actually...... now that you mention it... I suppose thanks to the magic of cookiepath (which I don't think we had yet when this bug was originally filed) that might not be true. We might be able to get away with having a different virtual path to attachment.cgi on the same hostname (via a symlink, an Alias directive, or a non-redirecting RewriteRule), and as long as cookiepath excludes the directory the alternate URL points at we'd be safe....

re comment 58: Discussion with Jesse in IRC reminds me that we have the "same origin" problem with javascript.... so we do indeed need to assure that the attachment base is actually on a different hostname and not just a different directory outside of cookiepath.

OK, to satisfy the folks who for some weird reason are actually using HTTP/1.0, we can fall back on SERVER_NAME if HTTP_HOST isn't present. However, it'll still cause problems if both names actually share the same IP because we'll have no way to tell which way we were reached by the browser. The only way HTTP/1.0 works in this situation is if the two base urls actually have two different IP addresses.

That sounds like a good solution. I find supporting HTTP/1.0 useful, even if there were some special issues to be documented. And yes, for example our Bugzilla can be (and in some extreme cases, is) used by HTTP/1.0. Such a "weird reason" may be, for example, a really mundane client library used by some application to retrieve http pages, or a proxy incapable of HTTP/1.1.

When implementing the redirect stuff, we should be careful to check the edit attachment screen for possible browser security dialogs (the focus being on IE with Bugzilla running in the standard Internet security zone - I believe we have Mozilla testing naturally covered). The redirection-to-another-site-within-iframe stuff can be potentially problematic. This is probably a non-issue since users can disable the dialogs anyway, but maybe worth testing anyway.

This doesn't look like it'll happen in the next 4 days :(

On the 2.17 branch, CGI.pm can tell us who we are and how we were accessed. We can use the CGI routines for said things and be much cleaner because CGI should always Do The Right Thing. We'll have to construct our own like this on 2.16 though because we aren't using CGI yet.

Addresses the HTTP/1.0 issue. I have not done any testing with Internet Explorer, as I don't have access to an MSIE to test it on. It would be good if someone could test it there.

The revised patch is once again live on http://landfill.bugzilla.org/bzdave-216/ for testing.

Comment on attachment 139302 [details] [diff] [review] token+redirect v2 (2.16 branch) >+sub IssueSessionToken { >+ # Generates a random token, adds it to the tokens table, and returns >+ # the token to the caller. >+ >+ my $loginname = $::COOKIE{'Bugzilla_login'}; This sub breaks totally if you're not logged in (Software error). Also, I got "URL redirection limit exceeded" on Firebird 0.7 when trying to click on the edit link - the view link did work. IE didn't issue this warning, but it just hang in an infinite redirection loop.

eek. It worked before I tried to add the HTTP/1.0 support. Wonder what I broke.

Is it not possible for the bmo pages to set the JS-security context hostname to a value which is more specific than bugzilla.mozilla.org (<random_value>.bugzilla.mozilla.org?) such that attachments can't then access bmo? (Possibly s/more/less/, depending on how you use the terminology)

From: http://msdn.microsoft.com/workshop/author/dhtml/reference/properties/security.asp <IFRAME SECURITY="restricted" src="http://www.microsoft.com"></IFRAME> Looks like Microsoft is thinking about this as well.

This bug is public because jon@ora.ro found it independently and mentioned it in bug 178993 comment 21. I can't uncheck "Webtools Security-Sensitive Bug", though.

*** Bug 251185 has been marked as a duplicate of this bug. ***

Anyone want to take a stab at resurrecting this patch? I'm not going to have time to work on it.

This vulnerability can also be used to steal password. See bug 251185. And hi, Jesse. (maybe I shouldn't be surprised you reportd this bug. But I am ;-) )

As far as I understand it, v.1 of this patch worked, but when Dave added HTTP 1.0 support, things broke. Would it be reasonable to say we should fix this for HTTP 1.1 now, and if people want HTTP 1.0 support, they can produce a separate patch at a later date? That way, we could revert to the working v.1 patch, fix it up and get it in for 2.18 final. That might be less work. Dave? Gerv

Hmm... it will be interesting mixing this with single-signon via reverse proxy. Any ideas?

Given that I have no idea what you are talking about, no. :-) Gerv

The way many corporate datacenters work is as follows.... There is a single machine operating as a reverse proxy. It receives requests for http://mycompany.com/bugzilla/whatever.cgi and proxys/rewrites them as http://my_dmz_bugzilla_host.datacenter_non_addressable.mycompany.com/bugzilla2_18/whatever.cgi So, even if the Bugzilla hostname is distinct, the datacenter proxy would need 2 names as well. In addition, single-signon is often done by having each webserver use a module that coordinates authentication with the reverse proxy. Until you authenticate, you cannot even access a URL in a protected space behind the proxy. When you do authenticate, the apache module takes the credentials and passes them to cgi scripts as environment variables. Probably, the only way to make this approach work is to permit one host address to proxy the entire bugzilla directory while requiring authentication and provide a second address and urlbase that get mapped to the same bugzilla server without requiring authentication. To do that, it means that we do not want to have an "attachment hostname" parameter. Instead, we should have an "attachment urlbase" so that a reverse proxy can steer the appropriate url appropriately.

If you're getting the username from Apache, you're not going to have this issue. The user has already authenticated before they hit Bugzilla, so neither Bugzilla nor the enclosing webpage is going to have any authentication information other than the user's username, which is assumed to already be authenticated because of the agreement with Apache to provide it to you. This means the authentication information will already be available to you on the alternate host, and thus no need to redirect back to the main host to get a token.

That's exactly the problem. When we switch hostnames to keep from being in the same-host context, the external signon mechanism dont get their cookies so you are logged out. That means that the attachment urlbase needs to differ from the normal urlbase by more than just the hostname so that the reverse proxy can tell that it should let the requests through without trying to figure out who the user is. So, query.cgi would be at... www.mycompany.com/bugzilla/secured/query.cgi and the cgi would know who the user is while we would use.... alternate.mycompany.com/bugzilla/unsecured/attachment.cgi&token=whatever and the cgi would not be told who the user is in this example, urlbase is "www.mycompany.com/bugzilla/secured/" and attachment base is "alternate.mycompany.com/bugzilla/unsecured/" just changing the hostname is not good enough.

> To do that, it means that we do not want to have an "attachment hostname" > parameter. Instead, we should have an "attachment urlbase" Er... both v.1 and v.2 of Dave's patch have an attachment URLbase. Gerv

Are we still trying to do this for 2.18? Given that 2.20 freezes in a week, surely it makes sense to push it out? We've lived with it for long enough... Either that, or Dave needs to revive his patch :-) Gerv

(In reply to comment #82) > Are we still trying to do this for 2.18? Given that 2.20 freezes in a week, > surely it makes sense to push it out? We've lived with it for long enough... Yeah, I was just thinking that yesterday when I was looking at this. > Either that, or Dave needs to revive his patch :-) I don't think I'll have time to touch it any time soon.

CCing some people who have done a lot of code work lately that *might* be interested in hacking on this. :)

*** Bug 256348 has been marked as a duplicate of this bug. ***

We have to do a 2.16 security release ASAP because of bug 272620. Therefore, this bug isn't going to make the 2.16.8 train. Gerv

*** Bug 280469 has been marked as a duplicate of this bug. ***

updated for HEAD and new cgi/db routines

Comment on attachment 182755 [details] [diff] [review] token+redirect v3 The patch does not work when I'm logged in (no problem if I'm logged out). I get a "too many redirections" error message using Firefox 1.0.3.

> The patch does not work when I'm logged in (no problem if I'm logged out). > I get a "too many redirections" error message using Firefox 1.0.3. weird, works for me logged in, ff 1.0.3. i'll investigate.

ok, yeah, it's broken. i don't even know how it was working before.

Comment on attachment 182786 [details] [diff] [review] token+redirect v4 I did not test this patch and did not do a full review, but I have some comments anyway. :) >--- attachment.cgi 28 Apr 2005 02:14:25 -0000 1.83 >+++ attachment.cgi 6 May 2005 15:15:18 -0000 >+sub ValidateSessionToken { Shouldn't this function go into Token.pm? >+ my $token = $cgi->param('t'); Are you sure 't' is always defined? What about: my $token = $cgi->param('t') || ''; trick_taint($token); Writing trick_taint() here will avoid duplication later. >+ # Get the user's ID from the tokens table. >+ my $userid; >+ if ($token ne '') { >+ my $sth = $dbh->prepare("SELECT userid FROM tokens WHERE token = ?"); >+ trick_taint($token); >+ $sth->execute($token); >+ ($userid) = $sth->fetchrow_array; >+ } Better: $userid = selectrow_array("SELECT userid FROM tokens WHERE token = ?", undef, $token); assuming trick_taint($token) has been called earlier, as suggested in my previous comment. >+ if (!defined $userid) { >+ # no valid token >+ print $cgi->redirect('-location' => Param('urlbase') . $path); >+ exit; >+ } >+ >+ # Change current user >+ Bugzilla->switchuser($userid); Couldn't we use ->logout() followed by ->login() ? >+ # delete the token from the tokens table. >+ trick_taint($token); >+ $dbh->bz_lock_tables('tokens WRITE'); >+ $dbh->prepare("DELETE FROM tokens WHERE token = ?")->execute($token); >+ $dbh->bz_unlock_tables(); $dbh->do("DELETE FROM tokens WHERE token = ?", undef, $token); >--- defparams.pl 7 Apr 2005 08:08:36 -0000 1.157 >+++ defparams.pl 6 May 2005 14:43:08 -0000 >+ name => 'attachmentbase', >+ desc => "An alternate URL that is the common initial leading part of " . >+ "all attachment URLs, not counting the 'attachment.cgi' on the " . >+ "end. This should have a <b>different</b> domain name than " . >+ "<tt>urlbase</tt> in order to prevent malicious attachments from " . >+ "being able to access your cookies. If you can't set up an " . >+ "alternate hostname, or aren't worried about the security " . >+ "implications, leave this field empty.", Maybe should you specify that the URL has to start with http and end with a slash. >--- Bugzilla/CGI.pm 16 Jan 2005 20:43:22 -0000 1.15 >+++ Bugzilla/CGI.pm 6 May 2005 14:43:08 -0000 >+sub using_attachment_host { >+=item C<using_attachment_base()> using_attachment_host or _base ?? >--- Bugzilla/Token.pm 3 Mar 2005 07:19:09 -0000 1.29 >+++ Bugzilla/Token.pm 6 May 2005 14:58:27 -0000 >+ $dbh->bz_lock_tables('tokens WRITE'); >+ my $sth = $dbh->prepare("INSERT INTO tokens (userid, issuedate, token, tokentype) VALUES (?, ?, ?, ?)"); >+ $sth->execute($userid, $issuedate, $token, 'session'); >+ $dbh->bz_unlock_tables(); $dbh->do("INSERT INTO ...", undef, ($userid, $issuedate, $token, 'session'));

thanks for the review. most things are good points that i will fix. > trick_taint($token); > Writing trick_taint() here will avoid duplication later. while other reviewers have said that the trick_taint always belongs as close to the sql call as possible :| > >+ # Change current user > >+ Bugzilla->switchuser($userid); > > Couldn't we use ->logout() followed by ->login() ? yeah, that's probably better. > Maybe should you specify that the URL has to start with http and end with a > slash. nah, no such description exists for urlbase or sslbase. > >+sub using_attachment_host { > >+=item C<using_attachment_base()> > > using_attachment_host or _base ?? opps. _base is probably better, i'll fix.

(In reply to comment #94) > > trick_taint($token); > > Writing trick_taint() here will avoid duplication later. > > while other reviewers have said that the trick_taint always belongs as close to > the sql call as possible :| Sorry for jumping in. The answer is: it depends. For best results, trick_taint should happen right after validation. You're marking data as "good" with it, and the best place to do this is right after validation -- other code may then rely on that. Such trick_taint calls would usually be rather far away from the SQL call. Having said that, semi-validation may consist of a comment line saying "we're using this in a SELECT only" or similar. Calls of trick_taint after such a comment should be rather close to the SQL call, and care must be taken when adding code later (think adding other SQL). I'm with Frédéric here regarding the trick_taint placement (at the top). Please add a comment, though, along the lines of "Untainting is safe here because we're using the token ID to user-specifically SELECT and DELETE from the tokens table only".

through the reworking of moving chunks of validateSessionToken into Token.pm, the trick_taint issue is mute. however it's good to get clarification on its usage - thanks :) note that we can't use logout then login as that would set the login cookie.

Comment on attachment 183111 [details] [diff] [review] token+redirect v5 If I understand correctly, all this token stuff is only to display useful-links.html.tmpl correctly when ThrowUserError() is called? I cannot see anywhere else you would need to be correctly identified. Removing Bugzilla->switch_user($userid); from validateSessionToken() works fine for me, except links that are different (same as if you were not logged in, which does not hurt IMHO). So unless I miss something important, we could remove all this token stuff. I need other opinions though.

Could it be that it isn't required on your particular setup (same IP address, maybe, or aliases of the same host)?

(In reply to comment #98) > Could it be that it isn't required on your particular setup (same IP address, > maybe, or aliases of the same host)? Yeah.. It could be. I use localhost for the urlbase and 127.0.0.1 for the attachmentbase, but my logincookies table only has 127.0.0.1 entries, which would explain why I can access attachments even when removing Bugzilla->switch_user($userid). So maybe is glob right! ;)

don't forget to test with attachments that have restricted access.

Isn't patch v5 obsolete per our discussion in IRC ??

the discussion centered around avoiding the requirment to create a token where possible. so the flow of things should become: urlbase: if (attachment is public) redirect to attachmentbase without token else validate that user can see attachment generate token, bound to attachmentid redirect to attachmentbase with token attachmentbase: if (attachment is public) view attachment else if (no token) redirect to urlbase else validate token is correct attachment id view attachment - tokens to exipre after 5 minutes and on logout - viewall and edit attachment to avoid redirect by issuing tokens and using attachmentbase as src

in progress patch. still have to do the 5 minute expiry, and update viewall & edit to pre-generate tokens.

mostly done, need to iron out an issue when viewing all attachments on a non-public bug.

Comment on attachment 185567 [details] [diff] [review] in progress >+sub IssueSessionToken { ... >+} >+sub GetToken($) { >+ # Returns the userid and eventdata for the specified token >+ >+ my ($token) = @_; >+ return unless defined $token; >+ trick_taint($token); >+ >+ my $dbh = Bugzilla->dbh; >+ return $dbh->selectrow_array("SELECT userid, eventdata FROM tokens WHERE token = ?", >+ undef, $token); >+} I would like to know the date when this token was created, so that I can check how old the token is. >+sub DeleteToken($) { ... >+} I need all these routines in order to fix bug 281181 (something I would like to do before 2.20). Could you please update your patch asap or even better open a new bug in order to get them quickly? If we commit these routines separately, I could write my patch without waiting for yours. ;)

i've resolved the issue with viewall :) this patch requires the patch on bug 297646.

Comment on attachment 186321 [details] [diff] [review] token+redirect v6 note to self: remove debugging code before uploading.

ok, sorry about that.

Comment on attachment 186322 [details] [diff] [review] token+redirect v7 this is obsolete; need to update the token stuff following changes introduced by bug 297646

requires the patch on bug 297646.

*** Bug 299443 has been marked as a duplicate of this bug. ***

(In reply to comment #110) > requires the patch on bug 297646. OK, after a long batter, bug 297646 has been checked in. Does that make this patch usable now? The current patch appears to be bitrotted in attachment.cgi.

Comment on attachment 187003 [details] [diff] [review] token+redirect v8 bitrot due to attachment.cgi! >Index: attachment.cgi > sub validateID > { >+ $param = 'id' unless $param; Nit: $param ||= 'id'; >+# Determines if the attachment is public -- that is, if users who are >+# not logged in have access to the attachment >+sub attachmentIsPublic { >+ my($attach_id, $bugid, $isprivate) = @_; >+ >+ if ($isprivate && Param("insidergroup")) { >+ return UserInGroup(Param("insidergroup")); >+ } If Param("insidergroup") is defined, you already know that the attachment is not public. > sub view > { >+ } else { >+ # no need to validate token for public attachments >+ if (!attachmentIsPublic($attach_id, $bugid, $isprivate)) { Nit: the comment says the opposite of what the test does. I would say "validate token for non-public attachments". >+ # - tokens to expire after 5 minutes or at logout >+ # - viewall and edit attachment to avoid redirect by issuing tokens and using attachmentbase as src These checks are missing. >Index: Bugzilla.pm >+sub switch_user { >+ my $class = shift; >+ $_user = new Bugzilla::User(@_); >+ return $_user; >+} Nit: switch_user() makes me think that cookies are also available for this user. What about "switch_to_restricted_user" or something similar? I couldn't apply your patch but what I have seen so far looks good.

*** Bug 322819 has been marked as a duplicate of this bug. ***

I'm tired by this bug and its dupes. glob, could you update your patch? We are frozen, meaning that the code won't change a lot before the release of 2.22 (even more true about 2.20 and 2.18). So you shouldn't bitrot again and again. And meanwhile, 2.16 will probably reach its EOL, so a backport for the 2.16 branch will probably not be required. I'm ready to review your patches again if nobody else wants to.

(In reply to comment #115) > And meanwhile, 2.16 will probably reach its EOL, so a backport for the 2.16 > branch will probably not be required. Hmm, well, I'm strongly interested in having a backport for the 2.16 branch as I maintain the Debian packages of Bugzilla, and sarge provides 2.16.7 So this patch is really welcome from the Debian side ;)

(In reply to comment #116) > Hmm, well, I'm strongly interested in having a backport for the 2.16 branch as > I maintain the Debian packages of Bugzilla, and sarge provides 2.16.7 > > So this patch is really welcome from the Debian side ;) > As discussed during our IRC meetings, we are not going to support 2.16 any longer (as soon as 2.22 is released) and we won't provide any patch for 2.16 anymore. In other words, you will have to write it yourself, maybe based on the backport for 2.18. Writing these patches ourselves would mean that we will still support 2.16 when 2.22 is released, which is not the case.

personally i'd go for bugXXX.bugzilla-attachments.mozilla.org ie. force a distinct common domain and a distinct bug host so you can't even share cookies between attachments on different bugs if scripts set them.

So even with all this stuff, i think that any attachment can still read any other attachment that the user has access to. An attachment, A, simply has to include an <iframe> pointing to another attachment, B. We will then redirect to bmo and back and load attachment B inside the iframe. Attachment A can then get any information from B using normal DOM methods since the two files will be located on the same domain. I don't see any way to get around that without separate domains for each bug like previous comment suggests.

> So even with all this stuff, i think that any attachment can still read any > other attachment that the user has access to. yes, but the current patch is an order of magnitude better than the existing situation. i barely have time to update the current patch so it applies (hopefully this weekend) so i recommend that timeless's suggestion will have to be taken up by another bug. it'll have to be optional anyhow as a lot of sysadmins don't like wildcard dns entries.

I guess what i'm arguing is that we use a simpler system then the current one. Rather then directing back and forth we might as well just display a log-in page and set a cookie. There is no value in stealing the cookie since the only thing it'll get you is other attachments which you can get anyway.

Alternativly, we do the token and redirect thing, but we also set a cookie. That way we only need to redirect to get authenticaion once. We also wouldn't need a login page for the attachment-server.

Support for the 2.16 branch is over. Also removing old flags.

*** Bug 345011 has been marked as a duplicate of this bug. ***

Okay, I can agree that this should block our next release, now that: * We have a solution * All our supported releases have Bugzilla::Token

Removing these flags as we released these versions already. And nobody seems to want to take it for 3.0rc1.

Byron, are you going to update and finalize your patch or should I do it myself? I will have some time these next few weeks (vacation!).

sorry, things have been pretty insane for me recently :( it's probably best if you take this.

This is mainly the same patch as v8, but updated to match our current code. AFAIK, the expiration after 5 minutes is not required as we delete the token as soon as it has been checked. Also, I'm not sure what your comment about viewall was. Did you want to avoid the multi back and forth redirects between the main host and the alternate host? I'm not sure that's even possible so I didn't worry about this case, especially because viewall is probably not used very often. Per discussion with justdave, timeless and bz, I prevent the interaction between attachments if the referrer is known and comes from another bug. But I allow attachments to interact if they come from the same bug. I can attach an evil attachment (which you can then attach to your test installation) to help you test my patch, if you want to. Just let me know.

One important note: if ssl = 'always', then your attachmentbase URL must also start with https://, else you get infinite redirects between both hosts.

(In reply to comment #132) > One important note: if ssl = 'always', then your attachmentbase URL must also > start with https://, else you get infinite redirects between both hosts. Does the parameter checker check that? That's important... :-)

(In reply to comment #133) > Does the parameter checker check that? That's important... :-) No it doesn't as you can set ssl to 'always' after setting attachmentbase. I rather thought about adding a comment about this, eventually, but no check. We must be sure that we are not preventing ourselves from editing these params with too restrictive checks. So I prefer to let the admin do this check for us. I can still change my mind later.

I don't think parameters should ever be allowed to be in an inconsistent state, particularly one that causes endless loops and that you couldn't then fix without editing data/params! Both those params are on the same page, so the param-checker should be able to handle it.

Both params are not on the same page as one is about the whole website (urlbase) and the other one about attachments (and so is in the Attachment section), but I could hack |new Bugzilla::CGI| to ignore the HTTP vs HTTPS check when going to the alternate URL. I just found a bug in my patch which prevents us to edit attachments as comment. I get: Erreur : uncaught exception: [Exception... "Access to property denied" code: "1010" nsresult: "0x805303f2 (NS_ERROR_DOM_PROP_ACCESS_DENIED)" location: "https://localhost/bugzilla/attachment.cgi?id=45&action=edit Line: 201"] I will have to update my patch, but justdave and glob can still have a look at this one, in case there is something else to fix.

This patch fixes both problems mentioned earlier: - No ping-pong anymore between both hosts if ssl = always but attachmentbase doesn't start with https://. - You can again edit attachments as comment. Moreover, thanks to this patch, we no longer use XMLSerializer, which is a very good thing as it shouldn't handle files with the text/plain MIME, see bug 86012. The only drawback with this fix is that we no longer can get the content of the iframe to populate the textarea when editing the attachment as comment, because the attachment is viewed from the alternate host and JS restrictions prevent to access it. So what I do is that I prefill the textarea with the attachment data, but only if its MIME is text/*. This means each attachment with the text/* MIME is loaded twice. But even AJAX couldn't make it better as it would have to download the attachment again too. I talked about this problem with justdave and myk, but we couldn't find a better idea. I suppose we can live with that.

> data, but only if its MIME is text/*. This means each attachment with the > text/* MIME is loaded twice. But even AJAX couldn't make it better as it would > have to download the attachment again too. This is, of course, IE's fault; if it weren't broken, we could serve text/plain attachments from the same hostname, and only dangerous ones from an alternative hostname. <sigh> Gerv

> This is, of course, IE's fault; if it weren't broken, we could serve text/plain > attachments from the same hostname, and only dangerous ones from an alternative > hostname. <sigh> We could do User-Agent sniffing. It's not that hard to detect IE.

> We could do User-Agent sniffing. It's not that hard to detect IE. That sounds like a bad idea given (1) proxy caching and (2) other browsers sniffing in certain cases (e.g. Firefox always sniffing for feeds and sniffing for other things if it detects non-ASCII characters). It might also break multi-part testcases, if Bugzilla decides a CSS part of a testcase is "safe", for example. Better to be consistent IMO.

Comment on attachment 273158 [details] [diff] [review] patch for 3.2 (and 3.0?), v10 This seems to work as advertised when applied to my install on landfill, and using either the IP address or landfill.mozilla.org for my attachmenturlbase. >+sub validate_referrer { >+ my ($attachment, $urlbase) = @_; >+ my $referrer = $ENV{'HTTP_REFERER'}; >+ >+ if ($referrer && $referrer =~ /^\Q$urlbase\Eattachment\.cgi\?id=(\d+)/) { >+ # The referrer is an attachment. We only let it access another >+ # attachment if coming from the same bug. This is a nice trick here. I worry that it might break some testcases on bugzilla.mozilla.org where someone uses files from another bug, but it's something they'll have to live with I suspect. Generally I try to avoid doing any kind of security based on Referer headers, but we're only acting here *if* there's a Referer header, and *if* the Referer header *does* match a specific thing. The folks who disable or modify their Referer header hopefully know what they're doing, and will be excluded from this security check (and thus have less security). I guess they should be prepared for things like that if they screw with it. :( >+ # Redirect to SSL if required, unless we are on the alternate host. LpSolit and I discussed this on IRC the other day, and I'm still not sure what to do with it. If you use SSL and it's really the same host, and you have this set up properly, the user is going to get a certificate warning for a hostname mismatch when they hit the attachment. I got that with it set up on landfill with the attachmentbase set to https://landfill.mozilla.org/bzdave/ . In order to honor the usessl setting properly (the off/authenticated sessions/always setting) we probably really need two attachmentbase params (one for SSL and one without) but that's probably overkill. FWIW, the new hack for "Edit Attachment as Comment" actually works better for me... see, I have this Greasemonkey script running that puts a colored border around the page based on which username I'm logged in with. Unfortunately, the way it hacks this winds up with a text/plain document in the iframe being surrounded with <html><body style="border: solid red 3px"><pre> for example, which is then included in my comment when I start it with the old code. With the new code, since Bugzilla is loading the attachment content itself instead of letting my browser do it, it doesn't get that added code from Greasemonkey included in it. Overall, I think this looks pretty thoroughly done, unless anyone has additional comments about the SSL issue.

A referrer check won't prevent a malicious attachment in a public bug from reading attachments in a security-sensitive bug. The malicious attachment can cause browsers to not send referrers in many ways, including using redirects from https.

(In reply to comment #142) I would say that's the browser's fault to let a script alter this kind of information. Anyway, the goal here is to use the referer as an additional security level rather than being *the* security level. The real security improvement is to use an alternate host, not to check the referer.

Comment on attachment 273158 [details] [diff] [review] patch for 3.2 (and 3.0?), v10 Also asking myk, to be sure we don't miss something. glob, still with us? ;)

> I would say that's the browser's fault to let a script alter this kind of > information. Script isn't allowed to alter it, but script is allowed to hide it for certain types of requests. I agree that this is bad, but I lost that battle long ago (see e.g. bug 141641 comment 1). > The real security improvement is to use an alternate host Does this prevent attachments from one bug reading attachments on other bugs?

(In reply to comment #145) > Does this prevent attachments from one bug reading attachments on other bugs? Yes. I discussed about this with bzbarsky on IRC, and he asked me to let an attachment access other attachments inn the *same* bug. But I deny access to attachments being on another bugs if we detect that the request comes from the alternate host (based on the referer).

I just pointed out that the referrer check won't work. With that check defeated, does this patch prevent attachments from one bug reading attachments on other bugs?

(In reply to comment #147) > I just pointed out that the referrer check won't work. With that check > defeated, does this patch prevent attachments from one bug reading attachments > on other bugs? No, as we have no way to know who calls the 2nd attachment. You would get the same result if you type the URL in the address bar directly. You wouldn't want to prevent this way to access attachments, isn't it?

I want attachments to be isolated from each other, using one hostname per bug (or two if the bug has private attachments) or one per attachment.

(In reply to comment #149) > I want attachments to be isolated from each other, using one hostname per bug > (or two if the bug has private attachments) or one per attachment. Isolating attachments from each others is definitely something many people will complain about. (21:59:05) LpSolit: one host per attachment?? (21:59:08) bz: yes (21:59:13) bz: or rather (21:59:16) bz: one host per bug (21:59:30) bz: you want attachments in a single bug to be able to talk to each other

We seem to be trying to fix two problems: 1) Prevent attachments stealing Bugzilla credentials 2) Prevent attachments stealing private Bugzilla data This patch clearly fixes the first, but only has referer-based checking against the second, which doesn't work. Now, we could either check this in, and say "great, we've fixed problem 1", and then work on problem 2, or we could try and change the patch to fix problem 2 first. There is a middle ground between "isolating the attachment of all bugs from each other", and "allowing all attachments to access all other attachments". A good first step, which would fix b.m.o., would be to have one domain for bugs which are in a group, and one domain for bugs in no group at all. A further step would be to have one domain per group - except that a bug can have multiple groups. Can we use an algorithm something like the following? - Make a list of the textual names of all the groups a bug is in - Sort it in alphabetical order - Turn that into a dot-separated domain set - Serve the attachment from that domain So: https://marketing-private.mozillaorgconfidential.security-group.attachments.bugzilla.mozilla.org?attachment.cgi?id=XXXXXX ? I _think_ this scheme would have the right inter-bug visibility properties. Gerv

(In reply to comment #151) > ? I _think_ this scheme would have the right inter-bug visibility properties. Not at all. If a bug is restricted to 2 groups, its attachments can only access attachments which are in the same 2 groups, but not those being in one group or no group. This is over-complicated and doesn't fix anything as I could still steal other private attachment data.

So are we suggesting one hostname per bug? Gerv

(In reply to comment #153) > So are we suggesting one hostname per bug? This would be the "ultimate" solution, yes. This solution has already been proposed, but we fear some admins may not agree to use wildcards for their DNS. Jesse on IRC suggested to have an additional parameter allowing us to enable/disable the "on host per bug" feature. If you turn it off, then my patch would apply as is, i.e. use the referer to restrict access to other bugs. If it's turned on, then the one host per bug solution would prevent the access to all attachments which are not in the same bug. So admins who worry (or care?) a lot about security could enable this param if they want to.

Since referrer checking doesn't work, I think we should just remove that. The only thing it does is give a false sense of security.

Please note that wildcard SSL certs can only be used for the one level for which they have been bought. For example, a wildcard SSL cert of *.mozilla.org can be used for bugzilla.mozilla.org, www.mozilla.org, etc., but it cannot be used for attachments.bugzilla.mozilla.org, as that's another level past what the wildcard cert says. gerv's idea of multiple subdomains (comment #151) will not work because it would be impossible to have SSL encryption for all those subdomains. Mozilla will have to get another wildcard cert (maybe *.bugzilla.mozilla.org) for a fix that requires multiple subdomains to work, but that isn't much of a problem. So, something like 123456.bugzilla.mozilla.org would work well for attachments in bug #123456, if it is necessary to have different hostnames for each bug to separate them.

(In reply to comment #156) If we implement the "one host per bug" solution, the subdomain would probably be of the form bug-123456.mozilla.org, or bugzilla-123456.mozilla.org. In no way I'm going to implement gerv's idea, though.

Doesn't the zone of trust for javascript DOM access extend to documents within a given level of subdomains? I seem to recall someone saying that the top-level domain needed to be different for there to be any guarantee of security. But maybe I'm imagining it.

Security checks use the whole domain name, modulo things setting document.domain. If document.domain is used, both pages involved have to explicitly set it to the same ancestor domain to touch each other.

Comment on attachment 273158 [details] [diff] [review] patch for 3.2 (and 3.0?), v10 >Index: attachment.cgi >+if ($action ne "view" && $cgi->using_attachment_base) { >+ $cgi->redirect_to_urlbase; >+} >+ > if ($action eq "view") > { > view(); Nit: instead of putting this before the |if ($action eq "view")| conditional, put it afterwards so you don't end up testing for $action eq "view" twice in a row, i.e.: if ($action eq "view") { view(); } elsif ($cgi->using_attachment_base) { $cgi->redirect_to_urlbase; } elsif ($action eq "interdiff") { interdiff(); } >@@ -124,7 +129,8 @@ exit; > # Validates an attachment ID. Optionally takes a parameter of a form > # variable name that contains the ID to be validated. If not specified, > # uses 'id'. >-# >+# If the second parameter is true, the attachment ID will be validated, >+# however the current user's access to the attachment will not be checked. Nit: instead of having validateID call validateAttachAccess, have the callers of validateID call validateAttachAccess themselves. Then you don't need the extra param and it'll be clear to someone looking at the calling code that the code is validating both the attachment ID and the user's access to the attachment. >+sub validate_referrer { >+ my ($attachment, $urlbase) = @_; >+ my $referrer = $ENV{'HTTP_REFERER'}; >+ >+ if ($referrer && $referrer =~ /^\Q$urlbase\Eattachment\.cgi\?id=(\d+)/) { >+ # The referrer is an attachment. We only let it access another >+ # attachment if coming from the same bug. >+ my $attach_caller = Bugzilla::Attachment->get($1); >+ if ($attach_caller->bug_id != $attachment->bug_id) { >+ ThrowUserError('auth_failure', {action => 'access', object => 'attachment'}); >+ } >+ } >+ return 1; >+} Hmm, I don't think we can count on referrers being correct. I hope we're not doing so. Nit: also, we can't count on "id" being the first URL parameter in the referrer. It could be "action" instead (or something else we come up with in the future), so the regexp shouldn't require the presence of the "id" parameter (or should allow it to appear anywhere after the question mark). Also, shouldn't we make sure that the other attachment from the same bug is similarly public? Or are we counting on callers of this function to do that check? >+ <iframe id="viewFrame" src="attachment.cgi?id=[% attachment.id %]" style="height: 400px; width: 100%;"> We should find some way to stretch the height of this iframe to the height of the viewport. But that's another bug. Other than the nits, this looks good.

I updated my patch to remove the referrer check and to implement the one host per bug feature (thanks to a magic %bugid% string in the attachmentbase URL). But I just found that we have a problem when the installation has the 'requirelogin' parameter turned on. We have to force attachment.cgi to not require credentials when you are on the alternate host with a valid token, but I fear it would be possible to bypass some checks this way. As you have no cookie defined on the 2nd host, we have no way to make sure the token was really generated by you. I have to investigate. I will attach my patch as soon as I've fixed this last remaining issue.

This patch addresses everything I mentioned in comment 161. Note that I cannot test the "one host per bug" feature locally; I hope justdave can test it on landfill.

Unbitrot

Comment on attachment 278254 [details] [diff] [review] patch for 3.2, v11.1 FTR, in Launchpad, we solved this the same way, by hosting all attachments on a different hostname (launchpadlibrarian.net). >Index: attachment.cgi >+# Byron Jones <bugzilla@glob.com.au> Why not pat yourself in the back, since some of the code here is yours. >+my $urlbase = correct_urlbase(); >+ >+# You must use the appropriate urlbase/sslbase param when not viewing an attachment. "when doing anything but viewing an attachment" is clearer. > # Display an attachment. >+ my $attachbase = Bugzilla->params->{'attachmentbase'}; >+ >+ if ($attachbase ne '' && $attachbase ne Bugzilla->params->{'urlbase'}) { You could have a function which made the check above clearer (something like if use_attachbase) and could be used later in this file as well. >+ # Make sure the attachment is served from the correct server. >+ if ($cgi->self_url !~ /^\Q$attachbase\E/) { >+ # We couldn't call Bugzilla->login earlier as we first had to make sure >+ # we were not going to request credentials on the alternate host. >+ Bugzilla->login(); >+ if (attachmentIsPublic($attachment) && !Bugzilla->params->{'requirelogin'}) { I don't quite understand the need to check requirelogin here. >Index: Bugzilla/CGI.pm >+ && i_am_cgi() >+ && $self->self_url =~ /^\Q$urlbase\E/) I really dislike this regexp. Could you before landing make this into a function like i_am_cgi() and use it where it is used. >+ # Redirect to urlbase if we are not viewing an attachment. >+ if (Bugzilla->params->{'attachmentbase'} ne '' Is there a good reason to do this? Is it to avoid people accessing Bugzilla itself through the attachment host? >+# Redirect to the urlbase version of the current URL. >+sub redirect_to_urlbase { >+ my $self = shift; >+ my $path = $self->url('-path_info' => 1, '-query' => 1, '-relative' => 1); >+ print $self->redirect(-location => correct_urlbase() . $path); Nit: is there a reason that we are inconsistent here and quote the named arguments to url() but not the '-location' supplied to redirect()? >Index: Bugzilla/Config/Attachment.pm >+ name => 'attachmentbase', Would the name attachmenturlbase be clearer? >Index: template/en/default/attachment/edit.html.tmpl >+[%# No need to display the Diff button and iframe if the attachment is not a patch. %] >+[% patchviewerinstalled = (patchviewerinstalled && attachment.ispatch) %] Really? Don't you like having this option available for text files and other sorts of attachment? At least I don't quite see why this should be done in the same change. > <script type="text/javascript"> I don't quite understand the change to the JS in this file either. >Index: template/en/default/admin/params/attachment.html.tmpl >+ "If you can't set up an alternate hostname, or aren't worried " _ >+ "about the security implications, leave this field empty.", I would say "and aren't worried" instead of using "or".

Hmm. This seems to have stalled. But there's no point in LpSolit updating the patch unless Dave or Myk can give some sort of assurance that they have time to review it. (It's on MoCo's wanted list of Bugzilla patches, after all: http://wiki.mozilla.org/Bugzilla_Fixup ). Dave? Myk? Gerv

besides being stalled, is the last patch close to working? the patch doesn't seem large.

some random thoughts on bugzilla. iss have reported xss in bugzilla - this has great comic value. hostname and stealing cookies aside, attachments are kind of free anonymous web hosting. phishing and malware problems? iirc wikipedia suffered from this, don't remember how they solved the problem.

(In reply to comment #167) > besides being stalled, is the last patch close to working? The last patch is supposed to work, yes. Just waiting for justdave and myk to put this review higher in their priority list. I know there is some bitrot, but as I said to other reviewers per email, I don't plan to update my patch till this one is reviewed. I already unbitrotten it once, and see the result: the patch is here for one year now. If this issue was really a problem for MoCo, I supposed it would have got much more traction these last few years.

Comment on attachment 278254 [details] [diff] [review] patch for 3.2, v11.1 Canceling this review request, as I'm no longer able to do Bugzilla reviews.

i haven't examined the patch in detail, but does it prevent stealing attachments from the ``different hostname''? i mean in theory malicious attachment can read secret attachments from the ``different hostname'' - they are on the same hostname. are attachment URIs easy to guess - e.g. differing by increasing numbers in a known range?

oops, sorry the previous comment was written in a hurry. after reading some of the 170 comments found out there should be some protection against stealing attachments. btw, i like the idea for one host per bug if feasible on the server side.

(In reply to comment #172) > btw, i like the idea for one host per bug if feasible on the server side. Looks like you are answering your own questions, which is good. :-D Yes, the goal of the last patch is to define one host per bug. One host per attachment, as requested by a few users, is not something we want. My only concern could be the increased traffic between hosts due to redirections, which may slow down Bugzilla.

what is the reason for not checking this in? probably the documentation should explain how to setup wildcard dns and multiple hostnames on the webserver if necessary.

(In reply to comment #174) > what is the reason for not checking this in? It has not been reviewed yet. justdave's and myk's r+ for patch v10 are not enough to commit patch v11.1 as I made some significant changes.

Comment on attachment 278254 [details] [diff] [review] patch for 3.2, v11.1 >+# You must use the appropriate urlbase/sslbase param when not viewing an attachment. >+if ($action ne 'view') { >+ if ($cgi->self_url !~ /^\Q$urlbase\E/) { >+ $cgi->redirect_to_urlbase; >+ } That'll do an endless redirect loop on installations that have ssl eq 'always' (because you don't check for sslbase). >+sub validateAttachAccess { On HEAD (after we check in this patch), I want to see $user->can_see_attachment instead of this. >+sub attachmentIsPublic { And this should be $attachment->is_public (upstream, at least--I understand that this would probably be painful on 2.20 or 2.22). >+ if (attachmentIsPublic($attachment) && !Bugzilla->params->{'requirelogin'}) { attachmentIsPublic already checks requirelogin. >+ if (!attachmentIsPublic($attachment) || Bugzilla->params->{'requirelogin'}) { Same comment here about requirelogin. (And elsewhere, too.) >+ # Not a valid token. >+ print $cgi->redirect('-location' => correct_urlbase() . $path); Um, won't that create an endless redirect loop for invalid tokens? It looks like "your token is invalid, go back to the page that redirected you here". Shouldn't we just throw an error? >+ # Change current user without creating cookies. >+ Bugzilla->set_user(new Bugzilla::User($userid)); That could cause a problem because that user won't have an authorizer(). However, we're just displaying an attachment, so... >+ if (attachmentIsPublic($attachment) && !Bugzilla->params->{'requirelogin'}) { >+ $vars->{'token'} = '-'; Why not just use an empty string? >Index: Bugzilla/CGI.pm >+ # Redirect to SSL if required, unless we are on the alternate host. >+ my $urlbase = Bugzilla->params->{'urlbase'}; > if (Bugzilla->params->{'sslbase'} ne '' > && Bugzilla->params->{'ssl'} eq 'always' >- && i_am_cgi()) >+ && i_am_cgi() >+ && $self->self_url =~ /^\Q$urlbase\E/) I'm guessing that this will have to be fixed in a different way now on 3.2 and tip once we check in the ssl redirect bug. >+ my $cgi_file = $self->url('-path_info' => 0, '-query' => 0, '-relative' => 1); I think you can get that more reliably out of $ENV{SCRIPT_NAME} or something like that. >Index: template/en/default/attachment/edit.html.tmpl >+ attachmentbase => "An alternate URL that is the common initial leading part of " _ [snip] This is a good description of how to use it, but I think we need slightly more info: It is possible for a malicious attachment to steal your cookies or access other attachments to perform an attack on the user. If you would like additional security on attachments to avoid this, set this parameter to an alternate URL for your $terms.Bugzilla that is not the same as your urlbase or sslbase. That is, a different domain name that resolves to this exact same $terms.Bugzilla installation. For added security, you can insert %bugid% into the URL, which will be replaced with the ID of the current $terms.bug that the attachment is on, when you access an attachment. This will limit attachments to accessing only other attachments on the same $terms.bug. Remember, though, that all those possible domain names (such as "1234.your.domain.com") must point to this same $terms.Bugzilla instance. Otherwise, I really like how this is implemented, particularly how you avoided tokens for public attachments.

> >+ # Not a valid token. > >+ print $cgi->redirect('-location' => correct_urlbase() . $path); > > Um, won't that create an endless redirect loop for invalid tokens? It looks > like "your token is invalid, go back to the page that redirected you here". > Shouldn't we just throw an error? it's been a while, but from what i recall this is here to ensure that if i email you a direct link to an attachment, and you don't have a token, you are redirected to get a token. > >+ my $cgi_file = $self->url('-path_info' => 0, '-query' => 0, '-relative' => 1); > > I think you can get that more reliably out of $ENV{SCRIPT_NAME} or something like that. no, SCRIPT_NAME doesn't include the protocol, host or port. also $cgi->url() draws information from SCRIPT_NAME, so it's just as "reliable".

Per discussion with justdave on IRC, we are not going to take it for 2.2x. The patch is too invasive and prone to regressions.

(In reply to comment #177) > it's been a while, but from what i recall this is here to ensure that if i > email you a direct link to an attachment, and you don't have a token, you are > redirected to get a token. Yes, you are correct. I plan to update my patch really soon now (as in the coming days, not hours), based on comment 165 and comment 176 (note to self).

I addressed everything which seemed relevant to me from kiko's and mkanat's review comments.

Bah, I forgot to include Util.pm.

I fixed a problem in the regexp in attachment.cgi + added POD to Util.pm. No other changes.

Comment on attachment 350698 [details] [diff] [review] patch for 3.2 + tip, v12.1 >Index: Bugzilla/CGI.pm >+ # Redirect to urlbase if we are not viewing an attachment. > [snip] I'm concerned about this. I'm fairly sure that it's not safe to do things like this inside of Bugzilla::CGI::new, because it can be called in non-CGI environments. Probably what you should do is put this inside Bugzilla::init_page wrapped with an i_am_cgi() check, or something like that. >+ if (use_attachbase()) { >+ my $cgi_file = $self->url('-path_info' => 0, '-query' => 0, '-relative' => 1); Why query => 0? >+=item C<redirect_to_urlbase> >+ >+Redirects from the current URL to one prefixed by the urlbase parameter. (or sslbase parameter, as appropriate). However, since you're doing all of this before we log in, in fact we can't get the correct_sslbase for authenticated_sessions, so there would be two redirects in this case. I think that's probably OK. >Index: Bugzilla/Config/Attachment.pm >+ name => 'attachmentbase', Let's be modern about it and call it attachment_base. >+ checker => \&check_urlbase Will that allow an https URL? >Index: template/en/default/attachment/edit.html.tmpl >@@ -47,37 +50,7 @@ > var has_viewed_as_diff = 0; > function editAsComment() Um, are we entirely removing the edit_as_comment functionality? >+ [% IF token %] That will always be true, because you made token "-" for the null case. It's probably best to not even define token if the attachment is public. >+ %]attachment.cgi?id=[% attachment.id %]&t=[% token FILTER html %]" Shouldn't that filter be url_quote? >+ attachmentbase => "It is possible for a malicious attachment to steal your cookies or access " _ It'd be nice if these lines were less than 80 characters. You could probably accomplish this partially by moving the string under "attachmentbase =>" and just indenting it two spaces. Everything else looks great.

(In reply to comment #183) > >+ my $cgi_file = $self->url('-path_info' => 0, '-query' => 0, '-relative' => 1); > > Why query => 0? Because all I want is the file name, to know if it's attachment.cgi or not. I don't want parameters passed to it. > >+ checker => \&check_urlbase > > Will that allow an https URL? yes. > Um, are we entirely removing the edit_as_comment functionality? No, this feature is still there. But instead of letting XMLSerializer() set the comment, I pass it directly using attachment.data. > >+ [% IF token %] > > That will always be true, because you made token "-" for the null case. No, the token is undefined if the attachment is public or if there is no alternate host. I will check this point again. > >+ %]attachment.cgi?id=[% attachment.id %]&t=[% token FILTER html %]" > > Shouldn't that filter be url_quote? Ah yes, it should. :) > Everything else looks great. Nice, thanks. :)