MFCEmbed can't handle "mismatched" certificates

RESOLVED WONTFIX

Status

defect
P3
normal
RESOLVED WONTFIX
18 years ago
3 years ago

People

(Reporter: amutch, Unassigned)

Tracking

({embed})

Other Branch
Future
x86
Windows XP
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [kerh-cuz], )

Attachments

(1 attachment)

Reporter

Description

18 years ago
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.
-> PSM
Assignee: asa → ssaux
Component: Browser-General → Client Library
Product: Browser → PSM
QA Contact: doronr → junruh
Version: other → 1.01
Version: 1.01 → unspecified

Comment 2

18 years ago
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.
Priority: -- → P3
Target Milestone: --- → Future

Comment 3

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

Comment 4

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

Comment 5

18 years ago
Oops, I should read more closely next time.

-> Chak
Assignee: adamlock → chak

Comment 6

18 years ago
cc javi for comments.

Comment 7

18 years ago
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.
Reporter

Comment 8

18 years ago
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.  
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.)
Status: UNCONFIRMED → NEW
Ever confirmed: true
> 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

18 years ago
->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?
Assignee: chak → danm

Comment 12

18 years ago
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.
Reporter

Comment 13

17 years ago
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

17 years ago
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).
Keywords: embed

Comment 15

17 years ago
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

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

Comment 17

17 years ago
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

17 years ago
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.
Reporter

Comment 19

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

Updated

17 years ago
Status: NEW → ASSIGNED
Reporter

Comment 20

17 years ago
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

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

Comment 22

17 years ago
WORKSFORME on windows xp, mozilla a1
Reporter

Comment 23

17 years ago
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.
Reporter

Comment 24

17 years ago
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.
Depends on: 152224
Reporter

Comment 25

17 years ago
Doh! Never mind on that dependency. That belongs on another bug.
No longer depends on: 152224
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?
Reporter

Comment 27

17 years ago
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

17 years ago
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

17 years ago
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

17 years ago
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

17 years ago
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
Attachment #90276 - Flags: review+

Comment 32

17 years ago
Changes suggested by Adam in his r= : Done

Cc:ing rpotts for sr=

Comment 33

17 years ago
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
Reporter

Comment 34

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

Comment 35

17 years ago
Correct. I will talk to Rick to get this resolved...thanks

Updated

17 years ago
Blocks: majorbugs
Reporter

Comment 36

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

Comment 37

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

Comment 38

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

Updated

14 years ago
Component: Security: UI → Security: UI
Product: PSM → Core

Updated

14 years ago
Assignee: danm.moz → nobody
Status: ASSIGNED → NEW

Updated

14 years ago
No longer blocks: majorbugs

Updated

14 years ago
Whiteboard: [kerh-cuz]
QA Contact: junruh → ui
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.
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → WONTFIX
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.