97044 - PSM is passing null string to preferences [@ nsPrefBranch::QueryObserver]

Status: VERIFIED FIXED

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

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The fix for bug 44042, specifically, the change to the beginning of
nsNSSDialogs::ConfirmDialog, caused a topcrash with the following stack:

         nsPrefBranch::QueryObserver   
[d:\builds\seamonkey\mozilla\modules\libpref\src\nsPrefBranch.cpp  line 697]
         nsPrefBranch::GetBoolPref     
[d:\builds\seamonkey\mozilla\modules\libpref\src\nsPrefBranch.cpp  line 174]
         nsPrefService::GetBoolPref    
[d:\builds\seamonkey\mozilla\modules\libpref\src\nsPrefService.h  line 42]
         nsPref::GetBoolPref   
[d:\builds\seamonkey\mozilla\modules\libpref\src\nsPref.cpp  line 194]
         nsNSSDialogs::ConfirmDialog   
[d:\builds\seamonkey\mozilla\security\manager\pki\src\nsNSSDialogs.cpp  line 578]
         nsNSSDialogs::ConfirmPostToInsecureFromSecure 
[d:\builds\seamonkey\mozilla\security\manager\pki\src\nsNSSDialogs.cpp  line 563]
         nsSecureBrowserUIImpl::ConfirmPostToInsecureFromSecure
[d:\builds\seamonkey\mozilla\security\manager\ssl\src\nsSecureBrowserUIImpl.cpp
 line 819]
         nsSecureBrowserUIImpl::CheckPost      
[d:\builds\seamonkey\mozilla\security\manager\ssl\src\nsSecureBrowserUIImpl.cpp
 line 611]
         nsSecureBrowserUIImpl::Notify 
[d:\builds\seamonkey\mozilla\security\manager\ssl\src\nsSecureBrowserUIImpl.cpp
 line 261]
         nsFormFrame::OnSubmit 
[d:\builds\seamonkey\mozilla\layout\html\forms\src\nsFormFrame.cpp  line 874]
...

The null check for prefName was needed because of
ConfirmPostToInsecureFromSecure, which passes a pref name of |nsnull| with the
comment "No preference for this one - it's too important".  I think the simple
fix for this would be to revert that +5/-5 change.

This is the #1 not-yet-fixed topcrash on the trunk for the period 8-22 to 8-24.

Comment 1

[Bob Lord]

Target -> 2.1
Assigning to Kai to see what he makes of it.

Comment 2

[Gervase Markham [:gerv]]

Attachment: Patch v.1

Comment 3

[Gervase Markham [:gerv]]

This is my fault. :-( I assumed the preferences would do something sensible with
a null string. On reflection, though, exactly what is sensible isn't entirely
obvious.

The above patch should fix it - I'm currently building it, but a URL where you
submit secure_to_insecure would be very useful for testing. Feedback on whether
it's the _right_ fix also welcome.

Gerv

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Seems right to me.  r=dbaron

Comment 5

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

Attachment: Patch v.2

Comment 6

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

Hmm, I did some work on this, too, in the same time. I basically agree, but have
additional comments.

Gervase obviously assumed that GetBoolPref will simply fail (but not abort or
crash) if the passed prefName is null, because program flow is intended to
continue in that case. However, this assumption about GetBoolPref's behaviour is
wrong, as we learn from the crash.

To fix this, we need to prevent the call to GetBoolPref with a nsnull prefName
parameter.

As the same code appears twice in the same source file, I changed both
locations, which only can add safety.

I suggest the patch that I'll attach after this comment.

Something else I'd just like to make sure, please advice: Gervase commented that
"showAgainName" may now be null, too. Whether there is another possibility for a
crash depends on the behaviour nsAutoString.

Question: Is it safe to do
  nsAutoString str(nsnull);
i.e. will this be a valid empty string?

Ok, but even if it is, nsPromptServer::ConfirmEx will receive a valid checkMsg
parameter, although the passed string is of zero length. And this causes the
prompt dialog to contain a ghost checkbox, i.e. a checkbox the user can click,
but no text will be displayed to the right of the checkbox. I've actually seen
this sometimes in the past, but was never able to (or really tried) to reproduce it.

I'm suggesting code to ConfirmDialog that cares for the !showAgainName scenario.
It will pass a real nsnull to ConfirmEx, preventing a ghost checkbox.

... For later discussion: Should we add the logic to prevent a ghost checkbox
directly to nsPromptService::ConfirmEx, by adding a test for a non-zero length
of checkMsg ?

dbaron, can you please review?

Comment 7

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

Regarding the requested test case: You can use https://www.kuix.de/misc/test5/
which always posts to http, regardless whether this page was accessed with https
or http.

Comment 8

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

-> Kai

Comment 9

[Gervase Markham [:gerv]]

This still won't work because of a typo in security.properties:

PostToInsecurefromSecureMessage
should be
PostToInsecureFromSecureMessage

That needs fixing as well. But I'll give up now and leave kai on the case :-)

Gerv

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It doesn't really make sense to change AlertDialog to support a null prefName
but not a null showAgainName if neither are ever passed to it.  I think it would
be better to either leave it as is, or add support for both to keep it in
parallel with ConfirmDialog.  Other than that, and what Gerv mentioned, it looks
fine.

Comment 11

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

I don't want to change nsNSSDialogs::AlertDialog without changing
nsPromptService::AlertCheck, because AlertCheck doesn't check for a null
checkMsg. ConfirmEx makes this check.

To avoid making bigger changes, I take your recommandation and leave
AlertCheck/AlertDialog as it is.

I'm creating a new patch, where I add in Gerv's typo fix.

Comment 12

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

Attachment: Patch v. 3

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I don't think there's any need to have |actualAlertMsg| -- |alertMe.get()|
should return null if the nsXPIDLString wasn't initialized (I think).  Otherwise
r=dbaron.

Comment 14

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

dbaron, you are right, I just tested this. I assumed it to return a non-null
pointer to a zero length area. But it indeed returns null and actualCheckMsg ist
therefore not required.

That means, Gerv's suggestions will be sufficient.

Comment 15

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

Attachment: Patch rev. 1b

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

But I thought you did want that null check for showAgainName, just not the extra
string?

Comment 17

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

I tried what happens if 

  const PRUnichar *showAgainName = 0;
  nsXPIDLString alertMe;
  mStringBundle->GetStringFromName(showAgainName,
                                   getter_Copies(alertMe));

is executed. After the call to GetStringFromName, alertMe.get() is nil.
GetStringFromName already behaves safe. But if you prefer, I'll add the check
back.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

ok, r=dbaron either way, although I'd slightly prefer having the check

Comment 19

[Ben Goodger (use ben at mozilla dot org for email)]

sr=[e-mail]

Comment 20

[Asa Dotzler [:asa]]

a=asa on behalf of drivers.

Comment 21

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

Patch checked in, including the additional safety check.

Comment 22

[Stephen Walker]

*** Bug 97210 has been marked as a duplicate of this bug. ***

Comment 23

[jag (Peter Annema)]

*** Bug 97316 has been marked as a duplicate of this bug. ***

Comment 24

[bsharma]

Verified on 2001-08-30-Trunk build on WinNT

The test case https://www.kuix.de/misc/test5/ did posts to http. 

Verified bugs 97210 and 97316 and they also work fine.