965379 - InitCertVerifierLog is not called if PSM is initialized without a profile directory

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

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

InitCertVerifierLog is not called if PSM is initialized without a profile directory, which results in logging code dereferencing a null gCertVerifierLog.

Comment 1

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

Attachment: patch

Comment 2

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

Comment on attachment 8367519 [details] [diff] [review]
patch

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

Timely review is timely. The whole function is wrong.

Comment 3

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

Attachment: patch 1/2: fix indentation (diff -w version)

Comment 4

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

Attachment: patch 1/2:fix indentation

This patch first fixes the indentation of nsNSSComponent::InitializeNSS() (see the diff -w version for an easier-to-review patch).

Comment 5

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

Attachment: patch 2/2: refactor getting profile directory

I refactored getting the profile directory and cleaned up the initialization a bit.

Comment 6

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

Comment on attachment 8368278 [details] [diff] [review]
patch 2/2: refactor getting profile directory

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

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +1051,5 @@
>  
>    return NS_OK;
>  }
>  
>  nsresult

static

@@ +1127,5 @@
> +  // this call for every re-init of NSS.
> +
> +  ConfigureInternalPKCS11Token();
> +
> +  InitCertVerifierLog();

IMO, it is better to put this call closer to the other CertVerifier initialization logic. The reason is that it will be more clear how the standalone embedded CertVerifier (bug 918629) initialization maps to the in-Gecko initialization.

@@ +1141,5 @@
>      SECStatus init_rv = NSS_NoDB_Init(nullptr);
>      if (init_rv != SECSuccess) {
>        nsPSMInitPanic::SetPanic();
>        return NS_ERROR_NOT_AVAILABLE;
>      }

NIt: should we try to coalesce this with the other cal to NSS_NoDB_Init that occurs after ::mozilla::psm::InitializeNSS fails?

Comment 7

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

Comment on attachment 8368275 [details] [diff] [review]
patch 1/2: fix indentation (diff -w version)

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

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +1056,5 @@
>  nsNSSComponent::InitializeNSS()
>  {
>    // Can be called both during init and profile change.
>    // Needs mutex protection.
> +  MutexAutoLock lock(mutex);

I think this should go back to where it was originally, since neither the PR_LOG nor the static_assert require the lock.

Comment 8

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

Comment on attachment 8368276 [details] [diff] [review]
patch 1/2:fix indentation

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

r+ assuming this is the same as the "diff -w" version + whitespace-only changes. See my comments on the other patch.

Comment 9

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

Attachment: patch 2/2 v2: refactor getting profile directory

Thanks for the quick reviews, Brian. I changed where InitCertVerifierLog gets called and consolidated the calls to NSS_NoDB_Init.

Comment 10

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

Comment on attachment 8368325 [details] [diff] [review]
patch 2/2 v2: refactor getting profile directory

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

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +981,5 @@
>    CertVerifier::ocsp_get_config ogc;
>  
>    SetClassicOCSPBehaviorFromPrefs(&odc, &osc, &ogc, lock);
> +  // This is idempotent. Ensure the log module is initialized.
> +  InitCertVerifierLog();

Nit: should we initialize the log before SetClassicOCSPBehaviorFromPrefs in case SetClassicOCSPBehaviorFromPrefs ends up needing it at some point? i.e. Should we put it before the declaration of odc?

@@ +1153,5 @@
>    }
> +  // If we haven't succeeded in initializing the DB in our profile
> +  // directory or we don't have a profile at all, attempt to initialize
> +  // with no DB. Due to the setup above, this is equivalent to
> +  // if init_rv is not SECSuccess.

I suggest removing the last sentence, which has questionable grammar and is kind of stating the obvious.

Comment 11

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

Attachment: patch 1/2 v2: fix indentation

Moved the mutex. Carrying over r+.

Comment 12

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

Attachment: patch 2/2 v3: refactor getting profile directory

Moved InitCertVerifierLog call to before any certverifier functions are called. Carring over r+.
https://tbpl.mozilla.org/?tree=Try&rev=0eb6f4cdeee5

https://hg.mozilla.org/integration/mozilla-inbound/rev/23639e7efe16
https://hg.mozilla.org/integration/mozilla-inbound/rev/68c546e8a848

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/23639e7efe16
https://hg.mozilla.org/mozilla-central/rev/68c546e8a848