917047 - Remove the security.enable_md5_signatures pref

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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.

Comment 1

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

mjh - I spoke with Brian, and he said there's no problem with you taking this bug.

Comment 2

[raymond [:retornam] (inactive)]

Attachment: bug-917047.patch

Comment 3

[Ajitesh Gupta]

Attachment: bug917047.patch

Comment 4

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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.

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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.

Comment 6

[Ajitesh Gupta]

Attachment: bug917047_patch2.patch

Comment 7

[mjh563]

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.

Comment 8

[raymond [:retornam] (inactive)]

I'll leave this to ajitesh since he has another patch up which addresses your review comments.

Comment 9

[Ajitesh Gupta]

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

Comment 10

[mjh563]

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.

Comment 11

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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.

Comment 12

[Ajitesh Gupta]

Attachment: bug917047.patch

Comment 13

[Masatoshi Kimura [:emk]]

Now the function name is misleading. Can we rename it to something like ConfigureMD5Disabled()?

Comment 14

[Ajitesh Gupta]

Attachment: bug917047.patch

Comment 15

[Ajitesh Gupta]

Attachment: bug917047.patch

Comment 16

[Camilo Viecco (:cviecco)]

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 17

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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.

Comment 18

[Ajitesh Gupta]

Attachment: bug917047.patch

Comment 19

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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"

Comment 20

[Ajitesh Gupta]

Attachment: bug917047.patch

Comment 21

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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

Comment 22

[Masatoshi Kimura [:emk]]

Should attachment 827311 [details] [diff] [review] be resurrected, then?

Comment 23

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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!

Comment 24

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 829828 [details] [diff] [review]
bug917047.patch

See comment 21.

Comment 25

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/230be93de11f

Thanks for the patch, and thanks to everybody else that reviewed the patches.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/230be93de11f