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

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

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.

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

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?

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

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

any progress on finding what broke and when?

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

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

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.

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

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

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.

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

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?

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

Comment on attachment 199476 [details] [diff] [review] Patch v1 This can't be the right fix.

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?

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

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

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?

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

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.

(sorry I accidentially clicked reassign before clicking commit)

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

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 on attachment 200043 [details] [diff] [review] Patch v2 Based on Nelson's comment I'm obsoleting this patch.

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.

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!

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.

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.

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.

I will develop a fix for this.

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 on attachment 200274 [details] [diff] [review] v1 patch r=bzbarsky

Comment on attachment 200274 [details] [diff] [review] v1 patch sr=dveditz

fixed-on-trunk

Verified with Linux and Windows trunk builds from 1021

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.

fixed1.8

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