300613 - insecure page can be made to appear secure

Status: VERIFIED FIXED

(Core :: Security, defect)

Description

[Matt Cooley]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.8) Gecko/20050511 Firefox/1.0.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.8) Gecko/20050511 Firefox/1.0.4

If a non-SSL site redirects a user to an SSL site which responds with a
certificate but doesn't respond with content (such as apache-ssl with broken
gcache), Firefox will keep the non-SSL content on the browser page, but add a
lock. This gives the appearance that an insecure page can be trusted. Clicking
on the lock causes Firefox to state that the non-SSL site supports
authentication and is verified by the CA of the SSL site.

Reproducible: Always

Steps to Reproduce:
1. Setup an SSL server (tested on Debian) using apache-ssl. In httpd.conf, add
the line: 
SSLCacheServerPort 1234
Run apache-ssl and kill the gcache process. Connecting to the site using curl or
openssl should show the site answering and neogiating with a certificate, but
the connection will close immediately afterward.

2. Put up a page on a secondary web server (non-SSL) with the following line:
<META HTTP-EQUIV=Refresh CONTENT="0; URL=https://apache-sslserver">
where apache-sslserver is the address to the SSL server setup in step 1.
Add other content to the page (i.e. "This resides on a non-SSL server")


3. Open Firefox 1.0.4 on Windows XP and go to the address
"http://nonsslserver/page" where nonsslserver is the address of the secondary
server and specify the page created in step 2.

Actual Results:  
Observe that a lock will appear in the browser but the address and content of
the site will be that of the non-SSL server. Clicking on the lock will show that
the non-SSL site supports authentication and the identity has been verified by
the CA that signed the certificate of the SSL site.

Expected Results:  
Return an error and redraw the browser window with no indication that the
current site is secure.

The bug can only be reproduced once before the browser has to be closed and
restarted. Additional attempts to reproduce the behavior without first
restarting the browser result in an error popup, "the connection has terminated
unexpectedly".

Comment 1

[Kai Engert (:KaiE:)]

Confirming bug, I see it with Mozilla Seamonkey 1.7.8 on Linux, too.

Comment 2

[Kai Engert (:KaiE:)]

Matt, I see a worse behaviour. You said, it only works once for you in a browser
session. The Linux version I'm playing with shows your bug every time.

Comment 3

[Daniel Veditz [:dveditz]]

Man, we can't seem to keep this thing fixed. This is like bug 238566 or bug
196827 or several others lock icon issues.

Comment 4

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Darin: can you help us figure out how we regressed this?

Bob: can you help us narrow down the regression window for this on the trunk or
branch?

Comment 5

[Bob Clary [:bc] (inactive)]

