38862 - [SECURITY] attachments should be at a different hostname

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, defect, P2)

Description

[Jesse Ruderman]

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.

Comment 1

[Jesse Ruderman]

Attachment: pops up an alert containing your cookie

Comment 2

[Jesse Ruderman]

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

Comment 3

[Jesse Ruderman]

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

Comment 4

[Dave Miller [:justdave]]

moving to real milestones...

Comment 5

[Myk Melez [:myk] [@mykmelez]]

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.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

Attachment: patches for showattachment.cgi, bug_form.pl, and globals.pl

Comment 7

[Myk Melez [:myk] [@mykmelez]]

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.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Attachment: includes CGI.pl along with the others

Comment 9

[Jacob Steenhagen]

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.

Comment 10

[Dave Miller [:justdave]]

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

Comment 11

[Dave Miller [:justdave]]

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.

Comment 12

[Jacob Steenhagen]

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 :)

Comment 13

[Myk Melez [:myk] [@mykmelez]]

Attachment: updated patch using Param('ipbase')

Comment 14

[Myk Melez [:myk] [@mykmelez]]

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.

Comment 15

[Jacob Steenhagen]

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.

Comment 16

[Myk Melez [:myk] [@mykmelez]]

Attachment: latest version

Comment 17

[Myk Melez [:myk] [@mykmelez]]

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.

Comment 18

[Dave Miller [:justdave]]

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.

Comment 19

[Myk Melez [:myk] [@mykmelez]]

Attachment: version of patch that links to CERT advisory

Comment 20

[Jacob Steenhagen]

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?"

Comment 21

[Myk Melez [:myk] [@mykmelez]]

Attachment: the "warn the user" approach

Comment 22

[Myk Melez [:myk] [@mykmelez]]

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 + "_"?

Comment 23

[Myk Melez [:myk] [@mykmelez]]

The answer to that question is unfortunately very complicated...

http://www.rpbourret.com/xml/NamespacesFAQ.htm#q11_2

Comment 24

[Myk Melez [:myk] [@mykmelez]]

Attachment: handles namespace prefixes a little better

Comment 25

[Jacob Steenhagen]

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...

Comment 26

[Myk Melez [:myk] [@mykmelez]]

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.

Comment 27

[Myk Melez [:myk] [@mykmelez]]

Attachment: no red boxes! bullets! case-insensitive! no warnings!

Comment 28

[timeless]

<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.

Comment 29

[Dave Miller [:justdave]]

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

Comment 30

[Jesse Ruderman]

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.

Comment 31

[Dave Miller [:justdave]]

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.

Comment 32

[Dave Miller [:justdave]]

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

Comment 33

[Myk Melez [:myk] [@mykmelez]]

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.

Comment 34

[Dave Miller [:justdave]]

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.

Comment 35

[Dave Miller [:justdave]]

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. :-)

Comment 36

[Jesse Ruderman]

>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.

Comment 37

[Dave Miller [:justdave]]

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.

Comment 38

[Jacob Steenhagen]

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?

Comment 39

[Dave Miller [:justdave]]

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.

Comment 40

[Jacob Steenhagen]

-> New Bugzilla Product

Comment 41

[Dave Miller [:justdave]]

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.

Comment 42

[Bradley Baetz (:bbaetz)]

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 43

[Dave Miller [:justdave]]

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 44

[Dave Miller [:justdave]]

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 45

[Gervase Markham [:gerv]]

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

Comment 46

[Jacob Steenhagen]

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 :)

Comment 47

[Gervase Markham [:gerv]]

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

Comment 48

[Bradley Baetz (:bbaetz)]

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 :)

Comment 49

[Jesse Ruderman]

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

Comment 50

[Gervase Markham [:gerv]]

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 [e-mail].

> 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

Comment 51

[Dave Miller [:justdave]]

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...

Comment 52

[Gervase Markham [:gerv]]

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

Comment 53

[Dave Miller [:justdave]]

sounds reasonable.

Comment 54

[Dave Miller [:justdave]]

Attachment: token+redirect v1 (2.16 branch)

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 55

[Bradley Baetz (:bbaetz)]

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 56

[Jouni Heikniemi]

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.

Comment 57

[Dave Miller [:justdave]]

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.

Comment 58

[Dave Miller [:justdave]]

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....

Comment 59

[Dave Miller [:justdave]]

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.

Comment 60

