Last Comment Bug 659666 - PSM passes uninitialized value for inout checkValue argument of promptPassword
: PSM passes uninitialized value for inout checkValue argument of promptPassword
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Security: PSM (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla7
Assigned To: Jonathan Protzenko [:protz]
:
Mentors:
Depends on:
Blocks: 658351
  Show dependency treegraph
 
Reported: 2011-05-25 09:50 PDT by Jonathan Protzenko [:protz]
Modified: 2011-06-06 10:54 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix the two bad cases (1.23 KB, patch)
2011-05-31 01:15 PDT, Jonathan Protzenko [:protz]
josh: review+
Details | Diff | Review

Description Jonathan Protzenko [:protz] 2011-05-25 09:50:08 PDT
In bug 658351, we added an assertion in the JS engine that checks for malformed JS Bools. An example of this which motivated the assertion can be found in the original description of bug 654126. However, the assert turned quite a few tests orange. One of the stacks <http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1306338785.1306339147.7955.gz> has PK11 stuff in it, so we need to investigate how this can possibly happen.

I've pushed this to try which will yield hopefully more interesting stack traces. http://tbpl.mozilla.org/?tree=Try&rev=e9bf068c8f4c
Comment 1 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-05-25 09:58:20 PDT
For what it's worth, I don't see any PK11 stuff in the log you're linking to...  I do see a broken stack (isGlobal() calling js_SetPropertyHelper???)
Comment 2 Blake Kaplan (:mrbkap) (please use needinfo!) 2011-05-25 10:17:38 PDT
bz, see http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1306339451.1306339806.11877.gz&fulltext=1#err0
Comment 3 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-05-25 11:21:22 PDT
Ah, that's a different log than comment 0!

The relevant C++ is this:

810       PRBool checkState;
811       rv = proxyPrompt->PromptPassword(nsnull, promptString.get(),
812                                        &password, nsnull, &checkState, &value);

and the relevant idl is this:

111      boolean promptPassword(in wstring dialogTitle,
112                             in wstring text,
113                             inout wstring password,
114                             in wstring checkMsg,
115                             inout boolean checkValue);

Note the passing of an uninitialized value (checkState) as an inout argument.  That would be bad.
Comment 4 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-05-25 11:22:59 PDT
And it doesn't matter what the value is, since checkMsg == null means the callee ignores that argument.  But in the meantime the marshalling code in XPConnect assumes that converting a PRBool to a JSBool just involves a cast, so we lose.

Initializing checkState to PR_FALSE, with a comment about how the exact value does not matter, should fix.
Comment 5 Jonathan Protzenko [:protz] 2011-05-25 13:06:27 PDT
So I've been looking into this, and we've got a few instances of the issue lurking around in the gecko codebase.
- nsDocumentViewer.cpp:1204 PRBool dummy is passed to a JS function without being ever initialized (it's, hem, a dummy),
- the one bz mentioned in comment #3, in nsNSSCallbacks.cpp:810,
- possibly others that are not covered by our tests.

After discussing with bz on IRC, the following might be reasonable:
- fix these two issues that have come up,
- keep the fatal JS_ASSERT in jsapi.h,
- have xpcconnect.cpp warn (possibly aggressively) if the boolean is malformed, as it possibly indicates a bug lurking around, but go on and pass !!thePRBool to make sure it doesn't violate the JS engine's invariants.

JS hackers, what's your opinion on this? :-)

I'll check for other failures in the try run and report here if I figure out another one of these situations.
Comment 6 Jeff Walden [:Waldo] (remove +bmo to email) 2011-05-25 13:34:16 PDT
Fix the issues, reject drek.  We require input to be well-formed many places in the JSAPI, and I see no reason why this should be any different, particularly not if would mean more work at runtime to be lenient.
Comment 7 Jonathan Protzenko [:protz] 2011-05-27 06:08:29 PDT
http://tbpl.mozilla.org/?tree=Try&rev=71dc30c95286

This is a try-run on all debug platforms with the JS_ASSERT patch and the two uninitialized PRBools properly initialized. I've got a few oranges remaining, could someone please confirm that these non-related, random oranges?
Comment 8 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-05-27 08:06:07 PDT
That doesn't seem to show anything...
Comment 9 Josh Matthews [:jdm] 2011-05-27 10:07:36 PDT
Every orange showing is a known intermittent. Looks good to me.
Comment 10 Jonathan Protzenko [:protz] 2011-05-31 01:15:57 PDT
Created attachment 536250 [details] [diff] [review]
Fix the two bad cases

This patch fixes the two bad cases, with a comment, as discussed before.
Comment 11 Josh Matthews [:jdm] 2011-05-31 04:09:54 PDT
Comment on attachment 536250 [details] [diff] [review]
Fix the two bad cases

I'm not certain why you've tagged me for review, but the comment seems pretty sensible.
Comment 12 Jonathan Protzenko [:protz] 2011-05-31 11:20:46 PDT
http://hg.mozilla.org/mozilla-central/rev/ec7f96260b25
Comment 13 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-06-06 10:44:24 PDT
> After discussing with bz on IRC, the following might be reasonable:

Uh... except we did not actually implement the solution we decided was reasonable?  What the hell?
Comment 14 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-06-06 10:45:28 PDT
And why exactly were callsites not actually audited, as we also discussed?

Note You need to log in before you can comment on or make changes to this bug.