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

RESOLVED FIXED in mozilla26

Status

()

RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: briansmith, Assigned: Cykesiopka)

Tracking

Trunk
mozilla26
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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

Comment 1

5 years ago
Created attachment 797620 [details] [diff] [review]
Proposed Patch v1
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-
(Assignee)

Comment 3

5 years ago
Created attachment 798120 [details] [diff] [review]
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
Assignee: nobody → cykesiopka.bmo
Attachment #797620 - Attachment is obsolete: true
Status: NEW → ASSIGNED
(Assignee)

Updated

5 years ago
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-
(Assignee)

Comment 5

5 years ago
(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 :)
(Assignee)

Comment 7

5 years ago
Created attachment 799014 [details] [diff] [review]
Proposed Patch v3

+ 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+
(Assignee)

Comment 10

5 years ago
(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)
(Assignee)

Comment 11

5 years ago
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)
(Assignee)

Comment 13

5 years ago
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.
(Assignee)

Comment 14

5 years ago
Created attachment 804141 [details] [diff] [review]
Patch for check in
Attachment #799014 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.