320349 - Crash on authentication to the proxy server (possibly MS ISA server)

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[John Found]

User-Agent:       Mozilla/4.0 (compatible; MSIE 5.5; Windows 98)
Build Identifier: Mozilla/5.0 

The problem is that, when FireFox uses password protected proxy, it crashes calling SECUR32.InitializeSecurityContextA function. Actually crashes secur32.dll, because of bad argumets passed from Firefox. 
The call is at address: 8916d1h in Firefox.exe 
The cause of the crash is the 12th argument of the function - pointer to TimeStamp structure. Microsoft states that this argument can be NULL, but Win98 version of secur32.dll crashes when this argument is NULL (maybe only in combination with some of the other arguments)... 
Visibly this bug is fixed in Win2000/XP versions of the .dll, because there Firefox works just fine. 
This crash is not observed in earlier versions of Firefox - 0.9 for example.



Reproducible: Always

Steps to Reproduce:
1. Set proxy to some MS ISA server with password authentification
2. Try to open any web page

Actual Results:  
FIREFOX caused an invalid page fault in
module SECUR32.DLL at 016f:7f8737da.
Registers:
EAX=76fdcee0 CS=016f EIP=7f8737da EFLGS=00010202
EBX=00000000 SS=0177 ESP=00d1f5bc EBP=00d1f640
ECX=00000000 DS=0177 ESI=00090312 FS=12e7
EDX=01c60159 ES=0177 EDI=023ce7a0 GS=2f26
Bytes at CS:EIP:
89 01 8b 45 fc 89 51 04 c7 40 20 01 00 00 00 83 
Stack dump:
023ceeb0 023ceea8 023ceeb0 00000000 00000000 00000000 00000000 023ce7b0 00000030 00000010 00af65c4 00000020 bff7a3a0 00af0000 00af65e4 00000010 

Expected Results:  
Not crashing at least ;)

I looked at the sourcecode and found two files where InitializeSecurityContext is called: 

1. mozilla/extensions/auth/nsAuthSSPI.cpp line 246: 

    rc = (sspi->InitializeSecurityContext)(&mCred, 
                                           ctxIn, 
                                           sn, 
                                           ctxReq, 
                                           0, 
                                           SECURITY_NATIVE_DREP, 
                                           inToken ? &ibd : NULL, 
                                           0, 
                                           &mCtxt, 
                                           &obd, 
                                           &ctxAttr, 
                                           NULL); 

I never wrote C++ before, but IMHO it should be: 

    TIMESTAMP Dummy; 
    rc = (sspi->InitializeSecurityContext)(&mCred, 
                                           ctxIn, 
                                           sn, 
                                           ctxReq, 
                                           0, 
                                           SECURITY_NATIVE_DREP, 
                                           inToken ? &ibd : NULL, 
                                           0, 
                                           &mCtxt, 
                                           &obd, 
                                           &ctxAttr, 
                                           &Dummy); 
 
2. mozilla/extensions/negotiateauth/nsNegotiateAuthSSPI.cpp line 322: 

The source code is absolutely the same as above (copy&pasted) and the solution too.

Comment 1

[Adam Guthrie]

John, so it looks like you know how to fix this. What you need to do is make a patch for this from CVS, then request a review on that patch from an appropriate reviewer.

Comment 2

[John Found]

(In reply to comment #1)
> John, so it looks like you know how to fix this. What you need to do is make a
> patch for this from CVS, then request a review on that patch from an
> appropriate reviewer.
> 

I am not C/C++ programmer at all, never worked with CVS and don't have time to learn these things right now. :)

Comment 3

[Ty Kelly]

I'm not a programmer.  I loaded firefox 1.5 yesterday. Now when I try to open the  "dailygraph" function that I use on the Investor's Business daily website Firefox crashes.  It has done this several times.   Any suggestions?   

Comment 4

[Adam Guthrie]

(In reply to comment #3)
> I'm not a programmer.  I loaded firefox 1.5 yesterday. Now when I try to open
> the  "dailygraph" function that I use on the Investor's Business daily website
> Firefox crashes.  It has done this several times.   Any suggestions?   

Install talkback, get a talkback ID for the crash. See if the bug's already filed, if not, file a new bug for the crash.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

Builds on windows, VC7.1.

Comment 6

[John Found]

(In reply to comment #5)
> Created an attachment (id=207311) [edit]
> patch
> 
> Builds on windows, VC7.1.
> 

Thank you. But don't forget about "mozilla/extensions/negotiateauth/nsNegotiateAuthSSPI.cpp" line 322. There is the same problem.

Regards

Comment 7

[timeless]

reporter: that file is dead on trunk, it was moved to the other place...

Comment 8

[Darin Fisher]

Comment on attachment 207311 [details] [diff] [review]
patch

We should get this in for FF 2 for sure, but it might also be a good one for a minor update to FF 1.5

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Checked in on the trunk. Thanks for filing this, John!

mozilla/extensions/auth/nsAuthSSPI.cpp; new revision: 1.6; previous revision: 1.5

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 207311 [details] [diff] [review]
patch

Might consider for 1.8.0.2 after more baking

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Landed on the 1.8 branch (for Firefox 2).
mozilla/extensions/auth/nsAuthSSPI.cpp; new revision: 1.4.2.2;

Comment 12

[Darin Fisher]

Comment on attachment 207311 [details] [diff] [review]
patch

OK, please consider this for 1.8.0.2.  See bug 330044.

Comment 13

[Tim Riley [:timr]]

Comment on attachment 207311 [details] [diff] [review]
patch

We don't want to re-spin bits.  "-" for 1.8.0.2,  "?" for 1.8.0.3.  This should be a no brainer for 1.8.0.3.

Comment 14

[Darin Fisher]

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

Comment 15

[Darin Fisher]

This is a trivial fix, that we should not pass over for 1.8.0.3

Comment 16

[Tim Riley [:timr]]

Comment on attachment 207311 [details] [diff] [review]
patch

a=timr.  crasher.  now baked. Land it!

Comment 17

[Ian Neal]

Yes please, land this on 1.8.0.x as we need this for SM1.0.2 - thanks

Comment 18

[Ian Neal]

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

Comment 19

[Daniel Veditz [:dveditz]]

Reopening to reassign to gavin (who checked in to other branches).

Comment 20

[:Gavin Sharp [email: [e-mail]]]

mozilla/extensions/auth/nsAuthSSPI.cpp 	1.4.2.1.4.4

Comment 21

[Colin Ogilvie [:cso]]

I'm now confused...

Is this fixed in 1.8.0.3, which should be the next release, or fixed in 1.8.0.4?

I can see approval1.8.0.3 being set on the attachment in the View Bug Activity table, but it's not set on the bug or attachment on show_bug.cgi, and I can't see it being removed in the table.

Comment 22

[Christian :Biesinger (don't email me, ping me on IRC)]

1.8.0.3 is special... it has just a single patch or so. what was planned to be 1.8.0.3 is now renamed to 1.8.0.4, so this is fixed only in .4.

Comment 23

[juan becerra [:juanb]]

John, could you verify this fix with one of the release candidates (1.5.0.4) that can be found here:

http://stage.mozilla.org/pub/mozilla.org/firefox/nightly/1.5.0.4-candidates/rc3/

I don't have a proxy I can specify to verify the fix.