343837 - SSL2/weak ciphers disabled messages contain "(null)" instead of "Camino"

Status: RESOLVED FIXED

(Camino Graveyard :: General, defect)

Description

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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

Comment 1

[:Gavin Sharp [email: [e-mail]]]

(null)

Comment 2

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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

Comment 3

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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. 

Comment 4

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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

Comment 5

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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.

Comment 6

[Samuel Sidler (old account; do not CC)]

Minus-ing for 1.0.3 per meeting.

Comment 7

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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

Comment 8

[timeless]

is there some reason you can't do:

SSLNoMatchingCiphers=Camino and %2$S cannot communicate securely because they have no common encryption algorithms.

Comment 9

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(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

[Håkan Waara]

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

Comment 11

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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

[Håkan Waara]

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

[Benjamin Smedberg]

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

[Håkan Waara]

(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

[Benjamin Smedberg]

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.

Comment 16

[Stuart Morgan]

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.

Comment 17

[Stuart Morgan]

Attachment: hack

Does the above.

Comment 18

[Mark Mentovai]

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?

Comment 19

[Stuart Morgan]

Attachment: with comment

Comment 20

[Stuart Morgan]

Attachment: branch version

Same diffs, but the base file isn't exactly the same in branch and trunk.

Comment 21

[Stuart Morgan]

Checked in on trunk and MOZILLA_1_8_BRANCH.

Comment 22

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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.

Comment 23

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: 1.8.0 branch patch

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!)

Comment 24

[froodian (Ian Leue)]

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.

Comment 25

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

I just wanted a sanity-check; thanks.

Comment 26

[froodian (Ian Leue)]

Checked in on 1.8.0branch