1277213 - Missing continuation state reset after auth dialog cancellation

Status: VERIFIED FIXED

(Core :: Networking: HTTP, defect)

Description

[Carsten Book [:Tomcat]]

Attachment: bughunter crash report

Found via bughunter and reproduced on a windows 7 trunk debug build based on m-c tip

Steps to reproduce: 
->> Load -> http://ib1c.nios.ru/Fresh_1c82_GUO_StAcc_R2_n4b01/1199/ru_RU/messageBox.html?sysver=8.3.7.1917
--> close the auth dialog

-->>

Hit MOZ_CRASH(nsAuthSSPI not thread-safe) at c:/Users/mozilla/debug-builds/mozilla-central/extensions/auth/nsAuthSSPI.cpp:181
#01: nsCOMPtr_base::assign_with_AddRef[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0x7d4b80]
#02: nsCOMPtr<nsISupports>::operator=[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0x6da6a2]
#03: nsHttpNegotiateAuth::MatchesBaseURI[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0xf5c3d4]
#04: nsHttpNegotiateAuth::Release[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0xf5cb4e]
#05: nsThread::ProcessNextEvent[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0x778459]
#06: NS_ProcessNextEvent[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0x7e3cd2]
#07: mozilla::ipc::MessagePumpForNonMainThreads::Run[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dl
#08: MessageLoop::RunInternal[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0xf9555d]
#09: MessageLoop::RunHandler[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0xf954d2]
#10: MessageLoop::Run[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0xf950cd]
#11: nsThread::ThreadFunc[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0x7834fa]
#12: _PR_NativeRunThread[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\nss3.dll +0x2fb02b]
#13: _PR_MD_YIELD[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\nss3.dll +0x2e01f9]
#14: crt_at_quick_exit[C:\Windows\system32\ucrtbase.DLL +0x7f794]
#15: BaseThreadInitThunk[C:\Windows\system32\kernel32.dll +0x4ef1c]
#16: RtlInitializeExceptionChain[C:\Windows\SYSTEM32\ntdll.dll +0x63648]
#17: RtlInitializeExceptionChain[C:\Windows\SYSTEM32\ntdll.dll +0x6361b]
[Child 4224] ###!!! ABORT: Aborting on channel error.: file c:/Users/mozilla/debug-builds/mozilla-central/ipc/glue/MessageChanne

Comment 1

[Carsten Book [:Tomcat]]

marking as s-s bug

Comment 2

[Nathan Froyd [:froydnj]]

Hm, that stack looks bogus, since MatchesBaseURI:

http://dxr.mozilla.org/mozilla-central/source/extensions/auth/nsHttpNegotiateAuth.cpp#625

doesn't even touch nsCOMPtr-ish things.  That stack is from a local build?

Comment 3

[Carsten Book [:Tomcat]]

