Closed Bug 917047 Opened 6 years ago Closed 6 years ago

Remove the security.enable_md5_signatures pref

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: briansmith, Assigned: ajgupta93)

References

Details

(Whiteboard: [good first bug][good first verify])

Attachments

(8 obsolete files)

This pref is a footgun. After this pref was added, MD5 warnings were made user-overridable on a per-certificate basis (like expired cert warnings), so even people who want to surf to MD5-based-cert-protected sites can do so.
No longer blocks: 917049
Whiteboard: [good first bug]
Assignee: nobody → brian
Blocks: 891066
Target Milestone: --- → mozilla27
No longer blocks: 891066
mjh - I spoke with Brian, and he said there's no problem with you taking this bug.
Assignee: brian → mjh563
Assignee: mjh563 → ajgupta93
Status: NEW → ASSIGNED
Attached patch bug-917047.patch (obsolete) — Splinter Review
Attachment #826960 - Flags: review?(dkeeler)
Attached patch bug917047.patch (obsolete) — Splinter Review
Attachment #826973 - Flags: review?(dkeeler)
Comment on attachment 826960 [details] [diff] [review]
bug-917047.patch

Review of attachment 826960 [details] [diff] [review]:
-----------------------------------------------------------------

Good, but there are a few more things that need to happen. In particular, all references to MD5_ENABLED_DEFAULT need to be removed as well.

::: netwerk/base/public/security-prefs.js
@@ +3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  pref("security.tls.version.min", 0);
>  pref("security.tls.version.max", 3);
> +

We don't need this empty line here.

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ -1262,5 @@
>        }
>  
> -      bool md5Enabled = Preferences::GetBool("security.enable_md5_signatures",
> -                                             MD5_ENABLED_DEFAULT);
> -      ConfigureMD5(md5Enabled);

We still need to call ConfigureMD5. However, md5Enabled will never be true, so we should just remove that parameter from the function and remove its else branch.
Attachment #826960 - Flags: review?(dkeeler) → review-
Comment on attachment 826973 [details] [diff] [review]
bug917047.patch

Review of attachment 826973 [details] [diff] [review]:
-----------------------------------------------------------------

