262116 - mixed http/https warnings after enabling browser.cache.disk_cache_ssl

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect, P4)

Description

[:glob ✱]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3) Gecko/20040926 Firefox/0.10
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3) Gecko/20040926 Firefox/0.10

after enabling browser.cache.disk_cache_ssl i get warnings that a secure site
contains mixed http and https objects.


Reproducible: Always
Steps to Reproduce:
1. enable browser.cache.disk_cache_ssl, enable security.warn_viewing_mixed 
2. visit https://bugzilla.mozilla.org/show_bug.cgi?id=260682
3. right click on banner image, view image
4. notice that while the url displays https, the addressbar is missing the
yellow background and lock image
5. click back, note the addressbar is still missing https indicators
6. click on the bug url link (Bug #: 260682 ..)
7. you should get the warn_viewing_mixed warning
8. reload page, you should get no warning and everything back to normal

Comment 1

[OstGote!]

Still there for Mozilla 1.7.11 and Seamonkey 1.1a trunk build 2005090105 on
WinXP. Tested with bugzilla pages, if you open a new tab with a Bugzilla page
you'll get the warning dialog.

Comment 2

[Darin Fisher]

-> default owner

Comment 3

[Gumaste]

Still exists in Firefox 2.0.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1) Gecko/20061010 Firefox/2.0

Comment 4

[Gumaste]

Still exists in Firefox 2.0.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1) Gecko/20061010 Firefox/2.0

Comment 5

[Marshall]

This still exists, and has suddenly become a major problem because of bug 30968 - https://bugzilla.mozilla.org/show_bug.cgi?id=309368

The default behavior of Firefox is now to give spurious encryption errors on any encrypted site. Since any properly-made shopping site will be encrypted, that means that Firefox will cause loss of sales to every e-commerce site visited by Firefox users. The percentage of lost sales for any given site is probably quite small, but added up the amount of money lost to e-commerce is probably a staggeringly large number. 

I should mention that the behavior of ignoring the no-cache directive mentioned in 309368 was not fixed either, so sites can't fix this by using no-cache. It's a browser-side only bug, and it's going to bite every shopping cart on the planet.

This has just gotta be fixed! Or else bug 309368 needs to be reversed (which wouldn't fix anything but it would get the problem hidden back under the rug where it belongs). 

Comment 6

[Marshall]

Okay - I made a dweebish error: Bug 309368 was not changed after all. That the option appeared to be on by default was actually the result of a misunderstanding on my part. However, there is a growing number of people with the option turned on due to a growing number of FF optimization tips sites suggesting turning it on. While these site aren't giving "official" advice, people are still following it. So it's a real-world effect and needs resolution through the fixing of this bug.

Comment 7

[Mike Kaply [:mkaply]]

From Darin Fisher - what would need to happen to fix this:

the challenge is to serialize SSL certificate info (the result of nsIChannel::securityInfo) to the disk cache so it can be surfaced again via nsICacheEntryDescriptor::securityInfo.  currently, we only do this in the memory cache b/c we can just hold onto the object w/o serializing. 

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: netwerk/ + xpcom/ part

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: PSM part

Includes the patch for bug 405470. Requires the patch for bug 405481.

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

requesting blocking, since this blocks bug 345181 (which is already a blocker)

Comment 11

[Darin Fisher]

Comment on attachment 290412 [details] [diff] [review]
netwerk/ + xpcom/ part

>Index: netwerk/base/src/nsBase64Encoder.cpp
...
>+nsresult
>+nsBase64Encoder::Finish(nsCSubstring& result)
>+{
>+  char* b64 = PL_Base64Encode(mData.get(), mData.Length(), nsnull);
>+  if (!b64)
>+    return NS_ERROR_OUT_OF_MEMORY;
>+
>+  result.Assign(b64);
>+  PR_Free(b64);
>+  mData.Truncate();

This .Truncate() call is just to free unneeded memory right?  Might be
good to add a comment explaining that.


>Index: netwerk/base/src/nsSerializationHelper.cpp

>+nsresult
>+NS_DeserializeObject(const nsCSubstring& str, nsISupports** obj)
>+{
>+  PRUint32 size = (str.Length() * 3) / 4;
>+  char* buf = PL_Base64Decode(str.BeginReading(), str.Length(), nsnull);
>+  if (!buf)
>+    return NS_ERROR_OUT_OF_MEMORY;

nit: you should add some comment about the 'size' calculation.
there should really be some macro in NSPR to compute that value :)


>Index: netwerk/base/src/nsSerializationHelper.h
...
>+ * Serialize an object to an ASCII string.
>+ */
>+nsresult NS_SerializeToString(nsISerializable* obj,
>+                              nsCSubstring& str);
>+
>+/**
>+ * Deserialize an object.
>+ */
>+nsresult NS_DeserializeObject(const nsCSubstring& str,
>+                              nsISupports** obj);
>+
>+#endif

It seems like there should be some unit tests for these functions :)



>Index: netwerk/cache/src/nsCacheEntry.h

>     nsresult GetSecurityInfo( nsISupports ** result);
>     void     SetSecurityInfo( nsISupports *  info) { mSecurityInfo = info; }
>+    nsISupports* GetSecurityInfo() { return mSecurityInfo; }

might be nice to name the new getter "SecurityInfo" without the Get
prefix to differentiate it from the getter that returns an out param.


>Index: netwerk/cache/src/nsDiskCacheEntry.cpp

>+    // Restore security info, if present
>+    const char* info = entry->GetMetaDataElement("security-info");
>+    if (info) {
>+      nsCOMPtr<nsISupports> infoobj;
>+      NS_DeserializeObject(nsDependentCString(info), getter_AddRefs(infoobj));
>+      entry->SetSecurityInfo(infoobj);

nit: 4 whitespace indent

nit: infoObj instead of infoobj?


>+    // Store security info, if it is serializable
>+    nsCOMPtr<nsISerializable> serializable =
>+        do_QueryInterface(entry->GetSecurityInfo());
>+    if (serializable) {
>+      nsCString info;
>+      NS_SerializeToString(serializable, info);
>+      entry->SetMetaDataElement("security-info", info.get());
>+    }

nit: 4 whitespace indent

Comment 12

[Darin Fisher]

Comment on attachment 290413 [details] [diff] [review]
PSM part

>Index: security/manager/ssl/src/nsNSSCallbacks.cpp
...
>     /* Set the SSL Status information */
>-    nsCOMPtr<nsSSLStatus> status;
>-    infoObject->GetSSLStatus(getter_AddRefs(status));
>+    nsRefPtr<nsSSLStatus> status;
>+    infoObject->GetSSLStatus(reinterpret_cast<nsISupports**>(
>+                               static_cast<void**>(getter_AddRefs(status))));

nit: since you have a pointer to the nsNSSSocketInfo object, perhaps it
would be nicer to just add a simple getter on nsNSSSocketInfo that
returns the nsSSLStatus pointer?  maybe nsNSSSocketInfo::SetSSLStatus
should even take a nsSSLStatus pointer instead of a nsISSLStatus pointer?

otherwise, sr=me

Comment 13

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #11)
> This .Truncate() call is just to free unneeded memory right?  Might be
> good to add a comment explaining that.

I was kind of thinking that someone might also want to reuse the encoder for another encoding... Finish() sounds to me like it should clear the state to allow that.

> >Index: netwerk/base/src/nsSerializationHelper.cpp
> nit: you should add some comment about the 'size' calculation.
> there should really be some macro in NSPR to compute that value :)

Yeah, I'll add that comment. The computation is from the comment in plbase64.h... Really, PL_Base64Decode ought to return the real count.

> >Index: netwerk/base/src/nsSerializationHelper.h
> It seems like there should be some unit tests for these functions :)

