113007 - MFCEmbed can't handle "mismatched" certificates

Status: RESOLVED WONTFIX

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

Description

[amutch]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:0.9.5) Gecko/20011011
BuildID:    MFCEmbed build 11/30/2001

MCFEmbed fails to properly handle certificates that don't come from the domain
that you are browsing.

Reproducible: Always
Steps to Reproduce:
1. Browse to https://www.levindustries.com/
2. Dialog windows appear with a checkbox for "Remember this certificate
permanently" and "OK", "Cancel" and "Help" buttons.
3. Checking the box and selecting the box clears the dialog windows.
4. The browser fails to go to the requested site.

Actual Results:  Browser fails to open the requested site.

Expected Results:  Browser should go to home page for https://www.levindustries.com/

This works OK in Mozilla 0.9.6 but needs to be fixed for embedded apps.

Comment 1

[Matthias Versen [:Matti]]

-> PSM

Comment 2

[Stephane Saux]

I don't know what mfc embed is.
If it works on the mozilla trunk (and it does)then it must be that the user of
the embedding kit is not using the relase properly.

Comment 3

[timeless]

MFC=microsoft foundation classes
MFC Embed uses them and gecko to make a browser application.

Comment 4

[Adam Lock]

Is this a problem just in MFCEmbed or is Mozilla affected too?

Comment 5

[Adam Lock]

Oops, I should read more closely next time.

-> Chak

Comment 6

[Stephane Saux]

cc javi for comments.

Comment 7

[Javier Delgadillo]

Just to get the issue straight here:

With MFCEmbed, you do see a dialog, but after hitting the "OK" button, you don't
get directed to the page you're expecting?

If that's the case, then it'll require some debugging.  It could be that
MFCEmbed's implementation of nsINSSDialogs isn't doing the same thing as the
pippki implementation of nsINSSDialogs.

Adding kaie to cc list because he's been in this code more recently than I have
and may have an idea as to what's going on.

Comment 8

[amutch]

That's essentially the problem. The dialog boxes appear but selecting "OK"
doesn't cause the browser to direct you to the correct page. I should note that
in the 12212001 build of MFCEmbed, the dialog boxes aren't displaying or
functioning properly but that may an unrelated issue.  

Comment 9

[Kai Engert [:KaiE:]]

I had a quick look on this:

I suspect it is indeed a problem specific to the embedding code. When you use
mfcembed to access any https site that throws up SSL information dialog boxes,
you see the problem.

The app obviously does not wait synchronously until the dialog boxes are closed.
Instead, the same dialog comes up twice (because of one automatic retry), and at
the same time the "you can't connect" error message comes up, too.

The implementation of nsIBadCertListener::UnknownIssuer tries to instantiate a
XUL window. (Although we have an an mfc embedded app, this seems to work.)
However, maybe the call to nsNSSDialogHelper::openDialog (which calls
nsIWindowWatcher->OpenWindow) returns immediately, giving a failure return code
to the callers? (Instead of waiting until the user makes the decision and closes
the dialog by pressing a button, and giving back that result code.)

Comment 10

[Conrad Carlen (not reading bugmail)]

> maybe the call to nsNSSDialogHelper::openDialog (which calls
> nsIWindowWatcher->OpenWindow) returns immediately, giving a failure return
> code to the callers?

If the nsNSSDialogHelper makes the window as a modal window, which I bet it
does, then the call to openWindow should not return until the user dismisses it.
If OpenWindow returns before the modal dialog is dismissed, it's a problem with
modality in mfcEmbed (I should try this with PPEmbed) and is not a problem in PSM.

Comment 11

[Chak Nanga]

->danm 

I'm not sure this is something to do with the MfcEmbed code per se - since the
cert windows are XUL windows invoked via WindowWatcher. 
Hi Dan : Please re-assign appropriately if you think this does not belong to you.

On another note, PSM code cannot assume that all the cert UI related XUL files
would be present in an embedding scenario which would resulting in empty dialogs
being shown.
Can someone provide me info on how an embeddor can override the XUL cert UI with
a native one i.e. how an embeddor can handle the dialogs described in this bug
themselves?

Comment 12

[Javier Delgadillo]