[Dave Miller [:justdave]]

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.

Comment 61

[Jouni Heikniemi]

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. 

Comment 62

[Jouni Heikniemi]

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.

Comment 63

[Dave Miller [:justdave]]

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

Comment 64

[Dave Miller [:justdave]]

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.

Comment 65

[Dave Miller [:justdave]]

Attachment: token+redirect v2 (2.16 branch)

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.

Comment 66

[Dave Miller [:justdave]]

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

Comment 67

[Jouni Heikniemi]

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.

Comment 68

[Dave Miller [:justdave]]

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

Comment 69

[Bradley Baetz (:bbaetz)]

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)

Comment 70

[Joel Peshkin]

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.

Comment 71

[Jesse Ruderman]

This bug is public because [e-mail] found it independently and mentioned it in
bug 178993 comment 21.  I can't uncheck "Webtools Security-Sensitive Bug", though.

Comment 72

[Dave Miller [:justdave]]

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

Comment 73

[Dave Miller [:justdave]]

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

Comment 74

[Daniel Wang]

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 ;-) )

Comment 75

[Gervase Markham [:gerv]]

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

Comment 76

[Joel Peshkin]

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

Comment 77

[Gervase Markham [:gerv]]

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

Gerv

Comment 78

[Joel Peshkin]

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.

Comment 79

[Dave Miller [:justdave]]

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.

Comment 80

[Joel Peshkin]

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.

Comment 81

[Gervase Markham [:gerv]]

> 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

Comment 82

