659666 - PSM passes uninitialized value for inout checkValue argument of promptPassword

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Jonathan Protzenko [:protz]]

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 [:bzbarsky]]

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) (inactive)]

bz, see http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1306339451.1306339806.11877.gz&fulltext=1#err0

Comment 3

[Boris Zbarsky [:bzbarsky]]

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 [:bzbarsky]]

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]]

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]]

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]]

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 [:bzbarsky]]

That doesn't seem to show anything...

Comment 9

[Josh Matthews [:jdm]]

Every orange showing is a known intermittent. Looks good to me.

Comment 10

[Jonathan Protzenko [:protz]]

Attachment: Fix the two bad cases

This patch fixes the two bad cases, with a comment, as discussed before.

Comment 11

[Josh Matthews [:jdm]]

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]]

http://hg.mozilla.org/mozilla-central/rev/ec7f96260b25

Comment 13

[Boris Zbarsky [:bzbarsky]]

> 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 [:bzbarsky]]

And why exactly were callsites not actually audited, as we also discussed?