We assume that if there is an nsINSSDialogs implementation, it uses the correct
mechanism for displaying UI.  pippki is the default UI implementation and it
uses XUL.  If an embeder replaces pippki's implementation of nsINSSDialogs, then
we'll use the embeder's implementation.  We don't assume xul in pipnss (the
embedding SSL module), but we do in pippki (the default UI module for the browser)

If you want to replace the cert UI, implement nsNSSDialogs like the one found in
mozilla/security/manager/pki/src.  You don't have to implement all of the
interfaces our default nsNSSDialogs class does.  Just pick and choose which ones
you care about.  Seems like you care about the SSL warnings interfaces which
would be the nsIBadCertListener interfaces.

Write a class that implements 2 interfaces nsINSSDialogs and nsIBadCertListener
and you'll be over-riding our default XUL UI.

Hope that helps.

Comment 13

[amutch]

I recommend changing the summary to something more general like "MFCEmbed can't
access SSL sites" or something similar. There have been several bug reports that
all seem to trace back to the same problem which is reported below:

http://bugzilla.mozilla.org/show_bug.cgi?id=113007#c9

Not sure if I can do that as reporter so hopefully the owner will do that. I've
also asked Chak to dupe this bug: 

http://bugzilla.mozilla.org/show_bug.cgi?id=127417

as it is simply another variation on the same bug here.  

Also, I know everyone is busy with 1.0 issues but this is a big problem for our
embedding application and along with the downloading regression, has tied up our
release. 

Comment 14

[Joonas]

Adding embed keyword. The kmeleon people would like to see this one fixed, this
seems to be the bug blocking their 0.7 release (see
http://kmeleon.sourceforge.net/forum/read.php?f=1&i=3556&t=3556).

Comment 15

[Chak Nanga]

Sorry, i have not had a chance to spend much time on this one due to other
priorities.

I took a quick look at it and it looks like MfcEmbed needs to implement modal
chrome windows - which PSM is trying to display. Note that MfcEmbed's not
overriding any of the PSM dialogs, hence the XUL windows/WindowWatcher come into
play.

We currently have CBrowserImpl::ShowAsModal() (in BrowserImpl.cpp) returning
NS_ERROR_NOT_IMPLEMENTED resulting in the two dialogs being shown etc.
Implementing the ShowAsModal() seems like a first step towards fixing this
issue. [Put a breakpint in this function and then step thru' to see what exactly
is happening]

Cc:ing Adam Lock for his thoughts on what we can do to a top level window(which
is what eventually gets created via WindowWatcher) to make it behave as a modal
dialog.

Comment 16

[M. Epstein]

Not sure about this, but this bug may also be related to
http://bugzilla.mozilla.org/show_bug.cgi?id=124901

Comment 17

[amutch]

Do we think bug #124901:

http://bugzilla.mozilla.org/show_bug.cgi?id=124901

is a dupe of this bug? We can broaden the summary here to include all embedding 
clients. It was noted that this problem affects Macs too:

http://bugzilla.mozilla.org/show_bug.cgi?id=135211#c24

We could also revise the summary to deal with all SSL issues in embedding unless 
we want to file new bugs. I prefer to keep it all in one place. 

Comment 18

[M. Epstein]

Many of the comments in this bug talk of fixes to MFCEmbed. Does this bug need
to be resolved for each embedding client (e.g. MFCEmbed, GTKEmbed, Java
Webclient, etc.) or can it be fixed in a segment of code shared by all embedding
clients? The answer to that question will help determine whether all bugs
similar to this one should be marked as duplicates of this one or if each needs
to remain until its respective embedding client is fixed.

Comment 19

[amutch]

Please up the severity on this. It is a serious bug for embedding apps. 

Comment 20

[amutch]

Chak,

We have reports that this is affecting embedding apps across platforms including
Windows and Macs as I noted here:

http://bugzilla.mozilla.org/show_bug.cgi?id=113007#c17

This is more frustrating because SSL was working with our app before November.
While PSM support may not be a core part of the embedding package, a lot of
embedders need this functionality. It's a lot rougher to lose functionality that
you had than to deal with functionality that you never had in the first place.

Comment 21

[Chak Nanga]

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

Comment 22

[jr]

WORKSFORME on windows xp, mozilla a1

Comment 23

[amutch]

Please read the summary and the comments. This bug affects embedded builds of
Mozilla, not the main Mozilla distribution. The fact that it WORKSFORME in
Mozilla is irrelevant to this bug.

Comment 24

[amutch]

Adding dependency to this bug:

http://bugzilla.mozilla.org/show_bug.cgi?id=152224

If that's not fixed, we'll just have a memory leaking download dialog.

Comment 25

[amutch]

Doh! Never mind on that dependency. That belongs on another bug.

Comment 26

[Dan Mosedale (:dmosedale, :dmose)]

As far as I know, danm is on sabbatical until August.  So unless we can find
someone else to drive this bug, I wouldn't anticipate seeing a lot of motion on
it until then.  Anyone on the CC list interested in a taking a shot at it?

Comment 27

[amutch]

Dan,

Thanks for confirming that. I thought I had read that somewhere but couldn't 
confirm it. 

Chak (or someone else in embedding): Is the root of this problem in PSM or in 
embedding? There seems to be some dispute on that between the two units.
:)

