Carsten Book [:Tomcat] Reporter
	Description • 8 years ago
str

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

	Carsten Book [:Tomcat] Reporter
	Comment 1 • 8 years ago

marking as s-s bug

	Nathan Froyd [:froydnj]
	Comment 2 • 8 years ago

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?

	Carsten Book [:Tomcat] Reporter
	Comment 3 • 8 years ago

(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

	Carsten Book [:Tomcat] Reporter
	Comment 4 • 8 years ago

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

	Carsten Book [:Tomcat] Reporter
	Comment 5 • 8 years ago

nathan, does this help more ?

	Nathan Froyd [:froydnj]
	Comment 6 • 8 years ago

(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.

	Bob Clary [:bc] (inactive)
	Comment 7 • 8 years ago

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?

	Andrew McCreight [:mccr8]
	Comment 9 • 8 years ago

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.

	Andrew McCreight [:mccr8]
	Comment 10 • 8 years ago

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

	Andrew McCreight [:mccr8]
	Comment 11 • 8 years ago

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

	Nathan Froyd [:froydnj]
	Comment 12 • 8 years ago

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

	Honza Bambas (:mayhemer)
	Comment 13 • 8 years ago

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

	Carsten Book [:Tomcat] Reporter
	Comment 14 • 8 years ago

(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

	Carsten Book [:Tomcat] Reporter
	Comment 15 • 8 years ago

(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

	Jan Horak [:jhorak] Assignee
	Comment 16 • 8 years ago

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

	Carsten Book [:Tomcat] Reporter
	Comment 17 • 8 years ago

(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.

	Jan Horak [:jhorak] Assignee
	Comment 18 • 8 years ago

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.

	Jan Horak [:jhorak] Assignee
	Comment 19 • 8 years ago

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

	Daniel Veditz [:dveditz]
	Comment 20 • 8 years ago

(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.

	Jan Horak [:jhorak] Assignee
	Comment 21 • 8 years ago

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.

	Honza Bambas (:mayhemer)
	Comment 22 • 8 years ago

(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.

	Honza Bambas (:mayhemer)
	Comment 23 • 8 years ago

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);
}

	Jan Horak [:jhorak] Assignee
	Comment 24 • 8 years ago

Thank you! Copying positive review from previous comment.

	Carsten Book [:Tomcat] Reporter
	Comment 25 • 8 years ago

needs sec-approval before landing btw

	Honza Bambas (:mayhemer)
	Comment 26 • 8 years ago

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.

	Al Billings [:abillings - ex-MoCo]
	Comment 27 • 8 years ago

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.

	Al Billings [:abillings - ex-MoCo]
	Comment 28 • 8 years ago

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

	Honza Bambas (:mayhemer)
	Comment 29 • 8 years ago

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.

	Al Billings [:abillings - ex-MoCo]
	Comment 30 • 8 years ago

Then we will want the newer patch everywhere.

	Daniel Veditz [:dveditz]
	Comment 31 • 8 years ago

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).

	Honza Bambas (:mayhemer)
	Comment 32 • 8 years ago

Apparently, Jan forgot about this bug..

	Ryan VanderMeulen [:RyanVM]
	Comment 33 • 8 years ago

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

	Carsten Book [:Tomcat] Reporter
	Comment 34 • 8 years ago

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

	Al Billings [:abillings - ex-MoCo]
	Comment 35 • 8 years ago

We need an ESR45 and 49 patch for this.

	Honza Bambas (:mayhemer)
	Comment 36 • 8 years ago

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

Jan?

	Jan Horak [:jhorak] Assignee
	Comment 37 • 8 years ago

(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.

	Jan Horak [:jhorak] Assignee
	Comment 38 • 8 years ago

Attached patch for 45 ESR. Copying previous positive review.

	Jan Horak [:jhorak] Assignee
	Comment 39 • 8 years ago

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

Approval Request Comment
See comment 26

	Jan Horak [:jhorak] Assignee
	Comment 40 • 8 years ago

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

[Approval Request Comment]
See comment 26.

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 41 • 8 years ago

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

Sec-high patch, ok to uplift to beta.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 42 • 8 years ago

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

	Sylvestre Ledru [:Sylvestre]
	Comment 43 • 8 years ago

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

Sec high, taking it to esr45.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 44 • 8 years ago

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

	Cornel Ionce [:noni] [Hubs QA]
	Comment 45 • 8 years ago

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.