(In reply to comment #4)

> Bob: can you help us narrow down the regression window for this on the trunk or
> branch?

I'll see what I can do about getting a test environment then look for the
regression.

Comment 6

[Kai Engert (:KaiE:)]

I just tried the testcase with Mozilla 1.7.10+ on Linux, and I no longer see the
bug.

Comment 7

[Asa Dotzler [:asa]]

any progress on finding what broke and when?

Comment 8

[Darin Fisher]

Nope.  I have not had a chance to investigate this bug at all.  Help would be
appreciated.

Comment 9

[Asa Dotzler [:asa]]

Dveditz, we need your help here. Can you investigate and help us come up with a fix?

Comment 10

[Kai Engert (:KaiE:)]

Matt, are you able to make your test site working again?

It seems your site at https port 443 is now running a SSH server.

With the current configuration, whatever browser I use, Firefox 1.0.4, 1.0.5,
Mozilla 1.7.12, 1.8 beta or head of 1.9, I am not able to reproduce the bug.

Comment 11

[Kai Engert (:KaiE:)]

I spent 30 minutes trying to set up a server that could allow me to reproduce
the bug, but I'm not able to. The option you mention (SSLCacheServerPort 1234)
seems to be supported by very old versions of Apache only, I installed a version
1.3.23 on my machine, it accepted the config option with a warning, I have SSL
running on port 443, but I do not have a gcache process???

Comment 12

[Matt Cooley]

I reset the test case, so give it another try
http://www.0x90.org/~txs/firefox
(changed server from hackmatt to mattcooley)

Also, regarding the change with apache-ssl 1.3.33 (this is the Debian package, 
other installs may vary)
in httpd.conf, make sure the following lines are as such:

# Set the global cache server port number, or path. If it is a path, a Unix
# domain socket is used. If a number, a TCP socket.
SSLCacheServerPort 1234
#SSLCacheServerPort /var/run/gcache_port

Restart apache-ssl and gcache should be listening on port 1234

Comment 13

[Kai Engert (:KaiE:)]

Attachment: Logfile

Logfile, created using NSPR_LOG_MODULES=pipnss:5,nsSecureBrowserUI:5

What I can see is:

- the HTTP channel code complains "No response head", I guess that means, no
data is being received

- nevertheless, OnStateChange gets called with a progress notification of
nsIWebProgressListener::STATE_TRANSFERRING. The nsSecureBrowserUI uses this
event to decide "document data has been received"

- in addition, OnLocationChange gets called, and because of the assumption of
data having been transfered, the UI gets updated.

Comment 14

[Kai Engert (:KaiE:)]

Thanks Matt, your test case works again, and I am able to reproduce, with all
recent versions.

Comment 15

[Kai Engert (:KaiE:)]

I have a patch, it fixes the problem on trunk and on the 1.7 and 1.8 branches.
With the patch applied, when connecting to the testcase, the URL bar remains at
the URL of the displayed content, and the lock icon remains in insecure state.

Darin, in this scenario, nsHttpChannel::OnStartRequest issues a warning
  NS_WARNING("No response head in OnStartRequest");

The code goes on to retry the connection, based on some error codes.

When not retrying, the code goes on to call CallOnStartRequest, which seems to
be responsible for sending out the STATE_TRANSFERRING event.

My suggestion is, after having decided not to retry, we should retest whether
headers have been received. If not, we should stop.

I implemented the stop using a simple
  return NS_ERROR_FAILURE;

This fixes the problem, but I can't tell if it will introduce regressions. Do
you think it might?

Comment 16

[Kai Engert (:KaiE:)]

Attachment: Patch v1

Comment 17

[Darin Fisher]

This seems wrong to me.  If a channel is in a failed state, then it still has to
call OnStartRequest and OnStopRequest.

Comment 18

[Darin Fisher]

Comment on attachment 199476 [details] [diff] [review]
Patch v1

This can't be the right fix.

Comment 19

[Kai Engert (:KaiE:)]

Darin, do you think the STATE_TRANSFERRING event should be sent out, even if no
data is being transferred?

In that case, the security tracking needs to become more complex and filter out
those events that carry a failure state.


However, I checked member
  nsresult nsIRequest::status
of the Request that is sent to OnStateChange, 
and it is 0 / NS_OK,
despite nsSSLIOLayerConnect having returned a failure.

Parameter aStatus passed to OnStateChange is NS_OK, too.

Is it a bug that "status" does not indicate the error condition? Or is there
another place to test the Request/Channel for a failure state?

Comment 20

[Darin Fisher]

> However, I checked member
>   nsresult nsIRequest::status
> of the Request that is sent to OnStateChange, 
> and it is 0 / NS_OK,
> despite nsSSLIOLayerConnect having returned a failure.
> 
> Parameter aStatus passed to OnStateChange is NS_OK, too.
> 
> Is it a bug that "status" does not indicate the error condition? Or is there
> another place to test the Request/Channel for a failure state?

Yes, that sounds like a bug.  If NSS and PSM think that there was a data
transfer error, then that error code should be reflected via nsIRequest::status
and the status parameter passed to onStateChange.  Then, the secure browser ui
would do the right thing.

Comment 21

[Asa Dotzler [:asa]]

kai, what needs to happen here? We're running very short on time for the next
release.

Comment 22

[Kai Engert (:KaiE:)]

Darin, I was wrong, I said I see an error returned, but it the WOULD_BLOCK error
code. Actually, no error is happening. It's just that SSL handshaking is working
fine, but no data is being transfered at all. This has the effect that no error
messages are being produced.

So, my current opinion is: The document loading could detect that no data has
been transfered, that we have received any secure data, and the notifications
about transfered data and location change should be suppressed.

My patch was a suggestion to suppress it based on having http headers received.
But Darin did not like that patch.

Unfortunately I don't understand the document loader internals well enough to
come up with another suggestion at this point.

Darin, with this new information, that no error message at the SSL level is
being produced, do you have an idea how to change the document loader?

Comment 23

[Kai Engert (:KaiE:)]

Matt, I'm having trouble again with your 2-part test case.
The URL you gave above gives a "forbidden" error message.

However, the "meat" of your test case currently works. The new intstructions to
reproduce are:

Go to any insecure site.
Then open https://www.mattcooley.com

Comment 24

[Kai Engert (:KaiE:)]

Nelson, FYI.
I'm attaching a curl and ssltap output of the bug scenario.

In short, SSL handshake seems to work fine, but directly afterwards the
connection is closed, no application data is being received by the client.
However, this partial success triggers the application to update the UI to the
secure state.

In my opinion, this is an application bug, and we should come up with a fix,
either in the document loading logic or with PSM.

But for completeness I would like to ask:

Nelson, do you think the SSL communication should produce an error message in
this special scenario? If I understand Darin correctly, an error message
produced during SSL read/write communication, passed up via PSM to the document
loader, could have the desired effect, to signal the problem.

Comment 25

[Kai Engert (:KaiE:)]

(sorry I accidentially clicked reassign before clicking commit)

Comment 26

[Kai Engert (:KaiE:)]

Attachment: curl -v --insecure https://127.0.0.1:1443

Comment 27

[Kai Engert (:KaiE:)]

Attachment: ./ssltap -h -s -p 1443 www.mattcooley.com:443

Comment 28

[Kai Engert (:KaiE:)]

Attachment: Patch v2

Here is another suggestion.

I looked at the SSL debugging tool ssltap.
When that tool detects a zero byte return value from PR_Read or PR_Write, it
reports that as an EOF error.

A SSL socket within Mozilla uses layered I/O code, where PSM provides an
intermediate layer, before calling I/O functions in the SSL library (NSS).

When PSM receives a zero byte return value from SSL read/write, no special
action is done. It simply passes up that zero byte value, with no indication of
an error. I also checked PR_GetError, but it's zero, the SSL library did not
set an error code.

Here is a patch, based on ssltap's behaviour. With that patch applied, if a
zero byte value is returned by the SSL library, PSM I/O layer code sets error
code PR_END_OF_FILE_ERROR and returns -1 as the byte count of the read/write
function.

This change makes Mozilla behave better than currently. In Matt's test case,
Mozilla will bring up an error message "the connection has terminated
unexpectedly", which is indeed what happens. And the lock remains in the
insecure state.

I tested this patch with other (good) SSL sites, and on first sight, it still
seems to work. But again, I'm unsure whether this patch has negative side
effects.

Comment 29

[Kai Engert (:KaiE:)]

Comment on attachment 200043 [details] [diff] [review]
Patch v2

Darin, do you have thoughts on possible side effects?

I wonder if a zero byte return value from read/write can ever be an allowed
condition, where we should not abort (like this patch implements).

Probably Nelson would have thoughts, too, but I emailed with him today, and he
will be away for a couple of days.

Comment 30

[Kai Engert (:KaiE:)]

NELSON sent the following comment to me by email:


> I looked at the SSL debugging tool ssltap.
> When that tool detects a zero byte return value from PR_Read or PR_Write, it
> reports that as an EOF error.

I would not say that SSLTAP reports every zero length return as an EOF *error*.
I would say that SSLTAP reports every zero length return *from read* as EOF.
That is, SSLTAP reports all EOFs, so that the human reading the output can
tell when the remote client or server closed its socket.  This does not
always indicate an error.  The receipt of EOF is usually not considered to be
an error, which is why it is reported with zero length and not an error code.

> A SSL socket within Mozilla uses layered I/O code, where PSM provides an
> intermediate layer, before calling I/O functions in the SSL library (NSS).
> 
> When PSM receives a zero byte return value from SSL read/write, no special
> action is done. It simply passes up that zero byte value, with no indication of
> an error. I also checked PR_GetError, but it's zero, the SSL library did not
> set an error code.

So, the question is: when EOF is received, when should it be reported as a
mere ordinary non-erroneous EOF, and when should it be reported as an error?
I believe the answer is that it is an error when a higher-level protocol
receives an ordinary EOF indication from the layer below it, and determines
that the connection should never be terminated while the protocol is in the
then-current state.

SSL returns the EOF error if/when a normal EOF is received from the TCP socket
below, while the first SSL handshake is in progress and no SSL error alert
has been received.  SSL determines that the receipt of an EOF in the middle
of a handshake, without receiving an error alert record, indicates that the
remote party terminated the handshake improperly.  So, in effect, the SSL
library turns the receipt of an ordinary TCP EOF during the initial
handshake into an SSL protocol EOF error.

I think that the protocols above the (SSL or non-SSL) socket should similarly
notice when an ordinary EOF is received from the layer below, and should then
determine for themselves whether the termination of the underlying connection
is acceptable or is a violation of their protocol.  For example, if an http
client (with or without SSL) sends an http request and gets back EOF instead
of an http response, it is probably appropriate for the http client to
change that ordinary EOF into an EOF error.  But I think this must be done
by the upper level (http) protocol code, because only that protocol knows
when an EOF is an error, and when it is not.

> Here is a patch, based on ssltap's behaviour. With that patch applied, if a
> zero byte value is returned by the SSL library, PSM I/O layer code sets error
> code PR_END_OF_FILE_ERROR and returns -1 as the byte count of the read/write
> function.


> This change makes Mozilla behave better than currently. In Matt's test case,
> Mozilla will bring up an error message "the connection has terminated
> unexpectedly", which is indeed what happens. And the lock remains in the
> insecure state.
> 
> I tested this patch with other (good) SSL sites, and on first sight, it still
> seems to work. But again, I'm unsure whether this patch has negative side
> effects.

It appears to me that this patch turns every zero return value from read
*or write* on an SSL socket into an error.  This means that:

1) every EOF will now be an error.  There will be no more ordinary EOFs.

