Closed
Bug 262116
Opened 20 years ago
Closed 17 years ago
mixed http/https warnings after enabling browser.cache.disk_cache_ssl
Categories
(Core :: Networking: Cache, defect, P4)
Core
Networking: Cache
Tracking
()
RESOLVED
FIXED
People
(Reporter: glob, Assigned: Biesinger)
References
Details
Attachments
(2 files, 4 obsolete files)
24.91 KB,
patch
|
KaiE
:
review+
|
Details | Diff | Splinter Review |
21.65 KB,
patch
|
Details | Diff | Splinter Review |
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
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.
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
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
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).
Updated•18 years ago
|
OS: Windows XP → All
Hardware: PC → All
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•18 years ago
|
||
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.
Assignee | ||
Updated•17 years ago
|
Assignee: nobody → cbiesinger
Assignee | ||
Comment 8•17 years ago
|
||
Attachment #290412 -
Flags: superreview?
Attachment #290412 -
Flags: review?
Assignee | ||
Updated•17 years ago
|
Attachment #290412 -
Flags: superreview?(darin.moz)
Attachment #290412 -
Flags: superreview?
Attachment #290412 -
Flags: review?(darin.moz)
Attachment #290412 -
Flags: review?
Assignee | ||
Comment 9•17 years ago
|
||
Includes the patch for bug 405470. Requires the patch for bug 405481.
Attachment #290413 -
Flags: superreview?(darin.moz)
Attachment #290413 -
Flags: review?(kengert)
Assignee | ||
Comment 10•17 years ago
|
||
requesting blocking, since this blocks bug 345181 (which is already a blocker)
Flags: blocking1.9?
Comment 11•17 years ago
|
||
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
Attachment #290412 -
Flags: review?(darin.moz) → review+
Updated•17 years ago
|
Attachment #290412 -
Flags: superreview?(darin.moz) → superreview+
Comment 12•17 years ago
|
||
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
Assignee | ||
Comment 13•17 years ago
|
||
(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.
Assignee | ||
Comment 14•17 years ago
|
||
Attachment #290412 -
Attachment is obsolete: true
Assignee | ||
Comment 15•17 years ago
|
||
transferring sr=darin
Attachment #290413 -
Attachment is obsolete: true
Attachment #290549 -
Flags: superreview+
Attachment #290549 -
Flags: review?(kengert)
Attachment #290413 -
Flags: superreview?(darin.moz)
Attachment #290413 -
Flags: review?(kengert)
Comment 16•17 years ago
|
||
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.
Updated•17 years ago
|
Flags: wanted1.9+
Flags: blocking1.9?
Flags: blocking1.9-
Updated•17 years ago
|
Flags: wanted1.9+
Flags: blocking1.9-
Flags: blocking1.9+
Priority: -- → P4
Comment 17•17 years ago
|
||
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...)
Attachment #290549 -
Flags: review?(kengert) → review+
Assignee | ||
Comment 18•17 years ago
|
||
Attachment #290549 -
Attachment is obsolete: true
Assignee | ||
Comment 19•17 years ago
|
||
(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.
Assignee | ||
Comment 20•17 years ago
|
||
Attachment #290544 -
Attachment is obsolete: true
Updated•17 years ago
|
Attachment #290739 -
Flags: review+
Assignee | ||
Comment 21•17 years ago
|
||
checked in
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
![]() |
||
Updated•6 years ago
|
Depends on: CVE-2018-12385
You need to log in
before you can comment on or make changes to this bug.
Description
•