Closed Bug 733644 Opened 8 years ago Closed 6 years ago

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

Categories

(Core :: Security: PSM, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: briansmith, Assigned: Cykesiopka)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
Attached patch Proposed Patch v1 (obsolete) — Splinter Review
Attachment #797620 - Flags: review?(brian)
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.
Attachment #797620 - Flags: review?(brian) → review-
Attached patch Proposed Patch v2 (obsolete) — Splinter Review
+ Switch to mozilla::Preferences
+  => Remove mPrefBranch
+ Use defines for default values
+ Remove trailing spaces (that I noticed anyways)
+ enabled -> cipherEnabled for consistency
Assignee: nobody → cykesiopka.bmo
Attachment #797620 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #798120 - Flags: review?(dkeeler)
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.
Attachment #798120 - Flags: review?(dkeeler) → review-
(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.
(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 :)
Attached patch Proposed Patch v3 (obsolete) — Splinter Review
+ Update and move the ocspEnabled comments
+ Separate
    configureMD5(Preferences::GetBool...
  into:
    bool md5Enabled = Preferences::GetBool...
    configureMD5(md5Enabled);
Attachment #798120 - Attachment is obsolete: true
Attachment #799014 - Flags: review?(dkeeler)
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).
Attachment #799014 - Flags: superreview?(brian)
Attachment #799014 - Flags: review?(dkeeler)
Attachment #799014 - Flags: review+
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).
Attachment #799014 - Flags: superreview?(brian) → superreview+
(In reply to Brian Smith (:briansmith, was :bsmith@mozilla.com) 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?
Flags: needinfo?(brian)
Ping?
(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.
Flags: needinfo?(brian)
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.
Attachment #799014 - Attachment is obsolete: true
Keywords: checkin-needed
This was backed out while investigating a test failure and re-landed.
https://hg.mozilla.org/integration/mozilla-inbound/rev/01003151c3d5
https://hg.mozilla.org/mozilla-central/rev/01003151c3d5
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.