(In reply to Nathan Froyd [:froydnj] from comment #2)
> Hm, that stack looks bogus, since MatchesBaseURI:
> 
> http://dxr.mozilla.org/mozilla-central/source/extensions/auth/
> nsHttpNegotiateAuth.cpp#625
> 
> doesn't even touch nsCOMPtr-ish things.  That stack is from a local build?

yes local debug build based and i think the bughunter build is from tinderbox

Comment 4

[Carsten Book [:Tomcat]]

ok, new debug builds based on https://hg.mozilla.org/mozilla-central/rev/22047a4eea784c15026c77911c0bd6ea1b70fa68

Hit MOZ_CRASH(nsAuthSSPI not thread-safe) at c:/Users/mozilla/debug-builds/mozilla-central/extensions/auth/nsAuthSSPI.cpp:181
#01: nsCOMPtr_base::assign_with_AddRef[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0x7d9380]
#02: nsCOMPtr<nsISupports>::operator=[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0x6deaa2]
#03: nsHttpNegotiateAuth::MatchesBaseURI[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0xf61764]
#04: nsHttpNegotiateAuth::Release[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0xf61ede]
#05: nsThread::ProcessNextEvent[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0x77cc79]
#06: NS_ProcessNextEvent[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0x7e84b2]
#07: mozilla::ipc::MessagePumpForNonMainThreads::Run[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0xffc4bc]
#08: MessageLoop::RunInternal[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0xf9a8bd]
#09: MessageLoop::RunHandler[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0xf9a832]
#10: MessageLoop::Run[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0xf9a42d]
#11: nsThread::ThreadFunc[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0x787cfa]
#12: _PR_NativeRunThread[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\nss3.dll +0x2fbc1b]
#13: _PR_MD_YIELD[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\nss3.dll +0x2e0de9]
#14: crt_at_quick_exit[C:\Windows\system32\ucrtbase.DLL +0x7f794]
#15: BaseThreadInitThunk[C:\Windows\system32\kernel32.dll +0x4ef1c]
#16: RtlInitializeExceptionChain[C:\Windows\SYSTEM32\ntdll.dll +0x63648]
#17: RtlInitializeExceptionChain[C:\Windows\SYSTEM32\ntdll.dll +0x6361b]
[Child 5772] ###!!! ABORT: Aborting on channel error.: file c:/Users/mozilla/debug-builds/mozilla-central/ipc/glue/MessageChannel.cpp, line 2052

Comment 5

[Carsten Book [:Tomcat]]

Attachment: windows debug build stack with todays m-c tip

nathan, does this help more ?

Comment 6

[Nathan Froyd [:froydnj]]

(In reply to Carsten Book [:Tomcat] from comment #5)
> Created attachment 8759053 [details]
> windows debug build stack with todays m-c tip
> 
> nathan, does this help more ?

That looks like the same stack. :(

There are a couple problems with that stack:

* nsHttpNegotiateAuth::Release shouldn't be getting called from nsThread::ProcessNextEvent.  There should be a ::Run method somewhere in there.

* nsHttpNegotiateAuth::Release is defined here:

http://dxr.mozilla.org/mozilla-central/source/extensions/auth/nsHttpNegotiateAuth.cpp#191

and there's nothing in that macro definition that would call MatchesBaseURI.

* nsHttpNegotiateAuth::MatchesBaseURI doesn't call any nsCOMPtr things; it's straight string manipulation.

bz (?) mentioned vtable corruption for some crashes on dev-platform the other day.  Perhaps that's what we're seeing here...I can try to reproduce at some point.

Comment 7

[Bob Clary [:bc] (inactive)]

asan builds on linux crash with operator++ nsNTLMAuthModule::AddRef() nsCOMPtr_base::assign_with_AddRef(nsISupports*) operator= ObtainCredentialsAndFlags

Perhaps you can get more information using them?

Comment 8

[Carsten Book [:Tomcat]]

Attachment: bughunter asan stack

Comment 9

[Andrew McCreight [:mccr8]]

The ASan stack says the crash is in this line in ObtainCredentialsAndFlags:
  362            mContinuationState = continuationState;
mContinuationState has type nsCOMPtr<nsISupports> mContinuationState so that makes sense.

Comment 10

[Andrew McCreight [:mccr8]]

This code changed a week or two ago in bug 890908.

Comment 11

[Andrew McCreight [:mccr8]]

[Tracking Requested - why for this release]: sec-high

Comment 12

[Nathan Froyd [:froydnj]]

That ASAN stack looks way, way more valid than the Windows ones did. :)

Comment 13

[Honza Bambas (:mayhemer)]

For starter I think we should back bug 890908 out and think more.

Comment 14

[Carsten Book [:Tomcat]]

(In reply to Honza Bambas (:mayhemer) from comment #13)
> For starter I think we should back bug 890908 out and think more.

this is backed out now in  https://hg.mozilla.org/mozilla-central/rev/a39da695528a

Comment 15

[Carsten Book [:Tomcat]]

(In reply to Carsten Book [:Tomcat] from comment #14)
> (In reply to Honza Bambas (:mayhemer) from comment #13)
> > For starter I think we should back bug 890908 out and think more.
> 
> this is backed out now in 
> https://hg.mozilla.org/mozilla-central/rev/a39da695528a

confirmed this is fixed by the backout

Comment 16

[Jan Horak [:jhorak]]

The server is sending two authentication requests:

WWW-Authenticate: NTLM
WWW-Authenticate: Negotiate

When NTLM dialog is cancelled the Negotiate authentication follows but mAuthContinuationState, instance of nsNTLMAuthModule, is not reset, so when proceeding with Negotiate authentication then [2] is called and since continuationState is set to instance nsNTLMAuthModule then the instance of nsAuthGSSAPI is not created [3]. 

Subsequently it is wrongly used in nsHttpNegotiateAuth::GenerateCredentials [4] so instead of nsAuthGSSAPI::GetNextToken is the nsNTLMAuthModule::GetNextToken called. So I presume the negotiate auth cannot work correcly even with current implementation.

This leads to crash when patch from bug 890908 is applied because nsNTLMAuthModule is not declared as NS_ITHREADSAFESUPPORT thus cannot be called from non-main thread from GetNextTokenRunnable::ObtainCredentialsAndFlags (code in patch [5] )

:Tomcat could you please test if WWW-authentication: Negotiate method work for you on http://ib1c.nios.ru/Fresh_1c82_GUO_StAcc_R2_n4b01/1199/ru_RU/messageBox.html?sysver=8.3.7.1917 if it is possible to test (ie. have convenient credentials for the site) that by cancelling NTLM dialog.

[2] http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannelAuthProvider.cpp#716
[3] http://mxr.mozilla.org/mozilla-central/source/extensions/auth/nsHttpNegotiateAuth.cpp#90
[4] http://mxr.mozilla.org/mozilla-central/source/extensions/auth/nsHttpNegotiateAuth.cpp#267
[5] https://bug890908.bmoattachments.org/attachment.cgi?id=8754813

Comment 17

[Carsten Book [:Tomcat]]

(In reply to Jan Horak from comment #16)
> The server is sending two authentication requests:
> 
> WWW-Authenticate: NTLM
> WWW-Authenticate: Negotiate
> 
> When NTLM dialog is cancelled the Negotiate authentication follows but
> mAuthContinuationState, instance of nsNTLMAuthModule, is not reset, so when
> proceeding with Negotiate authentication then [2] is called and since
> continuationState is set to instance nsNTLMAuthModule then the instance of
> nsAuthGSSAPI is not created [3]. 
> 
> Subsequently it is wrongly used in nsHttpNegotiateAuth::GenerateCredentials
> [4] so instead of nsAuthGSSAPI::GetNextToken is the
> nsNTLMAuthModule::GetNextToken called. So I presume the negotiate auth
> cannot work correcly even with current implementation.
> 
> This leads to crash when patch from bug 890908 is applied because
> nsNTLMAuthModule is not declared as NS_ITHREADSAFESUPPORT thus cannot be
> called from non-main thread from
> GetNextTokenRunnable::ObtainCredentialsAndFlags (code in patch [5] )
> 
> :Tomcat could you please test if WWW-authentication: Negotiate method work
> for you on
> http://ib1c.nios.ru/Fresh_1c82_GUO_StAcc_R2_n4b01/1199/ru_RU/messageBox.
> html?sysver=8.3.7.1917 if it is possible to test (ie. have convenient
> credentials for the site) that by cancelling NTLM dialog.
> 
Hi, i don't have any relation or login at this site, so i guess its not possible to login here.

Comment 18

[Jan Horak [:jhorak]]

Okay. With some help from our guys we'll try to setup server for testing. I'll fill a new bug as soon as I know more.

Comment 19

[Jan Horak [:jhorak]]

Attachment: reset-state-after-cancel v1 patch

I'm not sure if attaching patch to fix this issue is correct, but since I don't want to create another bug (not sure about security impact), attaching the patch there.

The patch is addressing a state when server sends more than one WWW-Authenticate header.

In case user cancels prompt for the first authentication method, then the continuationState is reused during processing next method. This is wrong, a new instance of continuationState must be created.

This is reproducible with server mentioned in comment 1, the Live HTTP Headers extensions shows following challenges for the server:
WWW-Authenticate: NTLM
WWW-Authenticate: Negotiate

Comment 20

[Daniel Veditz [:dveditz]]

(In reply to Jan Horak from comment #19)
> since I don't want to create another bug (not sure about security impact), attaching the patch there.

We should have created another bug:  "FIXED" bugs are invisible to most developers, and two fixes (even if related) in the same bug leads to confusion.

At this point maybe the best thing to do is to reopen the bug to at least address the first problem.

Comment 21

[Jan Horak [:jhorak]]

Could you please check it out or reassign the review to someone else? It seems to be a corner case, but it would be nice to have it fixed.

Comment 22

[Honza Bambas (:mayhemer)]

(In reply to Jan Horak from comment #21)
> Could you please check it out or reassign the review to someone else? It
> seems to be a corner case, but it would be nice to have it fixed.

I'll get to it next week.

Comment 23

[Honza Bambas (:mayhemer)]

Comment on attachment 8761516 [details] [diff] [review]
reset-state-after-cancel v1 patch

Review of attachment 8761516 [details] [diff] [review]:
-----------------------------------------------------------------

Definitely a good catch!  But it doesn't solve the root problem of the assertion.  Still, this should be landed (with sec+ approval) as a separate patch.

r+ on this patch with the one small change.

::: netwerk/protocol/http/nsHttpChannelAuthProvider.cpp
@@ +1245,5 @@
>              nsresult rv;
>  
> +            // Get rid of current continuationState to avoid reusing it in
> +            // next challenges since it is no longer relevant.
> +            NS_IF_RELEASE( mProxyAuth ? mProxyAuthContinuationState : mAuthContinuationState );

please do:

if (mProxyAuth) {
  NS_IF_RELEASE(mProxyAuthContinuationState);
} else {
  NS_IF_RELEASE(mAuthContinuationState);
}

Comment 24

[Jan Horak [:jhorak]]

Attachment: reset-state-after-cancel v2 patch

Thank you! Copying positive review from previous comment.

Comment 25

[Carsten Book [:Tomcat]]

needs sec-approval before landing btw

Comment 26

[Honza Bambas (:mayhemer)]

Comment on attachment 8765950 [details] [diff] [review]
reset-state-after-cancel v2 patch

(don't use checkin? flag on attachemnt, btw)


[Security approval request comment]
How easily could an exploit be constructed based on the patch?

  Hard to say, you would need a good knowledge of how all the auth modules and logic around works, so not that easy IMO.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

  I would say no.

Which older supported branches are affected by this flaw?

  All, this is there since ever AFAIK.

If not all supported branches, which bug introduced the flaw?
  
  It's there since ever AFAIK.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

  There is very low risk.

How likely is this patch to cause regressions; how much testing does it need?

  I was doing some manual testing with this patch, but I was focusing on a bit different problem.  I would leave this to Jan Horak, but this very much needs testing on Windows as well, which I'm not sure he posses.

Comment 27

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8765950 [details] [diff] [review]
reset-state-after-cancel v2 patch

sec-approval+ for trunk. We'll want patches made and nominated for ESR45, Aurora, and Beta as well though.

Comment 28

[Al Billings [:abillings - ex-MoCo]]

I'm confused by why 48 and ESR45 are marked as "unaffected" since your sec-approval? request says that everything is affected.

Comment 29

[Honza Bambas (:mayhemer)]

Because this has been discovered by a different patch landed just on m-c.  But later we found out the issue was unrelated and in the tree forever.  So, everything is affected.

Comment 30

[Al Billings [:abillings - ex-MoCo]]

Then we will want the newer patch everywhere.

Comment 31

[Daniel Veditz [:dveditz]]

What's the status here? We seem to have a reviewed sec-approved patch that never landed -- does it need a "check-in" keyword/flag? At this point we won't be able to get this into 48 (RC is already built).

Comment 32

[Honza Bambas (:mayhemer)]

Apparently, Jan forgot about this bug..

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/660eca9e31be

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/660eca9e31be

Comment 35

[Al Billings [:abillings - ex-MoCo]]

We need an ESR45 and 49 patch for this.

Comment 36

[Honza Bambas (:mayhemer)]

(In reply to Al Billings [:abillings] from comment #35)
> We need an ESR45 and 49 patch for this.

Jan?

Comment 37

[Jan Horak [:jhorak]]

(In reply to Honza Bambas (:mayhemer) from comment #36)
> (In reply to Al Billings [:abillings] from comment #35)
> > We need an ESR45 and 49 patch for this.
> 
> Jan?

Will do that.

Comment 38

[Jan Horak [:jhorak]]

Attachment: reset-state-after-cancel v2 patch [45esr version]

Attached patch for 45 ESR. Copying previous positive review.

Comment 39

[Jan Horak [:jhorak]]

Attachment: reset-state-after-cancel patch [beta]

Since 49 was transferred from central recently, attaching patch for beta.

Approval Request Comment
See comment 26

Comment 40

[Jan Horak [:jhorak]]

Comment on attachment 8776938 [details] [diff] [review]
reset-state-after-cancel v2 patch [45esr version]

[Approval Request Comment]
See comment 26.

Comment 41

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8778836 [details] [diff] [review]
reset-state-after-cancel patch [beta]

Sec-high patch, ok to uplift to beta.

Comment 42

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/52ad6d77d5e4

Comment 43

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8776938 [details] [diff] [review]
reset-state-after-cancel v2 patch [45esr version]

Sec high, taking it to esr45.

Comment 44

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-esr45/rev/d91fc76079e0

Comment 45

[Cornel Ionce [:noni] [Hubs QA]]

Verified that the issue no longer reproduces on Windows 7 x64, Windows 10 x64, Ubuntu 14.04x64 and Mac OS X 10.11.6 using the STR from the description.

Tested with the following Firefox versions:
*49.0 RC build 2, build ID 20160907153016
*50.0a2 Aurora, build ID 20160912004004
*45.4.0 ESR, build ID 20160905130425.