2) zero-length writes (that is, write calls where the length requested to
be written is zero) will now always return errors.  (Without this patch, a
zero length write will either return 0 for success or -1 for an error.)
Zero length writes are used to drive handshakes on non-blocking sockets,
and so they should not experience errors when the return value is also zero.

In short, your patch makes PSM turn ordinary EOFs into EOF errors without
knowledge of the higher layer's protocol state.  I think the right way to
deal with this is for the higher layer protocols to detect this invalid
EOF condition and turn it into an error themselves.

Comment 31

[Kai Engert (:KaiE:)]

Comment on attachment 200043 [details] [diff] [review]
Patch v2

Based on Nelson's comment I'm obsoleting this patch.

Comment 32

[Kai Engert (:KaiE:)]

I agree with Nelson's comment.

The HTTP code should make the decision, that not receiving a HTTP header
response is a failure, and should produce the error, and thereby suppress the UI
update.

That is the strategy that was behind patch v1. Darin, as you do not like that
patch, could you come up with a better way to handle this error on the HTTP
protocol level? Or do you have advice that could help us produce the fix to the
HTTP code? Thanks.

Comment 33

[Darin Fisher]

Remember that we support HTTP/0.9 responses, which consist of "the document"
followed by a closed connection.  Is an empty document a valid HTTP/0.9
response?  Our code currently assumes that it is.  Is it?  It seems like it
should be valid to me.  Since I'm very busy with other FF 1.5 blockers, can
someone who can reproduce this bug please take the time to generate a NSPR log
for me?

  NSPR_LOG_MODULES=nsSocketTransport:5,nsHttp:5

