128213 - Instead of an order form page I get an ecommerce security warning

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Richard Thomas]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Win98; en-US; rv:0.9.8+) Gecko/20020227
BuildID:    2002022703

I am wanting to order a particular product from this website, when I go to
checkout I get a page giving me a security warning, instead of the order form
that I get with IE.

Reproducible: Always
Steps to Reproduce:
1.Go to the URL
2.Click on the green Overseas "Add to Cart" button on the right hand side of the
page - you will get a page showing the contents of your shopping cart, 1 item of
product.
3.Click on the Order Checkout at the top right of the page - the security
warning page appears


Actual Results:  I get a page about a security warning

Expected Results:  I should have got an order form to fill in my details

Comment 1

[Darin Fisher]

-> badami for investigation

Comment 2

[Vinay Badami]

Looks like we are being pretty strict wrt setting referrer headers especially
wrt secure servers.
Commenting out the following lines solves the problem
            /*******
            if (nsCRT::strcasecmp(referrerHost, host) != 0) {
                return NS_OK;
            }
            ********/

I do not see in the spec anything saying that the hosts should match to set a
refererrer on secure servers. It indicates that we should never set a referrrer
while going to a non-secure server from a secure server.

why are we doing this ?

Comment 3

[Darin Fisher]

ah, we are doing this to prevent sending a Referer header from one secure site
to another secure site.  from the point of view of the browser, there is no way
of knowing that first secure site is part of the same "trust domain" as the
second secure site.  therefore, we assume that the second secure site should not
see an URL from the first secure site.  this is a security measure that isn't
implemented by other browsers but is implemented by mozilla as an extra measure
to protect user privacy.

moreover, there is a user preference to disable sending all Referer headers, as
recommended by RFC2616.  in this case, i think the site should be evangelized.

cc'ing some folks who may have an opinion on this.

Comment 4

[Doug Turner (:dougt)]

> from the point of view of the browser, there is no way
of knowing that first secure site is part of the same "trust domain" as the
second secure site. 

That is not totally correct IIRC.  We should be comparing the cert of the dest
site the the cert of the referer site.  The code has changed alot since I was
hacking on it, but PSM (NSS) does provide a way to verify that two certs match. 
If they don't, I believe that this is calls for user dialog.

Comment 5

[Darin Fisher]

hmm... would be nice to do that.

Comment 6

[Bradley Baetz (:bbaetz)]

We'vre been through this before. IE doesn't block https->https, anbd the http
rfc doesn't require it either.

Comment 7

[Vinay Badami]

1. We are doing something outsie of the spec that may break a few sites. So my
take  is that this behavior not be the default. 
2. We put in a patch that does implements 1 so that sites will work and get that
in. We then track this bug wrt Dougs comments and get a proper fix in if
possible by Moz1.0 else Moz1.01.

Comment 8

[Vinay Badami]

Attachment: patch as per my previous comment

Prevent referrer header from siteA to siteB, both sites being secure, if user
so desires.

Comment 9

[Vinay Badami]

Attachment: prevent referrer only if user so desires

prevent referrer only if user so desires  from Site A --> Site B bot being
secure.

Previous patch was invalid and wrong.

Comment 10

[Mitchell Stoltz (not reading bugmail)]

We aren't introducing a user dialog for this, are we? Please don't add a dialog.
I'd prefer it if we left the current behavior alone (not sending referrers
cross-host), at least as the default. We could add a pref to turn it off.

Comment 11

[Vinay Badami]

Attachment: YAP- yet another patch

Be secure by default. Allow user to override thru a preference.
Added logging to help in debugging in future.

Comment 12

[Darin Fisher]

problem: the referrer level in nsIHttpChannel is something the client of a
nsIHttpChannel uses to specify the kind of referrer being set.  it doesn't make
sense for the client of a nsIHttpChannel to say that a
"HTTPS_ALLOW_NON_MATCHING_HOSTS" referrer is being set... usually, they say that
this is a link click (REFERRER_LINK_CLICK), or this is an image
(REFERRER_INLINES), etc.  REFERRER_HTTPS_ALLOW_NON_MATCHING_HOSTS is not a type
of referrer.

moreover, it seems like the setting you're after is more of a preferences
setting... not a referrer type.  so i think it shouldn't be exposed on
nsIHttpChannel but should simply be mentioned in all.js... e.g.:

pref("network.http.sendRefererHeader", 2);
  // 0=don't send any
  // 1=send only on clicks
  // 2=send on image requests as well
  // 3=send from secure site A to secure site B