Essentially, same comments as with the other patch.
Also, the two of you need to coordinate on who's going to continue working on this patch. There are many other things to be working on, and we don't need two people writing the same patch.
Attachment #826973 - Flags: review?(dkeeler) → review-
Attached patch bug917047_patch2.patch (obsolete) — Splinter Review
Attachment #827052 - Flags: review?(dkeeler)
Ajitesh, when you attach updated patches in future, please mark the previous patch as obsolete at the same time. There's an option to do this on the 'add an attachment' page.
Attachment #826973 - Attachment is obsolete: true
I'll leave this to ajitesh since he has another patch up which addresses your review comments.
Attachment #826960 - Attachment is obsolete: true
(In reply to mjh563 from comment #7)
> Ajitesh, when you attach updated patches in future, please mark the previous
> patch as obsolete at the same time. There's an option to do this on the 'add
> an attachment' page.

Sorry, my bad. I'll take care of that from now on.
Does the new patch actually build without error? You made this change:

-      ConfigureMD5(md5Enabled);
+      ConfigureMD5();

but the ConfigureMD5 function is still defined as

void ConfigureMD5(bool enabled)

Also, it looks like you've removed the wrong branch of the condition in ConfigureMD5. Since the 'enabled' argument will now never be true, you should remove the branch where it's true, not the branch where it's false.
Attachment #827052 - Flags: review?(dkeeler)
Comment on attachment 827052 [details] [diff] [review]
bug917047_patch2.patch

Review of attachment 827052 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to mjh563 from comment #10)
> Does the new patch actually build without error? You made this change:
> 
> -      ConfigureMD5(md5Enabled);
> +      ConfigureMD5();
> 
> but the ConfigureMD5 function is still defined as
> 
> void ConfigureMD5(bool enabled)
> 
> Also, it looks like you've removed the wrong branch of the condition in
> ConfigureMD5. Since the 'enabled' argument will now never be true, you
> should remove the branch where it's true, not the branch where it's false.

Indeed. Also, you'll need to change the call at
media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:113
(https://mxr.mozilla.org/mozilla-central/ident?i=ConfigureMD5 might be helpful)

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +1673,5 @@
>      if (prefName.Equals("security.tls.version.min") ||
>          prefName.Equals("security.tls.version.max")) {
>        (void) setEnabledTLSVersions();
>        clearSessionCache = true;
>      } else if (prefName.Equals("security.enable_md5_signatures")) {

We can remove this entire branch in the compound if statement.
Attachment #827052 - Flags: review-
Attached patch bug917047.patch (obsolete) — Splinter Review
Attachment #827052 - Attachment is obsolete: true
Attachment #827249 - Flags: review?(dkeeler)
Now the function name is misleading. Can we rename it to something like ConfigureMD5Disabled()?
Attached patch bug917047.patch (obsolete) — Splinter Review
Attachment #827249 - Attachment is obsolete: true
Attachment #827249 - Flags: review?(dkeeler)
Attachment #827290 - Flags: review?(dkeeler)
Attached patch bug917047.patch (obsolete) — Splinter Review
Attachment #827290 - Attachment is obsolete: true
Attachment #827290 - Flags: review?(dkeeler)
Attachment #827311 - Flags: review?(dkeeler)
Comment on attachment 827311 [details] [diff] [review]
bug917047.patch

Review of attachment 827311 [details] [diff] [review]:
-----------------------------------------------------------------

Drive by review.

If it was up to me I would remove the funcion configureMD5Disabled and just place the 3 lines of disabling MD5 (the content of the disable call) within InitializeCipherSuite with a comment. (similar to how PKCS12 ciphers are enabled.
Comment on attachment 827311 [details] [diff] [review]
bug917047.patch

Review of attachment 827311 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Camilo Viecco (:cviecco) from comment #16)
> Comment on attachment 827311 [details] [diff] [review]
> bug917047.patch
> 
> Review of attachment 827311 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Drive by review.
> 
> If it was up to me I would remove the funcion configureMD5Disabled and just
> place the 3 lines of disabling MD5 (the content of the disable call) within
> InitializeCipherSuite with a comment. (similar to how PKCS12 ciphers are
> enabled.

I agree. Otherwise, looks good.
Attachment #827311 - Flags: review?(dkeeler) → review-
Attached patch bug917047.patch (obsolete) — Splinter Review
Attachment #827311 - Attachment is obsolete: true
Attachment #827771 - Flags: review?(dkeeler)
Comment on attachment 827771 [details] [diff] [review]
bug917047.patch

Review of attachment 827771 [details] [diff] [review]:
-----------------------------------------------------------------

Great - r=me with the comment change.

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +2024,5 @@
>    SEC_PKCS12EnableCipher(PKCS12_DES_EDE3_168, 1);
>    SEC_PKCS12SetPreferredCipher(PKCS12_DES_EDE3_168, 1);
>    PORT_SetUCS2_ASCIIConversionFunction(pip_ucs2_ascii_conversion_fn);
>  
> +  // Disable MD5 security signature check

how about just "Disable MD5"
Attachment #827771 - Flags: review?(dkeeler) → review+
Attached patch bug917047.patch (obsolete) — Splinter Review
Attachment #827771 - Attachment is obsolete: true
Attachment #829828 - Flags: review?(dkeeler)
Attachment #829828 - Flags: review?(dkeeler) → review+
Keywords: checkin-needed
(In reply to Camilo Viecco (:cviecco) from comment #16)
> Comment on attachment 827311 [details] [diff] [review]
> bug917047.patch
> 
> Review of attachment 827311 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Drive by review.
> 
> If it was up to me I would remove the funcion configureMD5Disabled and just
> place the 3 lines of disabling MD5 (the content of the disable call) within
> InitializeCipherSuite with a comment. (similar to how PKCS12 ciphers are
> enabled.

There is a reason that ConfigureMD5 isn't inlined into InitializeCipherSuite: ConfigreMD5 applies to all certificate and signature verification, not just SSL stuff. Remember that ConfigureMD5 will get moved to security/certverifier in one of my patches, but InitializeCipherSuite won't.
Should attachment 827311 [details] [diff] [review] be resurrected, then?
Comment on attachment 827311 [details] [diff] [review]
bug917047.patch

Review of attachment 827311 [details] [diff] [review]:
-----------------------------------------------------------------

I will check this in, with ConfigureMD5Disabled renamed to DisableMD5. Thank you for the patches!
Attachment #827311 - Flags: review+
Attachment #829828 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/230be93de11f

Thanks for the patch, and thanks to everybody else that reviewed the patches.
Target Milestone: mozilla27 → mozilla28
https://hg.mozilla.org/mozilla-central/rev/230be93de11f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug] → [good first bug][good first verify]
You need to log in before you can comment on or make changes to this bug.