Closed
Bug 343837
Opened 18 years ago
Closed 18 years ago
SSL2/weak ciphers disabled messages contain "(null)" instead of "Camino"
Categories
(Camino Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Camino1.5
People
(Reporter: alqahira, Assigned: stuart.morgan+bugzilla)
References
()
Details
(Keywords: fixed1.8.0.9, fixed1.8.1.1)
Attachments
(3 files, 1 obsolete file)
23.28 KB,
patch
|
Details | Diff | Splinter Review | |
23.11 KB,
patch
|
Details | Diff | Splinter Review | |
16.09 KB,
patch
|
froodian
:
review+
|
Details | Diff | Splinter Review |
Bug 236933 disables SSL2 and various weak ciphers under SSL3 and adds new error messages. Unfortunately, the message don't display properly in Camino. I'm pretty sure, although not 100% positive, that this issue is due to bug 302309 (just like we experienced with the error pages, where embedding apps don't ship with files that contain the codes those strings referenced, and where, in that case, the strings were genericized and overridden in Fx to say "Firefox" instead of "The browser"), but no one in the Core bug seems to care. It seems like we have a few options: 1) Get the Core to care and change to generic strings 2) Get bug 302309 fixed (unlikely for 1.8.1) 3) Override this dialogue using our existing own security dialogues component 4) Use mento's wonderful new embed-replacements foo and run a script that extracts pipnss.properties and replaces "%1$S" with "Camino" and repacks the changed file. Since Camino 1.1 will be the first release to ship with these ciphers/SSL2 disabled, people will be seeing these messages at a number of regionally-high profile sites for the first time, and the current appearance of the message is both ugly and very confusing--both bad on a security dialogue--so we need to do *something*.
Reporter | ||
Comment 3•18 years ago
|
||
This affects 1.0.x, too (see the dupe), for the same reasons; the old non-matching ciphers strings had one of those "branding" references, too.
Flags: camino1.0.3?
Summary: New SSL2/weak ciphers disabled messages contain "(null)" instead of "Camino" → SSL2/weak ciphers disabled messages contain "(null)" instead of "Camino"
Reporter | ||
Comment 4•18 years ago
|
||
For 1.1, mento thinks we should do option 4 above. We need to case-sensitively replace %1$S in pipnss.properties (there's a %1$s in the file, also). We could also go on keynames (SSL_Disabled, SSL2_Disabled, SSL_NoMatchingCiphers) and replace the whole string, but that would break when they rev those messages (the keynames change for l10n), and it looks like those messages might change again. For 1.0.x, the file is different; it just uses %S, but that string is used all over, so we'll have to use the keyname/string thing (it's only the SSLNoMatchingCiphers string on the 1.8.0 branch). Since that branch is frozen, that's not too big a concern. I guess the only question left is whether we run this as a live script during the process (which, on the 1.8.1 and the trunk, should help protect us when the strings change again) or if we manually edit the files and check them in to our embed-replacements tree (and then need to manually watch for changes in the original files).
Reporter | ||
Comment 5•18 years ago
|
||
It doesn't look like we'll be able to fix this using embed-replacements on the 1.8.0 branch for 1.0.x, because the message in question uses %S twice (the first time for the app name, and the second time for the site name), and when you replace the first %S in the string with "Camino", it merely pushes "(null)" into the second %S. SSLNoMatchingCiphers=%S and %S cannot communicate securely because they have no common encryption algorithms. We could change it to "Camino and this website", I suppose (and lose the hostname); is that worth doing? I'll have a look at the trunk/1.8branch file in a bit, but I'm hoping that the fact the substitution identifiers are different means that embed-replacements will work there.
Reporter | ||
Comment 7•18 years ago
|
||
Well, we can't use embed-replacements on the 1.8branch or the trunk, either; I finally got some time to run some tests, and the results are not pretty. In release builds, pre-substituting "Camino" for "%1$S" in those 3 strings just shows an entirely-devoid-of-text sheet (it crashes my debug builds without even triggering CrashReporter....) So at the moment it looks like we're stuck with crappy UI here, unless we can do option 3 from comment 0 (which I suppose may not actually be possible, either, but it's definitely beyond my ability to tell...).
is there some reason you can't do: SSLNoMatchingCiphers=Camino and %2$S cannot communicate securely because they have no common encryption algorithms.
Reporter | ||
Comment 9•18 years ago
|
||
(In reply to comment #8) > is there some reason you can't do: > > SSLNoMatchingCiphers=Camino and %2$S cannot communicate securely because they > have no common encryption algorithms. Yes, because that produces an empty sheet. That's what I was doing win comment 7 (embed-replacements just repacks modified files for us automatically). When Gecko thinks there are 2 things to sub in a string, Gecko is unhappy when it suddenly finds there is only one :P
Comment 10•18 years ago
|
||
Smokey, can you attach a patch with what you had when you tried to include a brand.dtd in our package? Basically, we should be fine if we include our own version of this. I think a jar.mn-change will be needed, so stuff can find it at chrome://branding/locale/brand.dtd
Reporter | ||
Comment 11•18 years ago
|
||
I only did a manual proof-of-concept hack when I messed with brand.dtd, which proved either 1) it doesn't work or 2) I don't know enough about how chrome works to put the files in the right place and modify the right references. My guess is it mostly proves the latter ;) For the record, I did something like this: bust up firefox.jar (since I had a fox and not a monkey), edit brand.dtd and the other file to have Camino branding, try to find the right relative URLs and pack them into embed.jar at those locations, and try to add the appropriate relative lines to installed-chrome.txt, then launch Camino and visit the site in the url field. (Also, just to be clear: if we successfully hack ourselves into shipping branding, we've applied a band-aid to Camino and pushed this class of bugs out of Mozilla.org and into external embeddors solely. Read bug 302309 comment 5, bug 302309 comment 8, and bug 302309 comment 18 for why this is so. I'm of course still in favor of fixing the branding issue in Camino however possible, including gross hacks, because the UE is awful without it, but everyone should be aware that Camino will then no longer be the "first line of defense" in catching these "bugs" for Moz embedding.)
Comment 12•18 years ago
|
||
What we need to do to get around this is ship our own branding, like firefox, sunbird and thunderbird do. I think they use --enable-branding Benjamin, do you think we could simply point --enable-branding= to some dir with only brand.dtd, brand.properties and a jar.mn? We don't need all the images and things that the other apps have.
Comment 13•18 years ago
|
||
you need a branding package registered in chrome. This is only incidentally related to the --enable-branding configure flag, which I doubt you want or need.
Comment 14•18 years ago
|
||
(In reply to comment #13) > you need a branding package registered in chrome. In practice, what does that mean that we have to do? Create our own /branding dir along with the others, in the tree?
Comment 15•18 years ago
|
||
Use jar.mn and whatnot to add a new chrome package. You can do that from any existing directory that has a jar.mn, or create a new one.
Assignee | ||
Comment 16•18 years ago
|
||
Comments 10-15 are now covered by 354013. The good news is that we can in fact hack around this with an embed-replacement pipnss.properties. What we need in each of those three strings is: %1$.0SCamino Which will do the substitution, so the code won't freak out about not having enough formatters for the arguments, but formats it as a zero-width string so it doesn't show up. I don't think we want to do the live script, since those are just positional, not meaningful; i.e., there's no guarantee that any occurrence of %1$S is supposed to be the app name, so I think the script would make us more likely to accidentally do the wrong thing than updating it manually before releases.
Assignee | ||
Comment 17•18 years ago
|
||
Does the above.
Assignee: nobody → stuart.morgan
Status: NEW → ASSIGNED
Attachment #246553 -
Flags: superreview?(mark)
Attachment #246553 -
Flags: review?(alqahira)
Comment 18•18 years ago
|
||
Comment on attachment 246553 [details] [diff] [review] hack Can you provide a brief comment, ideally at the top just past the license boilerplate, identifying the location in the tree of the original pipnss.properties file, and calling out the three lines that need to be changed?
Attachment #246553 -
Flags: superreview?(mark) → superreview+
Assignee | ||
Comment 19•18 years ago
|
||
Attachment #246553 -
Attachment is obsolete: true
Attachment #246553 -
Flags: review?(alqahira)
Assignee | ||
Comment 20•18 years ago
|
||
Same diffs, but the base file isn't exactly the same in branch and trunk.
Assignee | ||
Comment 21•18 years ago
|
||
Checked in on trunk and MOZILLA_1_8_BRANCH.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
No longer depends on: 360015
Keywords: fixed1.8.1.1
Resolution: --- → FIXED
Reporter | ||
Comment 22•18 years ago
|
||
We should probably take this hack in 1.0.4, if it works there (the formatter syntax is different [as are the strings, iirc]). Thanks for the fix, Stuart! Bug 360015 tracks fixing Core not to use branded strings for this file, for the interested.
Flags: camino1.0.4?
Reporter | ||
Comment 23•18 years ago
|
||
Here'a a 1.8.0 branch version of this (the file has different strings and a different formatter syntax). The "easiest" way to test this, since the original suspect sites have upgraded some of their certs, is to disable SSL2 and all the SSL2 and SSL3 ciphers on the 1.8.0 branch *except* security.ssl3.rsa_rc2_40_md5 and security.ssl3.rsa_rc4_40_md5 and then visit https://webmail.shaw.ca or https://bugzilla.mozilla.org. (If you visit a test site before disabling all of the ciphers, it's best to restart to avoid funny errors from the servers when testing.) (And, really, using cvsdo to add a file in a hierarchy of non-existent directories shouldn't be so @#$# difficult!)
Attachment #248143 -
Flags: review?(stridey)
Comment 24•18 years ago
|
||
Comment on attachment 248143 [details] [diff] [review] 1.8.0 branch patch r=me (man, that's annoying to test) As driver, I'll let you make the call about whether this needs sr.
Attachment #248143 -
Flags: review?(stridey) → review+
Reporter | ||
Comment 25•18 years ago
|
||
I just wanted a sanity-check; thanks.
Flags: camino1.0.4? → camino1.0.4+
Whiteboard: [Needs Checkin 1.8.0branch]
Comment 26•18 years ago
|
||
Checked in on 1.8.0branch
Keywords: fixed1.8.0.9
Whiteboard: [Needs Checkin 1.8.0branch]
You need to log in
before you can comment on or make changes to this bug.
Description
•