Yes, but I can't link to component libraries :/

> might be nice to name the new getter "SecurityInfo" without the Get
> prefix to differentiate it from the getter that returns an out param.

OK. In parts of the tree, a missing Get prefix indicates the method doesn't return null, but I guess we don't really do that in necko.

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: netwerk/ + xpcom/ part, v2

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: PSM part, v2

transferring sr=darin

Comment 16

[Wan-Teh Chang]

Comment on attachment 290544 [details] [diff] [review]
netwerk/ + xpcom/ part, v2

I only looked at nsBase64Encoder.{h,cpp}.  I'm surprised that
Mozilla doesn't yet have a base64 encoder class.  Perhaps it
has been calling PL_Base64Encode directly?

In nsBase64Encoder.h

>+ * A base64 encoder. Usage: Instantiate class, write to it using
>+ * nsIOutputStream methods, then call Finish() to get the base64-encoded data.

Since you didn't implement WriteFrom and WriteSegments, perhaps you
should just say "write to it using Write()".

>+    /// The data written to this stream. nsCString has can deal fine with

Remove "has".

In nsCacheEntry.h

>     nsresult GetSecurityInfo( nsISupports ** result);
>     void     SetSecurityInfo( nsISupports *  info) { mSecurityInfo = info; }
>+    nsISupports* SecurityInfo() { return mSecurityInfo; }

Why do you need two getters for mSecurityInfo?  Is it for convenience?
Can we remove the original getter?  Would be nice to list the two getters
together.

Comment 17

[Kai Engert [:KaiE:]]

Comment on attachment 290549 [details] [diff] [review]
PSM part, v2

Patch looks good to me, r=kengert, but please address the following.

+  {
+    "NSS IO Layer",
+    NS_NSSIOLAYER_CID,
+    nsnull,
+    nsNSSSocketInfoConstructor
   }

Could you please use something like

+  {
+    "NSS Socket Info",
+    NS_NSSSOCKETINFO_CID,
+    nsnull,


(I wish PSM wouldn't call its own I/O layer the NSS I/O layer, because both PSM and NSS use separate I/O layers...)

Comment 18

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: PSM part, v3

Comment 19

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #16)
> (From update of attachment 290544 [details] [diff] [review])
> I only looked at nsBase64Encoder.{h,cpp}.  I'm surprised that
> Mozilla doesn't yet have a base64 encoder class.  Perhaps it
> has been calling PL_Base64Encode directly?

Yes, it's been calling it directly.

> Why do you need two getters for mSecurityInfo?  Is it for convenience?
> Can we remove the original getter?  Would be nice to list the two getters
> together.

Good point, I'll remove GetSecurityInfo.

Comment 20

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: netwerk/ + xpcom/ part, v3

Comment 21

[Christian :Biesinger (don't email me, ping me on IRC)]

checked in