but this is bad because then you end up referencing 3 in the code... of course
you could introduce another #define or whatever... but something just doesn't
feel right about that, cuz it isn't symmetric w/ the other values.  in fact,
suppose we come up with yet another preference for when to send referrers...
would we add it before or after this HTTPS cross-host one?

it almost seems like we need another preference for this bug.  a boolean
"network.http.sendSecureReferrerCrossHost" pref might do... of course that's
pretty wordy.  suggestions?

Comment 13

[Vinay Badami]

Attachment: using a separate pref

Comment 14

[Darin Fisher]

how about picking one name for this attribute and make it universal?  in your
patch you use "mSecureXSiteReferrer" in one place... and i like that because it
is short... so how about this:

  "network.http.sendSecureXSiteReferrer"
  nsHttpHandler::SendSecureXSiteReferrer()
  nsHttpHandler::mSendSecureXSiteReferrer;

sound good?

Comment 15

[Vinay Badami]

Attachment: with darins comments

Comment 16

[Darin Fisher]

Comment on attachment 73517 [details] [diff] [review]
with darins comments


>+    PRPackedBool   mSendSecureXSiteReferrer; //default is false, if true allow referrer headers between secure non-matching hosts

nit:

please wrap this comment, or move it above the variable declaration, so
it doesn't extend so far beyond 80 chars.

sr=darin

Comment 17

[rpotts (gone)]

Comment on attachment 73517 [details] [diff] [review]
with darins comments

r=[e-mail]

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Two questions:
 * What's the point of making this a pref if there's no UI -- how will users
know to fix some broken site by twiddling the magic pref in their prefs.js?  Are
some implementations (e.g., Netscape commercial, some embedders) going to want
to set the default differently?  Or are you planning to add UI?
 * Aren't prefs supposed to have the default values listed in all.js (at the
very least, for documentation)?  (I have heard different answers to this
question, I admit...)

Comment 19

[Darin Fisher]

dbaron: good point... this pref should be documented in all.js.  also, i'd
imagine that we might one day want to add UI for the "send Referer header"
prefs.  at any rate, this patch is just the first step.

perhaps when i work on moving some of the networking prefs into the advanced
section of the preferences dialog, this preference can be added.

Comment 20

[Asa Dotzler [:asa]]

Comment on attachment 73517 [details] [diff] [review]
with darins comments

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 21

[Vinay Badami]

Fixed with checkin
D:\mozilla\netwerk\protocol\http\src>cvs commit
cvs commit: Examining .
? bak
Checking in nsHttpChannel.cpp;
/cvsroot/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp,v  <--  nsHttpChann
new revision: 1.98; previous revision: 1.97
done
Checking in nsHttpHandler.cpp;
/cvsroot/mozilla/netwerk/protocol/http/src/nsHttpHandler.cpp,v  <--  nsHttpHandl
new revision: 1.52; previous revision: 1.51
done
Checking in nsHttpHandler.h;
/cvsroot/mozilla/netwerk/protocol/http/src/nsHttpHandler.h,v  <--  nsHttpHandler
new revision: 1.24; previous revision: 1.23
done

Comment 22

[Jeff Thieleke]

Adding a new hidden, undocumented pref might not be the best way to fix this
problem...  

You might want to take a look at the work I have been doing with referrers in
bug #55477.  I have added a UI for configuring the current referrer settings, as
well as adding some additional levels of referrer control - including the
ability to send the referrer only to the same host.  

My patch is currently blocking https->https referrers, however, mainly because I
just didn't want to change the original piece of code because of potential
privacy problems.  But I think that removing the https->https check entirely
wouldn't be unreasonable, iff the user had more options on how and when to send
the referrer header.  The very best solution, as mentioned above, would be to
verify that the domains in the certificates match, then decide what to send as
the referrer.

Comment 23

[Philip White]

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

Comment 24

[Jesse Ruderman]

I don't understand how adding a hidden pref "fixed" this bug.  I proposed a
solution in bug 141641: send the hostname (but not the path) as the referrer if
the origin is https, regardless of the target.

Comment 25

[Darin Fisher]

jesse: right.. this was not a complete solution.  post 1.0, i'm planning to test
out the partial referrer solution.

Comment 26

[John Unruh]

Reopening.

Comment 27

[Darin Fisher]

ok, we actually allow cross site HTTPS referrers now by default, so this bug
should be fixed.