99566 - nsNSSComponent can init when already inited and crash

Status: VERIFIED FIXED

(Core Graveyard :: Security: UI, defect, P1)

Description

[Conrad Carlen (not reading bugmail)]

If this line is hit:
http://lxr.mozilla.org/mozilla/source/security/manager/ssl/src/nsNSSComponent.cpp#789
when mNSSInitialized == 1, crash. This happens with my patch for bug 97821. If 
initialization succeeds before the mentioned line, this will happen. The
nsNSSComponent code should put in a check to prevent this.

Comment 1

[Conrad Carlen (not reading bugmail)]

Attachment: suggested patch

Comment 2

[Conrad Carlen (not reading bugmail)]

The reason for this is that the NSS component can be made and successfully
initialized whenever a window is made and a profile available, due to
nsGlobalWindowImpl. Then, if the NSS component gets a "profile-after-change"
notification, it will re-initialize itself and crash. This can happen if some
other profile observer does something on its "profile-do-change" or
"profile-after-change" which causes a window to open. It needs to be possible to
do that.

Comment 3

[(Don't use, old account) - Kai Engert]

I think we are required to call the NSS init code again at profile switch,
because this does not only prepares the library, but it also opens and loads the
profile's certificate databases.

I will look into it.

Comment 4

[Conrad Carlen (not reading bugmail)]

Right, we need to do that but we just need to *not* do it when we already have
done it and have not shutdown.

Comment 5

[Stephane Saux]

P1

Comment 6

[Conrad Carlen (not reading bugmail)]

Can this be sooner than future please? It's blocking a bug  which I'd like to
get in and there is a patch?

Comment 7

[Stephane Saux]

Kai, feel free to help Conrad and work on this bug.

Kai please review the patch.
cc'ing relyea for review as well.

Comment 8

[(Don't use, old account) - Kai Engert]

Ok, I agree to your patch.

There might be more to be done to make profile switching work correctly. If I
look at the nsNSSComponent code, I fear calling InitializeNSS multiple times in
a session might not be the best idea. Especially we might want to avoided
calling RegisterCallback multiple times. Maybe we should move this call to a
different place. Not sure if the callback function will be called multiple times
with the current code.

r= kaie

Comment 9

[Conrad Carlen (not reading bugmail)]

> There might be more to be done to make profile switching work correctly.

Any improvement that can be done - let's do it :-) It would be nice to have
methods called something like LoadProfileData() and UnloadProfileData().

LoadProfileData() would load up everything from the current profile and, when it
succeeded, set an instance variable mProfileDataLoaded. The state of this var
could be checked at all entry points in the public API of nsNSSComponent and an
error returned to indicate the it was not initialized if that was the case.
Right now, it attempts to initialize NSS right away from Init() by calling
InitializeNSS() and, if that fails, the whole creation of the component has to
be repeated until it succeeds - about 4 or 5 failed attempts at startup when you
are creating a new profile (once for each window opened).

Instead, the component could do the bare minimum in Init() and not attempt to
initialize NSS until it recieved a "profile-after-change" notification.
Initializing NSS isn't going to succeed until that point anyway.

UnloadProfileData() would do the opposite of LoadProfileData and would be used
on getting a "profile-before-change" notification. Both of them would  check the
state of mProfileDataLoaded and safely do nothing depending on its state.

As you say, some code should probably be moved out of the profile start/shutdown
code path. In the meanwhile, until you want to do something more comprehensive,
we could use my simple patch. Kai - when it gets another review, can you check
it in - since I can't?

Comment 10

[(Don't use, old account) - Kai Engert]

Thanks for your suggestions. We also have bug 98182, that is caused by the same
nsNSSComponent method. However, the crashes in 98182 are not caused by NSS
called multiple times. In turbo mode, there is just a period of time, where the
Profile is unloaded and then reloaded, and some thread seems to be still alive,
trying to access the currently deactivated NSS library.

I wonder if we should move the NSS_Shutdown code to the if branch in ::Observe,
where we call init, so that there is no time gap between shutdown and re-init.

In any case, I'd like to find an implementation, that fixes both bug 98182 and
this one.

As we see that this method is called even without your new patch, I think this
problem should be fixed now.

Targetting PSM 2.1.

Comment 11

[Conrad Carlen (not reading bugmail)]

So, the profile is shut down, there is an outstanding async call, and something
 calls into NSS and since it's uninitialized, crash. I found one case of this
and added this:
http://lxr.mozilla.org/mozilla/source/security/manager/ssl/src/nsNSSComponent.cpp#732
Do we need protection against being called while not initialized on all entry
points?

I think we need to keep startup and shutdown independent and just fail
gracefully if there is time between the two events. Wasn't there some other
crash where, if NSS, failed to initialize, it had to be detected at startup or
else we'd crash. I think we need to be more tolerant of being  called when not
inited. What do you think?

Comment 12

[Robert Relyea]

From a practical point of view, you are talking a lot time work.... certainly
not the kind of thing that will be approved for 6.2 RTM. NSS was never designed
to be called when uninitialized. There are lots of entry points into NSS, and
PSM uses lots of private entry points as well. NSS was designed to be
initialized at application start and stay initialized throughout the lifetime of
the application. In fact it was only recently that you could shut NSS down, and
the caviat has always been that you must quiesce your system before you shut it
down, and you must initialize before you start things back up.

Comment 13

[(Don't use, old account) - Kai Engert]

This would require us to remove the turbo mode from the product.

I'm currently writing a (longer) discussion of what we could do to add
protection.

But a quick question first, to reassure what you said:

Bob, do you think it should work to init and shutdown NSS within a session
multiple times, if we guarantee, that no NSS function is ever called between the
shutdown and init calls?

Or is this still supposed to fail?

Comment 14

[Robert Relyea]

Yes, that should work.

You could also use PSM to keep track of NSS's initialization state since it
should be the only caller of NSS functions.

One word of warning, Initialize is idempotent and not reference counted. You can
safely call initialize multiple times and only the first will have an effect,
but the following sequence:

NSS_Init();
NSS_Init();
NSS_Init();
NSS_Shudown();

will result in NSS being shutdown.

bob

Comment 15

[(Don't use, old account) - Kai Engert]

Thanks Bob.

I just saw a place that might be really difficult to protect. During closing the
windows, while multiple windows were connecting at the same time, I received the
alerter that tells me "certificate domain name mismatch". But NSS was already
shut down. So I crashed when I selected to view the details...

I'm still working on a suggestion of what has to be done. But the more I think
about it, I agree with Bob, that this involves many changes.

Too many changes to land for 6.2, and the changes will introduce new risks
because we need to add mutexes to make it work safe.

Was the turbo mode feature present in 6.1? If not, can we just leave this
feature out and deactivate it in the branch?

Conrad, another question: Is it a simple change to suppress unloading /
reloading the profile? Or we could simply say, we assume that profile switching
is currently not possible, and do not try to shutdown or reinit NSS when we
observe the profile switch event (which is just triggered by turbo mode).

Comment 16

[(Don't use, old account) - Kai Engert]

Attachment: suggested temporary patch for 094 branch

Comment 17

[(Don't use, old account) - Kai Engert]

This patch is appropriate if we can assume that no other profile will be loaded.
In that case, we just leave NSS initialized with the already known cert databases.

This patch is not approriate for the trunk, where we might want provide support
for profile switching.

However, for the branch, I believe this patch can fix bug 98182. I wanted to
discuss here, before I attach the patch to 98182 (where it belongs to).

Comment 18

[(Don't use, old account) - Kai Engert]

Comment on attachment 50176 [details] [diff] [review]
suggested temporary patch for 094 branch

moving over to bug 98182

Comment 19

[(Don't use, old account) - Kai Engert]

Comment on attachment 49310 [details] [diff] [review]
suggested patch

Conrad, I'm not sure if this patch makes still sense for you, unless bug 101329 is not fixed.

If you need this on the trunk now, r=kaie

Comment 20

[Stephane Saux]

->future.  I don't believe this is needed for the 0.9.4 branch.

Kai, is this bug going to be addressed in the patch to 101329?

Comment 21

[(Don't use, old account) - Kai Engert]

Yes. The fix for 101329 will make sure that NSS can not be initialized multiple
times. In addition, I will make sure NSS is not reinitialized if this code path
is reached, but the profile did not change. I assume I can make this decision
based on the current profile directory, queryable using
  NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR, ...);

I agree that it doesn't need to land on the branch with the fix for 101364.

Conrad, if you agree, we could mark this bug as a duplicate of bug 101329, and
make bug 101329 the blocker for bug 97821.

Comment 22

[(Don't use, old account) - Kai Engert]

Changing my prefered e-mail address.

Comment 23

[Kai Engert [:KaiE:]]

Attachment: Suggested fix

I changed my mind, and now suggest to fix this bug with the attached patch.

It adds code to nsNSSComponent, enforcing that it is treated as a singleton.

I cleaned up the init and shutdown code, to make sure the right things are done
when the profile is being switched. I also added an assertion to make sure the
network is down when we shutdown NSS.

Javi, can you please review?

Comment 24

[Kai Engert [:KaiE:]]

Comment on attachment 57680 [details] [diff] [review]
Suggested fix

Sorry, need to update the patch slightly.

Comment 25

[Kai Engert [:KaiE:]]

Attachment: Updated fix

I was required to clean up the global hashtable of remembered certs between
profile changes - that's the only change from the previous patch.

Javi, can you please review?

Comment 26

[Conrad Carlen (not reading bugmail)]

Comment on attachment 57686 [details] [diff] [review]
Updated fix

>+#define PROFILE_CHANGE_NET_TEARDOWN_TOPIC NS_LITERAL_CSTRING("profile-change-net-teardown").get()
>+#define PROFILE_CHANGE_NET_RESTORE_TOPIC NS_LITERAL_CSTRING("profile-change-net-restore").get()
> #define PROFILE_BEFORE_CHANGE_TOPIC NS_LITERAL_CSTRING("profile-before-change").get()
> #define PROFILE_AFTER_CHANGE_TOPIC NS_LITERAL_CSTRING("profile-after-change").get()
>

I don't see where the code registers with observer service for the added
topics.
Also, I don't see the point in observing these topics. It does this so that it
can
ensure that the expected sequence of profile change notifications has been
sent?
The sequence of those calls can only change if the profile mgr code was changed
- it
doesn't change at runtime. If you do want to keep these checks in place, put
them
inside #if DEBUG

>+  if (nsCRT::strcmp(aTopic, PROFILE_CHANGE_NET_TEARDOWN_TOPIC) == 0) {
>+    isNetworkDown = PR_TRUE;
>+  } else if (nsCRT::strcmp(aTopic, PROFILE_CHANGE_NET_RESTORE_TOPIC) == 0) {
>+    isNetworkDown = PR_FALSE;
>+  } else if (nsCRT::strcmp(aTopic, PROFILE_BEFORE_CHANGE_TOPIC) == 0) {

Comment 27

[Kai Engert [:KaiE:]]

I thought it would be nice if PSM did this sanity check, so if the profile
manager code should ever change we would recognize it. But you are right, that
all should be done in debug mode only. I will add the forgotten topic
registration calls to debug mode only, too.

Comment 28

[Kai Engert [:KaiE:]]

Attachment: Patch including Conrad's suggestions

Comment 29

[Kai Engert [:KaiE:]]

The patch lacks a default initialization "hashTableCerts = nsnull" in the
nsNSSComponent::ctor. I will add that when I check it in.

Comment 30

[Conrad Carlen (not reading bugmail)]

I'll apply this patch and then the one from bug 97821 (the one which exposed
this problem) to make sure it's OK.

Comment 31

[Kai Engert [:KaiE:]]

I making this bug dependent on 108600 and 75947.
Both patches are required, because they remove the likelyhood for nsNSSComponent
being instantiated multiple times. This is required, as nsNSSComponent needs to
be treated as a singleton.

Comment 32

[Kai Engert [:KaiE:]]

Comment on attachment 57723 [details] [diff] [review]
Patch including Conrad's suggestions

This patch will be landing together with the patch in bug 75947.

Comment 33

[Kai Engert [:KaiE:]]

Fix checked in with patch for bug 75947.

Comment 34

[John Unruh]

Verified per kaie's comment.