If this is a PSM issue, can someone from PSM give this a little bit of time? I 
know that SSL support is not "required" by embedders but many of us need it, it 
appears to be affecting a wide range of applications and it's the one bug 
holding up the next release of K-Meleon. As this has been around since November 
2001, it's a long-standing issue.

Thanks!

Comment 28

[Chak Nanga]

I think the main issue here is with the way window watcher component creates XUL
"dialogs" rather than with PSM per se. When window watcher is done creating a
window on behalf of PSM, the PSM XUL content gets hosted in a *Window* and not a
*dialog* - this is where the issue of modality comes up. How do we make a top
level window modal?

Currently the only way i can think of trying to fix is by following the Javier's
suggestion at http://bugzilla.mozilla.org/show_bug.cgi?id=113007#c12

Unfortunately, i do not have many cycles right now to look at it.

Comment 29

[Mark Liffiton]

Attachment: Patch for ShowAsModal() in MFCEmbed

I did some digging, and I believe I have found a solution.

The problem seems to be simply that MFCEmbed does not implement ShowAsModal(). 
When WindowWatcher makes a new modal window, it tries to call this.  MFCEmbed
just returns NS_ERROR_NOT_IMPLEMENTED.

My fix was simply to borrow some code I found in WinEmbed (its version of
ShowAsModal()) and modify it slightly to make it work.	I've attached a patch
for BrowserImpl.cpp.  (Or at least I've tried to...  I am not yet learned in
the ways of diff and patch...  Sorry if it's not a proper patch.)

Comment 30

[Chak Nanga]

I've applied Mark's patch above and it seems to work fine. I no longer see the
dual mismatched cert dialogs. Clicking OK takes me to the levinindustries page
as expected.

Mark Liffiton : Thanks for the patch!

Adam : Can you please review the patch...thanks

Comment 31

[Adam Lock]

Comment on attachment 90276 [details] [diff] [review]
Patch for ShowAsModal() in MFCEmbed

aRunCondition seems to be mixing it's boolean types. It should be PRBool and
probably called runCondition since it isn't an arg. The method should also
return NS_OK, rather than msg.wParam unless there is an nsresult in there or
something.

Otherwise r=adamlock

Comment 32

[Chak Nanga]

Changes suggested by Adam in his r= : Done

Cc:ing rpotts for sr=

Comment 33

[rpotts (gone)]

I suspect that this implementation will 'eat' a WM_QUIT message :-(

If it is possible to generate a WM_QUIT message from showAsModal(...), then i
think that we should probably call PostQuitMessage() *again* after exiti9ng the
loop -- so the main message pump can receive it...

does this make sense?
-- rick

Comment 34

[amutch]

Chak: Was this back to you to make the changes Rick suggested?

Comment 35

[Chak Nanga]

Correct. I will talk to Rick to get this resolved...thanks

Comment 36

[amutch]

The current nightly of MFCEmbed (9/11) now throws an error in "gkcontent.dll"
when visiting the test site.

Comment 37

[amutch]

This is even more broken in MFCEmbed now. It fails to negotiate to the https
site at all. 

Comment 38

[John G. Myers]

Somebody in MFCEmbed space needs to push this patch and diagnose any additional
problems.

Comment 39

[Ed Morley [:emorley]]

Mass marking all MFCEmbed bugs as wontfix, since bug 377410 removed it. If this bug was erroneously included (or say equally applies to winEmbed), please reopen & accept my apologies.

Filter bugspam on mfcEmbedwontfix.