That would be very helpful as I could see what is happening inside Necko.  Thanks!

Comment 34

[Kai Engert (:KaiE:)]

Attachment: Logfile NSPR_LOG_MODULES=nsSocketTransport:5,nsHttp:5,pipnss:5

Darin, this is a full log of starting Mozilla, loading the mozilla.org
homepage, then accessing https://www.mattcooley.com

You can search for 9bf3280 for some trace lines from SSL/PSM, that surround the
interesting parts of the file.

Comment 35

[Daniel Veditz [:dveditz]]

Attachment: nsSocketTransport/nsHttp log

Comment 36

[Daniel Veditz [:dveditz]]

We've been talking about how to keep the secure connection information from
showing, but the other way to fix this is to clear the old content display
instead. The latter is arguably closer to the reality of the situation.

Comment 37

[Darin Fisher]

Yes, I think the bug here is that nsHttpTransaction::ProcessData is never
called.  That causes us to end up in a partial state, which confuses things
downstream.  We should treat this response for what it is: an empty document.

Comment 38

[Darin Fisher]

I will develop a fix for this.

Comment 39

[Darin Fisher]

Attachment: v1 patch

This simple patch fixes the bug.  The fix is to treat an empty response as a
HTTP/0.9 response.  Here, I am just extending the check that is already done to
handle unterminated header sections.  Instead of calling ParseLineSegment, I
use ParseHead, which preserves the old codepath in the case where mLineBuf is
not empty.  I think this is a fairly safe fix to this bug.

Comment 40

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 200274 [details] [diff] [review]
v1 patch

r=bzbarsky

Comment 41

[Daniel Veditz [:dveditz]]

Comment on attachment 200274 [details] [diff] [review]
v1 patch

sr=dveditz

Comment 42

[Darin Fisher]

fixed-on-trunk

Comment 43

[Tracy Walker [:tracy]]

Verified with Linux and Windows trunk builds from 1021

Comment 44

[Bob Clary [:bc] (inactive)]

to second Tracy,

today's winxp branch: http://www.0x90.org/~txs/firefox, ok the cert warnings,
the url bar does not change and the lock icon appears.

today's winxp trunk: the url switches to https://www.mattcooley.com/

verified on winxp/trunk.

Comment 45

[Darin Fisher]

fixed1.8

Comment 46

[Daniel Veditz [:dveditz]]

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