Remove SSLv3 option from SSL panel in Privacy & Security preferences

RESOLVED FIXED in seamonkey2.37

Status

SeaMonkey
Preferences
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: ewong, Assigned: rsx11m)

Tracking

SeaMonkey 2.36 Branch
seamonkey2.37
Dependency tree / graph

SeaMonkey Tracking Flags

(seamonkey2.35 unaffected, seamonkey2.36 fixed, seamonkey2.37 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

3 years ago
Bug 1106470 will be dropping support for SSLv3.  Once that happens,
there's little point in having the SSLv3 option in Privacy & Security->SSL
panel.
(Reporter)

Updated

3 years ago
Depends on: 1106470
(Assignee)

Comment 1

3 years ago
Taking, but nothing has been checked in yet on the Core/PSM side (targeted for Gecko 39). This may need some migration code for people who still have SSL 3.0 enabled on purpose (if we remove the checkbox, no way for the user to uncheck it from the UI).
Assignee: nobody → rsx11m.pub
Blocks: 1123673
Status: NEW → ASSIGNED
OS: Windows 8.1 → All
Hardware: x86_64 → All
Version: SeaMonkey 2.32 Branch → SeaMonkey 2.36 Branch
When bug 1106470 is fixed, security.tls.version.min=0 will be invalid and it will fallback to the default value (1).
(Assignee)

Comment 3

3 years ago
Thanks, good to know. Indeed, that's more a UI problem as the boxes can get into a state where you can't modify either .min (if its value is less than the allowed minimum) or .max (if its value exceeds the allowed maximum). Thus, I should probably revisit the logic here to ensure that only valid input ranges are considered to initialize the checkboxes, thus to avoid locking up the selections.
(Assignee)

Comment 4

3 years ago
On a side note: With the term "SSL" officially retiring soon, I'm wondering if we should rename that prefpane (which I'd file a separate bug for as it would likely require widespread adjustments to labels and Help content beyond the scope of what we are doing here). On the other hand, "SSL" is a reasonably established term, and other browsers/mail programs still use it routinely.

