733644 - nsNSSComponent does not check result of GetBoolPref/GetCharPref/GetIntPref and may initialize options to wrong values if they fail

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

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

nsNSSComponent::InitializeNSS and nsNSSComponent::Observe both have several calls to these methods that are made without initializing the output value before the call, and which don't check the return value from GetBoolPref/GetCharPref/GetIntPref to see if the output value was initialized. Consequently, if these calls failed for some reason, then these options may get initialized to unexpected values.

Comment 1

[:Cykesiopka]

Attachment: Proposed Patch v1

Comment 2

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

Comment on attachment 797620 [details] [diff] [review]
Proposed Patch v1

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

I'm sure this would be fine, but mozilla::Preferences already handles using default values (see https://mxr.mozilla.org/mozilla-central/source/modules/libpref/public/Preferences.h and https://mxr.mozilla.org/mozilla-central/search?string=mozilla%3A%3APreferences (include as '#include "mozilla/Preferences.h"')). As far as I can tell, it's functionally equivalent and would clean up nsNSSComponent.cpp a bit (basically, we could remove mPrefBranch).
Thanks for the patch!

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +1206,5 @@
>  
> +      bool md5Enabled;
> +      rv = mPrefBranch->GetBoolPref("security.enable_md5_signatures", &md5Enabled);
> +      if (NS_FAILED(rv)) {
> +        md5Enabled = false;

Around line 887, there's a list of #defines like "#define CRL_DOWNLOAD_DEFAULT false" - we should do the same thing for these (e.g. "#define MD5_ENABLED_DEFAULT false").

@@ +1233,1 @@
>          "security.ssl.allow_unrestricted_renego_everywhere__temporarily_available_pref", 

You might as well get rid of this trailing space while you're here.

@@ +1256,5 @@
>          uint16_t cipher_id = SSL_ImplementedCiphers[i];
>          SSL_CipherPrefSetDefault(cipher_id, false);
>        }
>  
> +      bool enabled;

Looks like this is called cipherEnabled later in equivalent code - might as well be consistent here.

Comment 3

[:Cykesiopka]

Attachment: Proposed Patch v2

+ Switch to mozilla::Preferences
+  => Remove mPrefBranch
+ Use defines for default values
+ Remove trailing spaces (that I noticed anyways)
+ enabled -> cipherEnabled for consistency

Comment 4

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

Comment on attachment 798120 [details] [diff] [review]
Proposed Patch v2

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

Looks great - just a couple more minor changes I think would be good, so r- for now.

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +906,1 @@
>    // 2 = enabled with given default responder

As far as I can tell, the 2 case doesn't apply anymore (default responders were removed in bug 531067), so we can remove this line. However, the line above it should go before the declaration/definition of ocspEnabled.

@@ +1006,1 @@
>    // 2 = enabled with given default responder

Same as above.

@@ +1115,3 @@
>  #endif
>  
> +    bool suppressWarningPref =

I'm not sure it's worth changing the name of this variable.