[Gervase Markham [: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

Comment 83

[Dave Miller [:justdave]]

(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.

Comment 84

[Dave Miller [:justdave]]

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

Comment 85

[Dave Miller [:justdave]]

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

Comment 86

[Gervase Markham [:gerv]]

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

Comment 87

[Daniel Veditz [:dveditz]]

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

Comment 88

[:glob ✱]

Attachment: token+redirect v3

updated for HEAD and new cgi/db routines

Comment 89

[Frédéric Buclin]

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.

Comment 90

[:glob ✱]

> 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.

Comment 91

[:glob ✱]

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

Comment 92

[:glob ✱]

Attachment: token+redirect v4

Comment 93

[Frédéric Buclin]

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'));

Comment 94

[:glob ✱]

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.

Comment 95

[Marc Schumann [:Wurblzap]]

(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".

Comment 96

[:glob ✱]

Attachment: token+redirect v5

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 97

[Frédéric Buclin]

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.

Comment 98

[Christian Reis]

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

Comment 99

[Frédéric Buclin]

(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! ;)

Comment 100

[:glob ✱]

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

Comment 101

[Frédéric Buclin]

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

Comment 102

[:glob ✱]

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

Comment 103

[:glob ✱]

Attachment: in progress

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

Comment 104

[:glob ✱]

Attachment: in progress

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

Comment 105

[Frédéric Buclin]

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. ;)

Comment 106

[:glob ✱]

Attachment: token+redirect v6

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

Comment 107

[:glob ✱]

Comment on attachment 186321 [details] [diff] [review]
token+redirect v6

note to self: remove debugging code before uploading.

Comment 108

[:glob ✱]

Attachment: token+redirect v7

ok, sorry about that.

Comment 109

[:glob ✱]

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

Comment 110

[:glob ✱]

Attachment: token+redirect v8

requires the patch on bug 297646.

Comment 111

[Gervase Markham [:gerv]]

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

Comment 112

[Dave Miller [:justdave]]

(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 113

[Frédéric Buclin]

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.

Comment 114

[Frédéric Buclin]

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

Comment 115

[Frédéric Buclin]

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.

Comment 116

[Alexis Sukrieh]

(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 ;)

Comment 117

[Frédéric Buclin]

(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.

Comment 118

[timeless]

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.

Comment 119

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 120

[:glob ✱]

> 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.

Comment 121

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 122

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 123

[Frédéric Buclin]

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

Comment 124

[Dave Miller [:justdave]]

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

Comment 125

[Max Kanat-Alexander]

Okay, I can agree that this should block our next release, now that:

* We have a solution
* All our supported releases have Bugzilla::Token

Comment 126

[Frédéric Buclin]

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

Comment 129

[Frédéric Buclin]

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!).

Comment 130

[:glob ✱]

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

Comment 131

[Frédéric Buclin]

Attachment: patch for 3.2 (and 3.0?), v9

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.

Comment 132

[Frédéric Buclin]

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

Comment 133

[Max Kanat-Alexander]

(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... :-)

Comment 134

[Frédéric Buclin]

(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.

Comment 135

[Max Kanat-Alexander]

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.

Comment 136

[Frédéric Buclin]

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.

Comment 137

[Frédéric Buclin]

Attachment: patch for 3.2 (and 3.0?), v10

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.

Comment 138

[Gervase Markham [:gerv]]

> 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

Comment 139

[Max Kanat-Alexander]

> 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.

Comment 140

[Jesse Ruderman]

>  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 141

[Dave Miller [:justdave]]

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.

Comment 142

[Jesse Ruderman]

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.

Comment 143

[Frédéric Buclin]

(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 144

[Frédéric Buclin]

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? ;)

Comment 145

[Jesse Ruderman]

> 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?

Comment 146

[Frédéric Buclin]

(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).

Comment 147

[Jesse Ruderman]

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?

Comment 148

[Frédéric Buclin]

(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?

Comment 149

[Jesse Ruderman]

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.

Comment 150

[Frédéric Buclin]

(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

Comment 151

[Gervase Markham [:gerv]]

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

Comment 152

[Frédéric Buclin]

(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.

Comment 153

[Gervase Markham [:gerv]]

So are we suggesting one hostname per bug?

Gerv

Comment 154

[Frédéric Buclin]

(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.

Comment 155

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 156

[Reed Loden [:reed]]

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.

Comment 157

[Frédéric Buclin]

(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.

Comment 158

[Dave Miller [:justdave]]

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.

Comment 159

[Boris Zbarsky [:bzbarsky]]

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 160

[Myk Melez [:myk] [@mykmelez]]

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.

Comment 161

[Frédéric Buclin]

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.

Comment 162

[Frédéric Buclin]

Attachment: patch for 3.2 (and 3.0?), v11

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.

Comment 163

[Frédéric Buclin]

Attachment: patch for 3.2, v11.1

Unbitrot

Comment 165

[Christian Reis]

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 <[e-mail]>

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".

Comment 166

[Gervase Markham [:gerv]]

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

Comment 167

[georgi - hopefully not receiving bugspam]

besides being stalled, is the last patch close to working?

the patch doesn't seem large.

Comment 168

[georgi - hopefully not receiving bugspam]

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.

Comment 169

[Frédéric Buclin]

(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 170

[Myk Melez [:myk] [@mykmelez]]

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.

Comment 171

[georgi - hopefully not receiving bugspam]

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?

Comment 172

[georgi - hopefully not receiving bugspam]

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.

Comment 173

[Frédéric Buclin]

(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.

Comment 174

[georgi - hopefully not receiving bugspam]

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.

Comment 175

[Frédéric Buclin]

(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 176

[Max Kanat-Alexander]

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.

Comment 177

[:glob ✱]

> >+                    # 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".

Comment 178

[Frédéric Buclin]

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.

Comment 179

[Frédéric Buclin]

(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).

Comment 180

[Frédéric Buclin]

Attachment: patch for 3.2 + tip, v12

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

Comment 181

[Frédéric Buclin]

Attachment: patch for 3.2 + tip, v12.1

Bah, I forgot to include Util.pm.

Comment 182

[Frédéric Buclin]

Attachment: patch for 3.2 + tip, v12.2

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

Comment 183

[Max Kanat-Alexander]

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.

Comment 184

[Frédéric Buclin]

(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. :)

Comment 185

[Frédéric Buclin]

Attachment: patch for 3.2 + tip, v13

I think I addressed all your comments.

Comment 186

[Frédéric Buclin]

Also, I checked that Bugzilla->login calls Bugzilla->cgi before doing anything else, so having the redirect stuff in CGI->new is fine I think.

Comment 187

[Frédéric Buclin]

Attachment: patch for 3.2 + tip, v14

To clean up the code a bit and to avoid to create a token which is not immediately validated and deleted, we let attachment.cgi do the redirect when editing an attachment. This costs one additional redirect; but that's probably harmless.

Comment 188

[Max Kanat-Alexander]

Comment on attachment 351276 [details] [diff] [review]
patch for 3.2 + tip, v14

This is a great patch, and it works fine. :-) r=mkanat

Comment 189

[Frédéric Buclin]

Attachment: patch for 3.0, v1

I changed validateID() to return an attachment object rather than the attachment ID and bug ID, because I really need information the object has. This forced me to clean up the code in attachment.cgi a bit, but I'm confident with these changes as the new code baked on the trunk for ~2 years now.

Comment 190

[Frédéric Buclin]

Attachment: patch for 3.0, v1.1

Wow, I forgot a line in edit.html.tmpl (forgot to save changes to the file before creating the patch).

Comment 191

[Max Kanat-Alexander]

Comment on attachment 351446 [details] [diff] [review]
patch for 3.0, v1.1

Looks good. :-) You tested this, yeah?

Comment 192

[Frédéric Buclin]

(In reply to comment #191)
> You tested this, yeah?

Of course. OK, so this bug is ready for checkin, all backports are here. \o/

Comment 193

[Max Kanat-Alexander]

Great. :-)

LpSolit: Would you be in favor of letting bmo run this code (and possibly our other security patches) for a while before we release, just to make sure that we haven't missed any bugs?

Comment 194

[Frédéric Buclin]

(In reply to comment #193)
> LpSolit: Would you be in favor of letting bmo run this code (and possibly our
> other security patches) for a while before we release

I was thinking about having a test installation first, but if justdave agrees to apply all the patches to bmo and be around to back them out if something goes wrong, sure.

Note that we still have some other security bugs to fix before we can release 3.2.1, so I wouldn't disclose what our security fixes are when applied to bmo (we apply them, but that's our secret :) ).

Comment 195

[Frédéric Buclin]

Attachment: patch for 3.2 + tip, v14.1

Now correctly handles sslbase = "".

Comment 196

[Frédéric Buclin]

Attachment: patch for 3.0, v1.2

Also fixes the problem when sslbase is empty, for 3.0.

Comment 197

[Dave Miller [:justdave]]

(In reply to comment #194)
> I was thinking about having a test installation first, but if justdave agrees
> to apply all the patches to bmo and be around to back them out if something
> goes wrong, sure.
> 
> Note that we still have some other security bugs to fix before we can release
> 3.2.1, so I wouldn't disclose what our security fixes are when applied to bmo
> (we apply them, but that's our secret :) ).

So basically this means that I can't commit these to our bzr repo (which is publicly visible) that the production server pulls from, and have to apply the patches manually in production when it comes time to deploy it...

Comment 198

[Max Kanat-Alexander]

(In reply to comment #197)
> So basically this means that I can't commit these to our bzr repo (which is
> publicly visible) that the production server pulls from, and have to apply the
> patches manually in production when it comes time to deploy it.

  Yeah, or wait for us to do the security release, which will probably happen after the holidays (I don't want to do a security release at a time when people can't upgrade.)

Comment 199

[Max Kanat-Alexander]

Comment on attachment 354362 [details] [diff] [review]
patch for 3.2 + tip, v14.1

This is fine, except that once you've done qr// against something, you don't need to // it to compare against it. It's not a big deal, though.

Comment 200

[Max Kanat-Alexander]

Comment on attachment 354364 [details] [diff] [review]
patch for 3.0, v1.2

Yeah, looks fine.

Comment 201

[Max Kanat-Alexander]

I am CC'ing the maintainers of all the major public Bugzilla installations that I know of. If you know of another maintainer of a major public installation, please CC them and point them to this comment. (For some installations, I couldn't find the Bugzilla account of their maintainer.)

We are going to be releasing a version of Bugzilla very soon with this security fix in it. However, it requires a bit of sysadmin preparation, so I thought I'd warn you about it in advance. The security bug is, of course, still confidential, but we are CC'ing you on it so that you have advance warning of the sysadmin tasks necessary to prepare for the fix.

Basically, you will need an alternate domain that resolves to the same IP and virtualhost as your current Bugzilla. For extra security, you should configure a wildcard subdomain that points to the same IP and virtualhost as your current Bugzilla. This additional domain does not need SSL, unless you are concerned about protecting the contents of attachments over the wire, since no cookies or login information will ever be sent to this additional domain--it will only be used for viewing attachments. However, you may use SSL if you wish, and in that case you may need to get a new SSL certificate for this additional domain. 

If you have any questions, feel free to come into #mozwebtools on irc.mozilla.org and ask us.

Comment 202

[Dave Miller [:justdave]]

Two installations I can think of off the top of my head that should be notified about this are Facebook and Activestate.  I bet schrep knows who to add at Facebook (and may know who at Activestate as well).

Comment 203

[Robin H. Johnson [:robbat2]]

mkanat:
1. is there a patch for this that applies to 2.22.x?
2. I saw some weird stuff from a spammer about 2 weeks ago that smells very badly of this attack - at a glance it looked like just a pharmaceuticals spam but there was some weird JavaScript. I just nuked it at the time as spam, now I'm kicking myself that I didn't keep the attachment.

Comment 204

[Max Kanat-Alexander]

(In reply to comment #203)
> mkanat:
> 1. is there a patch for this that applies to 2.22.x?

  There is not, and there won't be. Only 3.0 and 3.2 are getting the fix. We looked into backporting it to 2.22 but it was just too invasive.

> 2. I saw some weird stuff from a spammer about 2 weeks ago that smells very
> badly of this attack - at a glance it looked like just a pharmaceuticals spam
> but there was some weird JavaScript. I just nuked it at the time as spam, now
> I'm kicking myself that I didn't keep the attachment.

  Hmmm. Yeah, I wouldn't be surprised if people have tried this from time to time. It's a general sort of vulnerability in any web application that takes attachments, and this bug itself is eight years old, so I don't think this is the most secret vulnerability in the world. :-) (Still, don't announce or discuss it publicly until we've released.)

Comment 205

[Denis Roy]

Thanks for the heads up.

(In reply to comment #201)
> Basically, you will need an alternate domain

By 'domain' you mean, a subdomain, or an alternate host, right?  For instance, at Eclipse, the Bugzilla login cookie is bound to 'bugs.eclipse.org' so all I will need is something like bugsattachments.eclipse.org right?

Comment 206

[David Lawrence [:dkl]]

Red Hat IT suggested using HttpOnly cookie attributes which would
help this issue.

http://www.codinghorror.com/blog/archives/001167.html

1. If cookies are set HttpOnly client side scripts will not have access to them, only the server.
2. No redirect is necessary.

Problems: 
1. Problem is it may not be universally available in all user agents.
2. Doesn't yet work for XmlHttpRequests so script could use that to call back to the server and then get the cookie contents from that call. 

Number 2 has been fixed in bug 380418 but as I understand it is not publicly available as a Firefox update.

Having the alternate domain fix being proposed does not bode well for clustered/failover environments. RH IT has said that this is a band-aid at best and weak one at that. They suggest that properly sanitizing attachments is a much better route.

Dave

Comment 207

[Frédéric Buclin]

Login cookies already have the HTTPonly attribute, see bug 368502. Sanitizing attachments is not something we are going to do. You have no way to know what a HTML page is supposed to do and in some cases you want the JavaScript code to be executed. You really want an alternate host.

Comment 208

[Denis Roy]

(In reply to comment #206)
> Having the alternate domain fix being proposed does not bode well for
> clustered/failover environments. RH IT has said that this is a band-aid at best
> and weak one at that. They suggest that properly sanitizing attachments is a
> much better route.

+1 from Eclipse IT as well.

(In reply to comment #207)
> in some cases you want the JavaScript code to
> be executed. You really want an alternate host.

IMHO, these are attachments, nothing more, and I wouldn't expect *any* code to run directly from an untrusted attachment (just like any mail client, really).

Comment 209

[Frédéric Buclin]

(In reply to comment #208)
> IMHO, these are attachments, nothing more, and I wouldn't expect *any* code to
> run directly from an untrusted attachment (just like any mail client, really).

In this case, don't view attachments. Download them! Right-click on the attachment link and choose "Save as...".

Comment 210

[Dave Miller [:justdave]]

I've mentioned it a couple times on IRC, but I'll say it again here.  Mozilla needs to have javascript execute in Bugzilla for testcases to work, because the primary product having bugs tracked is a browser.  Mozilla is a special case.  Most places with a Bugzilla don't need to keep the attachments executable in a browser.  This fix fixes the problem on Mozilla's Bugzilla, but it's a pain in the ass to set up if you use SSL on your site (second IP with a new SSL cert, or a wildcard cert with a ServerAltName, etc).  The problem also goes away (without all of this hassle) if you restrict the types of files that can be viewed or downloaded online.  Even for those folks who do want to go the full route that we're already providing, it would give them a backup plan that doesn't require major config changes that they can use until they're ready to do the full switch.  I think it would be a lot of goodwill towards the community if we offered attachment type filters (at viewing time in addition to upload, since presumably people already have existing attachments :) that at a mimimum force downloads of dangerous types instead of viewing them in the browser.

Comment 211

[David D. Kilzer (ddk)]

(In reply to comment #210)
> I've mentioned it a couple times on IRC, but I'll say it again here.  Mozilla
> needs to have javascript execute in Bugzilla for testcases to work, because the
> primary product having bugs tracked is a browser.  Mozilla is a special case.

The WebKit Project needs this functionality as well.

Comment 212

[Frédéric Buclin]

One way to fix the problem for those who aren't going to add an alternate host for attachments is to add a user pref "If the attachment is an HTML file...":

1) display the file in the browser, unaltered,
2) display a trimmed copy of the file (with JS code removed (and iframes?))
3) display the file as plain text
4) download the file

1) is what we currently have, with or without the alternate host.
2) is easy to do with HTML::Scrubber, which is already an optional Perl module used by Bugzilla
3) is a one-liner in attachment.cgi at line 317 (on my patched copy of attachment.cgi). I know IE sniffs the content of the file, but we all know IE sucks, and IE users are free to choose option 4).
4) is also a one-liner at the same line as above.


From a usability point of view, the attachments table could have an additional link or two for MIME type = text/html. Currently, there is only the "Details" link. We could add "View original", "View trimmed" (if HTML::Scrubber is installed), "View source" (a.k.a text/plain, or as text/html but as [% attachment.data FILTER html %], so that it's safe for IE users too). No need for a "Download" link, they can already right-click the attachment name. This way, you could override your user preference on a per-attachment basis.

Would this user preference be useful? Do we want it? (It's very easy to implement AFAIK). Note that this user preference is not invasive and could be easily backported to Bugzilla 3.0 and 2.22. And to make things clear, this user preference would be *in addition* to the alternate host, not a replacement for it.

Comment 213

[Gervase Markham [:gerv]]

LpSolit: surely admin pref, not user pref? "Disable important feature which keeps this Bugzilla secure" is not a user pref you want to have.

Your scheme sounds rather like overkill. What the non-browser-project admins want is a blacklist of executable content attachment types which are always served as Content-Disposition: attachment. We could hard-code that list or make it configurable, it doesn't really matter. It would include:

text/html and other HTML-like MIME types
application/vnd.mozilla.xul+xml
application/svg and image/svg
.*\+xml
...

Here's a list to get us started (grep for "svg"):
http://209.85.229.132/search?q=cache:U7ymDrnpztUJ:www.leviathansecurity.com/pdf/Flirting%2520with%2520MIME%2520Types.pdf+browser+executable+content+types+text/html+image/svg&hl=en&ct=clnk&cd=1&gl=uk

(They've fixed the IE bug with Content-Disposition and sniffing, I believe.)

If there's no way to execute an attachment in the context of the Bugzilla website, we're safe.

Gerv

Comment 214

[Frédéric Buclin]

(In reply to comment #213)
> LpSolit: surely admin pref, not user pref? "Disable important feature which
> keeps this Bugzilla secure" is not a user pref you want to have.

No, I really meant a user pref. If an admin doesn't want to let users choose the option, he is free to do so (disable the pref). And I don't see how my suggestion is overkill. If what bothers you is the sanitized copy and the links, we could have only 3 options for the user pref: download, view source, display the HTML page, and we would display no alternate links. But this would prevent the per-attachment override. Maybe that's a good enough compromise?

Comment 215

[Max Kanat-Alexander]

(In reply to comment #205)
> By 'domain' you mean, a subdomain, or an alternate host, right?  For instance,
> at Eclipse, the Bugzilla login cookie is bound to 'bugs.eclipse.org' so all I
> will need is something like bugsattachments.eclipse.org right?

  That's right. :-)

(In reply to comment #206)
> Red Hat IT suggested using HttpOnly cookie attributes which would
> help this issue.

  As LpSolit pointed out, we already do this.

> They suggest that properly sanitizing attachments is a much better route.

  That will never happen unless you devise some Bugzilla extension that does what you need. "Properly sanitizing" is an impossible thing to do when you need to have HTML attachments that work and different requirements at different organizations.

(In reply to comment #212)
> One way to fix the problem for those who aren't going to add an alternate host
> for attachments is to add a user pref "If the attachment is an HTML file...":

  This is a discussion that we should have in a separate bug.

Comment 216

[Frédéric Buclin]

(In reply to comment #215)
>   This is a discussion that we should have in a separate bug.

Actually, I'm interested to file a separate bug only if there is interest in this user pref.

Comment 217

[Marc Schumann [:Wurblzap]]

(In reply to comment #216)
> Actually, I'm interested to file a separate bug only if there is interest in
> this user pref.

Imho, this can be decided in the bug (which then ends up as either FIXED or WONTFIX), but ok, here's my vote to have the pref. I'd love to turn the pref to option (2) on b.m.o for myself, while browser devs will want to keep theirs at (1).

Comment 218

[Max Kanat-Alexander]

(In reply to comment #216)
> Actually, I'm interested to file a separate bug only if there is interest in
> this user pref.

  I've filed bug 472206, any further discussion about the user preference should go there.

Comment 219

[Adam Barth]

(In reply to comment #218)
>   I've filed bug 472206, any further discussion about the user preference
> should go there.

"You are not authorized to access bug #472206."  :(

Comment 220

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

How would a user ever know it is safe to flip the pref from the default value? It seems to me like there are three possible situations:

1. the bugzilla install is configured to use separate hostnames for attachments
2. accounts to the bugzilla install is only given to people trusted not to
   attach evil attachments
3. none of the above

In the two first scenarios filtering would not need to be enabled. In the last it would.
In neither scenario I can think of any situation where it would be desirable for a user to change that.

Comment 221

[Frédéric Buclin]

I replied in bug 472206. If some others want to be CC'ed to the other bug about the user pref, ping me on IRC (nick: LpSolit). This will avoid the "me too" in comments.

Comment 222

[Dave Miller [:justdave]]

Adding lots of preferences makes for UI bloat and only confuses users.  I like Jonas' idea in comment 220 to keep it simple.

We have two options: one is to filter everything, and the other is to use an alternate hostname.  It's really that simple, no additional preference needed.  If attach_urlbase (or whatever we called it) isn't set, then we filter everything.  If it is, then we don't.  It doesn't need to be any more complicated than that.

Comment 223

[Frédéric Buclin]

(In reply to comment #222)
> If attach_urlbase (or whatever we called it) isn't set, then we filter
> everything.  If it is, then we don't.  It doesn't need to be any more
> complicated than that.

I still think that's a bad approach. But let's follow this discussion in the other bug.

Comment 224

[Dave Miller [:justdave]]

Just so everyone knows (even if you didn't get CCed on the other bug yet) I did flag it as blocking 3.2.1, so we'll do *something* before this gets released for the people who can't set up the alternate domain name thing, it's not going to get pushed off.

Comment 225

[georgi - hopefully not receiving bugspam]

>1. If cookies are set HttpOnly client side scripts will not have access to
>them, only the server.

i am not sure this is effective.

1. is HTTP TRACE method trick for getting the cookies fixed?
2. one may not need reading the cookies. it may be enough to script an admin session (cookies get sent to the server) and elevate privileges via form filling.

Comment 226

[georgi - hopefully not receiving bugspam]

>In this case, don't view attachments. Download them! Right-click on the
>attachment link and choose "Save as...".

...and the script steals the content of the directory you saved as, usually Desktop ;)

Comment 227

[Denis Roy]

(In reply to comment #222)
> If attach_urlbase (or whatever we called it) isn't set, then we filter
> everything.  If it is, then we don't.  It doesn't need to be any more
> complicated than that.

+1!!  And, with Jonas' idea in bug 472206 comment 6 to add navigation to an unfiltered attachment makes Bugzilla simple to use and maintain, secure, and doesn't cut any functionality.

Comment 228

[David Lawrence [:dkl]]

(In reply to comment #222)
> Adding lots of preferences makes for UI bloat and only confuses users.  I like
> Jonas' idea in comment 220 to keep it simple.
> 
> We have two options: one is to filter everything, and the other is to use an
> alternate hostname.  It's really that simple, no additional preference needed. 
> If attach_urlbase (or whatever we called it) isn't set, then we filter
> everything.  If it is, then we don't.  It doesn't need to be any more
> complicated than that.

+1 as well.

Dave

Comment 229

[Gervase Markham [:gerv]]

+1 too; and that seems to be what's happening in bug 472206. It's a separate pref there, rather than tied to the attach_urlbase pref, but the effect is mostly the same. The current patch just permits you to configure Bugzilla insecurely, whereas Dave's scheme in comment #222 doesn't.

Gerv

Comment 230

[Marc Schumann [:Wurblzap]]

(In reply to comment #203)
> 2. I saw some weird stuff from a spammer about 2 weeks ago that smells very
> badly of this attack - at a glance it looked like just a pharmaceuticals spam
> but there was some weird JavaScript. I just nuked it at the time as spam, now
> I'm kicking myself that I didn't keep the attachment.

This sounds like http://bugzilla.ganderbay.net/attachment.cgi?id=21&action=edit. I modified the content type from text/html to text/plain so that it isn't active any more.

Comment 231

[Frédéric Buclin]

tip:

Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.151; previous revision: 1.150
done
Checking in Bugzilla/CGI.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/CGI.pm,v  <--  CGI.pm
new revision: 1.43; previous revision: 1.42
done
Checking in Bugzilla/Util.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Util.pm,v  <--  Util.pm
new revision: 1.83; previous revision: 1.82
done
Checking in Bugzilla/Config/Attachment.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/Attachment.pm,v  <--  Attachment.pm
new revision: 1.5; previous revision: 1.4
done
Checking in template/en/default/admin/params/attachment.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/params/attachment.html.tmpl,v  <--  attachment.html.tmpl
new revision: 1.6; previous revision: 1.5
done
Checking in template/en/default/attachment/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.55; previous revision: 1.54
done


3.2:

Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.144.2.3; previous revision: 1.144.2.2
done
Checking in Bugzilla/CGI.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/CGI.pm,v  <--  CGI.pm
new revision: 1.36.2.5; previous revision: 1.36.2.4
done
Checking in Bugzilla/Util.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Util.pm,v  <--  Util.pm
new revision: 1.69.2.8; previous revision: 1.69.2.7
done
Checking in Bugzilla/Config/Attachment.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/Attachment.pm,v  <--  Attachment.pm
new revision: 1.3.4.1; previous revision: 1.3
done
Checking in template/en/default/admin/params/attachment.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/params/attachment.html.tmpl,v  <--  attachment.html.tmpl
new revision: 1.4.2.1; previous revision: 1.4
done
Checking in template/en/default/attachment/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.51.2.1; previous revision: 1.51
done


3.0.6:

Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.126.2.2; previous revision: 1.126.2.1
done
Checking in Bugzilla/CGI.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/CGI.pm,v  <--  CGI.pm
new revision: 1.31.2.2; previous revision: 1.31.2.1
done
Checking in Bugzilla/Util.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Util.pm,v  <--  Util.pm
new revision: 1.56.2.2; previous revision: 1.56.2.1
done
Checking in Bugzilla/Config/Attachment.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/Attachment.pm,v  <--  Attachment.pm
new revision: 1.3.2.1; previous revision: 1.3
done
Checking in template/en/default/admin/params/attachment.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/params/attachment.html.tmpl,v  <--  attachment.html.tmpl
new revision: 1.3.2.1; previous revision: 1.3
done
Checking in template/en/default/attachment/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.41.2.5; previous revision: 1.41.2.4
done

Comment 232

[Max Kanat-Alexander]

Removing this bug from the security group, as the Security Advisory was sent (bug 468249)

Comment 233

[Vitaly Fedrushkov]

+  "Remember, though, that all those possible domain names " _
+  "(such as <tt>1234.your.domain.com</tt>) must point to " _

example.com seems more appropriate, per RFC 2606.

Comment 235

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/bugzilla/qa/4.2/
modified t/test_security.t
Committed revision 209.

Committing to: bzr+ssh://[e-mail]/bugzilla/qa/4.0/
modified t/test_security.t
Committed revision 198.

Committing to: bzr+ssh://[e-mail]/bugzilla/qa/3.6/
modified t/test_security.t
Committed revision 156.

Comment 240

[Manish Goregaokar [:manishearth]]

Has this been fixed? The attachment in comment 1 still works for this site.

Comment 241

[Dave Miller [:justdave]]

(In reply to Manish Goregaokar [:manishearth] from comment #240)
> Has this been fixed? The attachment in comment 1 still works for this site.

The attachment still pops up an alert, but the content of the alert no longer contains any sensitive info, so yes, it's fixed.