1380706 - PSM should depend on mozStorage, as a workaround for a sqlite3_config race

Status: RESOLVED FIXED

(Core :: Security: PSM, enhancement, P1)

Description

[Kai Engert [:KaiE:]]

This suggests to implement a PSM-specific workaround for bug 730495, which is required for bug 783994.

(The workaround makes sense for now, because solving bug 730495 in general seems difficult and has unknown timing.)

As mentioned in bug 730495, the workaround is to require loading of mozStorage from within PSM module init, prior to calling NSS library init.

Once bug 730495 is fixed in a general way, this workaround could get removed.

The patch has already been reviewed in bug 730495 comment 34.

I'm attaching a second revision of the patch, which contains the changes that David had requested.

Comment 1

[Kai Engert [:KaiE:]]

Attachment: 1380706-v2.patch

(In reply to David Keeler [:keeler] (use needinfo?) from bug 730495 comment #34)
> I think it might be best to put this in nsNSSComponent::Init so that it's
> impossible to instantiate PSM without the storage service. Also, since we're
> not checking rv, I think we can use the do_GetService variant that doesn't
> have the extra parameter.

New patch attached. Does the error code make sense?

try build:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6aa54693e2e6920cc8ee328085ce617494f3a3e7

Comment 2

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

Comment on attachment 8886235 [details] [diff] [review]
1380706-v2.patch

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

This looks good. Just the one nit and I think we should add a comment. Thanks!

::: security/manager/ssl/nsNSSComponent.cpp
@@ +56,5 @@
>  #include "sslerr.h"
>  #include "sslproto.h"
>  #include "prmem.h"
>  
> +#include "mozStorageCID.h"

nit: this should be earlier in the include list (it's intended to be sorted alphabetically, case-sensitive)

@@ +2038,5 @@
>  
> +  nsCOMPtr<nsISupports> storageService =
> +    do_GetService(MOZ_STORAGE_SERVICE_CONTRACTID);
> +  if (!storageService) {
> +    return NS_ERROR_NOT_AVAILABLE;

Yep - I think this is a good error code. Let's also have a comment explaining why we added this.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8e722cacf61
PSM should depend on mozStorage, as a workaround for a sqlite3_config race, r=keeler

Comment 4

[Kai Engert [:KaiE:]]

Thanks, pushed including the requested changes.

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b8e722cacf61

Comment 6

[Kai Engert [:KaiE:]]

for the record, this workaround was removed, as bug 730495 has been fixed.

Comment 7

[Jan Beich]

Can you backport this to ESR52 to fix a crash --with-system-nss after bug 1377940?

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=225364

Comment 8

[Kai Engert [:KaiE:]]

Jan, several fixes were applied to Firefox to make it fully with the NSS SQL DB, this bug is just one of the related bugs. I don't think we can backport all of them. If you're using NSS 3.35+ as a shared system library, you could consider to patch NSS to keep the old DBM default, as long as you cannot yet upgrade to Firefox 58 or newer, and plan to accept the change of default in NSS at the same time when you will upgrade your environment to Firefox 58+.

In any way, this bug is probably not the right place for this discussion, I suggest the mozilla.dev.tech.crypto list.

Comment 9

[Kai Engert [:KaiE:]]

s/fully/fully compatible/

Comment 10

[Jan Beich]

NSS only converts from DBM to SQL but not vice versa. Firefox 52.6.0esr built --with-system-nss did so after NSS 3.34.1 -> 3.35 upgrade, so reverting isn't an option due to potential loss of user data.

FreeBSD offers Firefox ESR as a *separate* package from Firefox 58. Bundled libraries are frowned upon. Mozilla treats BSDs as Tier3, so it's very hard to trust upstream to not introduce regressions.

https://www.freebsd.org/doc/en/books/porters-handbook/bundled-libs.html

Comment 11

[Mark Straver]

> several fixes were applied to Firefox to make it fully [compatible] with the NSS SQL DB, this bug is just one of the related bugs.

Is there a record of the applicable bugs for these "several fixes"? Backporting may be some work but for some projects hopping onto Quantum is simply a no-go area.

Comment 12

[Kai Engert [:KaiE:]]

Mark, you can find the relevant work, by looking at bug 783994 and bug 1377940, and for each of them, by looking at their "depends on" lists.

Comment 13

[Mark Straver]

Thanks Kal,

I've made note of it, if we have to revisit this later on (i.e. if we're forced to move to SQL which I hope won't be needed).

Our current solution is to prefix the cert db path with 'dbm:' to tell NSS we prefer DBM so we avoid this race condition and can keep using our preferred db format.