@@ +1190,5 @@
>          nsPSMInitPanic::SetPanic();
>          return NS_ERROR_UNEXPECTED;
>        }
>  
> +      configureMD5(Preferences::GetBool("security.enable_md5_signatures",

To be more clear and consistent, I think it would be good to separate this out into:

bool md5Enabled = Preferences::GetBool...
configureMD5(md5Enabled);

@@ +1628,3 @@
>        clearSessionCache = true;
>      } else if (prefName.Equals("security.enable_md5_signatures")) {
> +      configureMD5(Preferences::GetBool("security.enable_md5_signatures",

Same here with splitting this up.

Comment 5

[:Cykesiopka]

(In reply to David Keeler (:keeler) from comment #4)
> @@ +1115,3 @@
> >  #endif
> >  
> > +    bool suppressWarningPref =
> 
> I'm not sure it's worth changing the name of this variable.

It was mainly to correct the spelling mistake, but OK.

Comment 6

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

(In reply to Cykesiopka from comment #5)
> (In reply to David Keeler (:keeler) from comment #4)
> > @@ +1115,3 @@
> > >  #endif
> > >  
> > > +    bool suppressWarningPref =
> > 
> > I'm not sure it's worth changing the name of this variable.
> 
> It was mainly to correct the spelling mistake, but OK.

Oh, right - didn't see that. Sounds good, then :)

Comment 7

[:Cykesiopka]

Attachment: Proposed Patch v3

+ Update and move the ocspEnabled comments
+ Separate
    configureMD5(Preferences::GetBool...
  into:
    bool md5Enabled = Preferences::GetBool...
    configureMD5(md5Enabled);

Comment 8

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

Comment on attachment 799014 [details] [diff] [review]
Proposed Patch v3

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

Great!
Here's a try run: https://tbpl.mozilla.org/?tree=Try&rev=4fb8047aa22a
sr? to Brian to get approval for switching to using the static mozilla::Preferences from nsIPrefBranch (even though, again, as far as I can tell they're functionally equivalent - this is just more clean/clear).

Comment 9

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

Comment on attachment 799014 [details] [diff] [review]
Proposed Patch v3

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

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +1036,5 @@
> +#define TLS_SESSION_TICKETS_ENABLED_DEFAULT true
> +#define REQUIRE_SAFE_NEGOTIATION_DEFAULT false
> +#define ALLOW_UNRESTRICTED_RENEGO_DEFAULT false
> +#define FALSE_START_ENABLED_DEFAULT true
> +#define CIPHER_ENABLED_DEFAULT false

nit: Please use static const bool instead of #define. Also, please move these to the most restrictive scope possible (i.e. the function that uses them).

Comment 10

[:Cykesiopka]

(In reply to Brian Smith (:briansmith, was :[e-mail]) from comment #9)
> Comment on attachment 799014 [details] [diff] [review]
> Proposed Patch v3
> 
> Review of attachment 799014 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: security/manager/ssl/src/nsNSSComponent.cpp
> @@ +1036,5 @@
> > +#define TLS_SESSION_TICKETS_ENABLED_DEFAULT true
> > +#define REQUIRE_SAFE_NEGOTIATION_DEFAULT false
> > +#define ALLOW_UNRESTRICTED_RENEGO_DEFAULT false
> > +#define FALSE_START_ENABLED_DEFAULT true
> > +#define CIPHER_ENABLED_DEFAULT false
> 
> nit: Please use static const bool instead of #define. Also, please move
> these to the most restrictive scope possible (i.e. the function that uses
> them).

These values are used by both InitializeNSS() and Observe(), so should I just leave them where they are currently?

Also, do you want me to change SEND_LM_DEFAULT and the previously existing #defines to static const bool as well?

Comment 11

[:Cykesiopka]

Ping?

Comment 12

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

(In reply to Cykesiopka from comment #10)
> These values are used by both InitializeNSS() and Observe(), so should I
> just leave them where they are currently?

I'm sure that's fine.

> Also, do you want me to change SEND_LM_DEFAULT and the previously existing
> #defines to static const bool as well?

How about this: use static const bools for what you're adding, and then file a follow-up bug to convert the pre-existing #defines to static consts.

Comment 13

[:Cykesiopka]

Ok, thanks!

(In reply to David Keeler (:keeler) from comment #12)
> How about this: use static const bools for what you're adding, and then file
> a follow-up bug to convert the pre-existing #defines to static consts.

Bug 915937.

Comment 14

[:Cykesiopka]

Attachment: Patch for check in

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/38a63dbeab37

Comment 16

[Ryan VanderMeulen [:RyanVM]]

This was backed out while investigating a test failure and re-landed.
https://hg.mozilla.org/integration/mozilla-inbound/rev/01003151c3d5

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/01003151c3d5