Something like "Transport Layer Security (SSL/TLS)" sounds more clear anyway (what's a "Socket" and why does it have to be "Secure"?), or maybe "Connection Security (SSL/TLS)" would be least ambiguous (with the short panel title in the prefpane tree being "SSL/TLS" or just "Connection Security").
(Assignee)

Updated

3 years ago
Summary: Remove SSLv3 option from SSL panel in Privacy & Security panel → Remove SSLv3 option from SSL panel in Privacy & Security preferences
(Assignee)

Comment 5

3 years ago
Bug 1106470 has landed on mozilla-inbound.
(Assignee)

Comment 6

3 years ago
Created attachment 8574761 [details] [diff] [review]
Proposed patch

This removes the allowSSL30 checkbox with respective changes to labels, logic, and Help content. The logic is extended to verify in UpdateSslBoxes() that the min/max values are in the valid range, and assumes valid min/max limits them if not; the preferences themselves aren't modified until the user actually touches any of the boxes.
Attachment #8574761 - Flags: ui-review?(neil)
Attachment #8574761 - Flags: review?(iann_bugzilla)

Comment 7

3 years ago
Comment on attachment 8574761 [details] [diff] [review]
Proposed patch

>+  gSslPrefElementIds = ["allowTLS10", "allowTLS11", "allowTLS12"];
>+  gSslPrefValues = [1, 2, 3];
Maybe use a Map to make the relation between pref values and the pref elements clearer?
gSslPrefElementIds = new Map([[1, "allowTLS10"], [2, "allowTLS11"], [3, "allowTLS12"]]);
(Assignee)

Comment 8

3 years ago
Created attachment 8574850 [details] [diff] [review]
Use a Map instead (v2)

Yeah, that works nicely and simplifies things.

(From bug 1106470 attachment 8569820 [details] [diff] [review])
>   // convert min/maxFromPrefs to the internal representation
>   minFromPrefs += SSL_LIBRARY_VERSION_3_0;
>   maxFromPrefs += SSL_LIBRARY_VERSION_3_0;
>   // if min/maxFromPrefs are invalid, use defaults
>   if (minFromPrefs > maxFromPrefs ||
>-      minFromPrefs < range.min || maxFromPrefs > range.max) {
>+      minFromPrefs < supported.min || maxFromPrefs > supported.max ||
>+      minFromPrefs < SSL_LIBRARY_VERSION_TLS_1_0) {
>     return;
>   }

I've revisited that based on comment #2. Since indeed the defaults are used when either of the conditions is met, I've modified the minVersion/maxVersion definitions used for the checkbox initialization respectively and take the defaults for both even if just one of them is outside of the valid range (also get them from the default prefbranch as they may not match the minimum/maximum values of the allowed pref settings).
Attachment #8574761 - Attachment is obsolete: true
Attachment #8574761 - Flags: ui-review?(neil)
Attachment #8574761 - Flags: review?(iann_bugzilla)
Attachment #8574850 - Flags: ui-review?(neil)
Attachment #8574850 - Flags: review?(iann_bugzilla)

Comment 9

3 years ago
Comment on attachment 8574850 [details] [diff] [review]
Use a Map instead (v2)

>+    minVersion = defaultBranch.getIntPref("security.tls.version.min");
>+    maxVersion = defaultBranch.getIntPref("security.tls.version.max");
document.getElementById("security.tls.version.min").defaultValue;
document.getElementById("security.tls.version.max").defaultValue;

>+    let currentBox = document.getElementById(name);
I wouldn't be averse to naming the id "id" ;-)
(Assignee)

Comment 10

3 years ago
Created attachment 8574894 [details] [diff] [review]
Comment #9 addressed (v3)

Getting better...
Attachment #8574850 - Attachment is obsolete: true
Attachment #8574850 - Flags: ui-review?(neil)
Attachment #8574850 - Flags: review?(iann_bugzilla)
Attachment #8574894 - Flags: ui-review?(neil)
Attachment #8574894 - Flags: review?(iann_bugzilla)
(Assignee)

Updated

3 years ago
Blocks: 1141324
(Assignee)

Comment 11

3 years ago
(Quoting to Masatoshi Kimura [:emk] from bug 1141394)
> In bug 1106470, I intentionally left strings and idl constants
> in case we have to backout the change on beta or aurora.
> Once the SSLv3 removal reaches to release, we can cleanup them.

Shall we proceed in the same way "just in case" that the SSL 3.0 strings are needed if and when it should be decided not to let the Core changes proceed down the release train? It sure would be trivial to defer removal of those two entities for 3+ cycles after the changes have hit the releases.
(Assignee)

Comment 12

3 years ago
Created attachment 8575034 [details] [diff] [review]
String changes deferred (v4)

Indeed, I think we should proceed in this way given that the core changes may depend on Google timing: 

(Quoting David Keeler [:keeler] from bug 1106470 comment #4)
> In general, looks good. I found more things to remove, however. Also, we
> should keep a very close eye on this. If it doesn't look like SSL 3 is going
> to be similarly removed from Chrome 44 or if that release date is not close
> enough to the release date for Firefox 39 (looks like 30 June 2015), we
> should hold this back at least one release.
Attachment #8574894 - Attachment is obsolete: true
Attachment #8574894 - Flags: ui-review?(neil)
Attachment #8574894 - Flags: review?(iann_bugzilla)
Attachment #8575034 - Flags: ui-review?(neil)
Attachment #8575034 - Flags: review?(iann_bugzilla)
Comment on attachment 8575034 [details] [diff] [review]
String changes deferred (v4)

Sorry for the delay, my computer has been acting strangely and I just noticed that it has 360 bad sectors. (But S.M.A.R.T. says that that's fine... go figure.)
Attachment #8575034 - Flags: ui-review?(neil) → ui-review+
(Assignee)

Comment 14

3 years ago
Yeah, S.M.A.R.T. isn't always very smart, though it should report at least that those blocks on the disk have been reallocated to spares (or that it ran out of those). Thanks for the ui-review!

Comment 15

2 years ago
Comment on attachment 8575034 [details] [diff] [review]
String changes deferred (v4)

r=me
Could you link the relevant bug for the string removal.
Attachment #8575034 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Updated

2 years ago
Depends on: 1141394
(Assignee)

Comment 16

2 years ago
Push for comm-central, please. I'll open a follow-up bug for the removal of the strings.
Keywords: checkin-needed
(Assignee)

Updated

2 years ago
Blocks: 1149581
(Assignee)

Updated

2 years ago
No longer depends on: 1141394
(Assignee)

Updated

2 years ago
status-seamonkey2.35: --- → unaffected
status-seamonkey2.36: --- → affected
status-seamonkey2.37: --- → affected
(Assignee)

Comment 17

2 years ago
Comment on attachment 8575034 [details] [diff] [review]
String changes deferred (v4)

[Approval Request Comment]
Regression caused by (bug #): Bug 1106470
User impact if declined: non-functional UI
Testing completed (on c-c, etc.): tested on c-c prior to the merge, thus c-a
Risk to taking this patch (and alternatives if risky): low
String changes made by this patch: Help only, see bug 1149581 for strings
Attachment #8575034 - Flags: approval-comm-aurora?

Updated

2 years ago
Attachment #8575034 - Flags: approval-comm-aurora? → approval-comm-aurora+
(Assignee)

Comment 18

2 years ago
Thanks, make that push please for comm-central /and/ comm-aurora.
Whiteboard: [c-n: comm-central/comm-aurora]

Comment 19

2 years ago
Comment on attachment 8575034 [details] [diff] [review]
String changes deferred (v4)

http://hg.mozilla.org/comm-central/rev/2dfbfcd7d32c

Comment 20

2 years ago
(In reply to Philip Chee from comment #19)
> http://hg.mozilla.org/comm-central/rev/2dfbfcd7d32c
http://hg.mozilla.org/releases/comm-aurora/rev/976e3ac4f9d9
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-seamonkey2.36: affected → fixed
status-seamonkey2.37: affected → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [c-n: comm-central/comm-aurora]
Target Milestone: --- → seamonkey2.37
You need to log in before you can comment on or make changes to this bug.