The default bug view has changed. See this FAQ.

PSM passes uninitialized value for inout checkValue argument of promptPassword

RESOLVED FIXED in mozilla7

Status

()

Core
Security: PSM
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: protz, Assigned: protz)

Tracking

unspecified
mozilla7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
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
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???)
bz, see http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1306339451.1306339806.11877.gz&fulltext=1#err0
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.
Summary: PSM possibly creates a badly formed JSBool → PSM passes uninitialized value for inout checkValue argument of promptPassword
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.
(Assignee)

Comment 5

6 years ago
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.
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.
(Assignee)

Comment 7

6 years ago
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?
That doesn't seem to show anything...

Comment 9

6 years ago
Every orange showing is a known intermittent. Looks good to me.
(Assignee)

Comment 10

6 years ago
Created attachment 536250 [details] [diff] [review]
Fix the two bad cases

This patch fixes the two bad cases, with a comment, as discussed before.
Assignee: nobody → jonathan.protzenko
Status: NEW → ASSIGNED
Attachment #536250 - Flags: review?(josh)
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.
Attachment #536250 - Flags: review?(josh) → review+
(Assignee)

Comment 12

6 years ago
http://hg.mozilla.org/mozilla-central/rev/ec7f96260b25
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
> 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?
And why exactly were callsites not actually audited, as we also discussed?
You need to log in before you can comment on or make changes to this bug.