Closed Bug 1277213 Opened 4 years ago Closed 3 years ago

Missing continuation state reset after auth dialog cancellation

Categories

(Core :: Networking: HTTP, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla50
Tracking Status
firefox47 --- wontfix
firefox48 + wontfix
firefox49 + verified
firefox-esr45 49+ verified
firefox50 + verified

People

(Reporter: cbook, Assigned: jhorak)

References

(Blocks 1 open bug, )

Details

(Keywords: assertion, csectype-race, sec-high, Whiteboard: hidden while we figure out comment 16/18[fixed by backout of bug 890908][necko-active][adv-main49+][adv-esr45.4+])

Attachments

(6 files, 1 obsolete file)

Attached file 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
marking as s-s bug
Group: core-security
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?
Flags: needinfo?(cbook)
(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
Flags: needinfo?(cbook)
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
nathan, does this help more ?
Flags: needinfo?(nfroyd)
(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.
Flags: needinfo?(nfroyd)
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?
Attached file bughunter asan stack
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.
This code changed a week or two ago in bug 890908.
Blocks: 890908
Flags: needinfo?(honzab.moz)
[Tracking Requested - why for this release]: sec-high
That ASAN stack looks way, way more valid than the Windows ones did. :)
For starter I think we should back bug 890908 out and think more.
Flags: needinfo?(honzab.moz)
(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
Whiteboard: [fixed by backout of bug 890908]
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
(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
Group: core-security → core-security-release
Group: core-security-release
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
Flags: needinfo?(cbook)
(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.
Flags: needinfo?(cbook)
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.
Group: core-security-release
Whiteboard: [fixed by backout of bug 890908] → hidden while we figure out comment 16/18[fixed by backout of bug 890908]
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
Attachment #8761516 - Flags: review?(honzab.moz)
(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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Flags: needinfo?(honzab.moz)
(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.
Flags: needinfo?(honzab.moz)
Assignee: nobody → jhorak
Status: REOPENED → ASSIGNED
Component: XPCOM → Networking: HTTP
Whiteboard: hidden while we figure out comment 16/18[fixed by backout of bug 890908] → hidden while we figure out comment 16/18[fixed by backout of bug 890908][necko-active]
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);
}
Attachment #8761516 - Flags: review?(honzab.moz) → review+
Summary: Hit MOZ_CRASH(nsAuthSSPI not thread-safe) at c:/Users/mozilla/debug-builds/mozilla-central/extensions/auth/nsAuthSSPI.cpp:181 → Missing continuation state reset after auth dialog cancellation
No longer blocks: 890908
Thank you! Copying positive review from previous comment.
Attachment #8761516 - Attachment is obsolete: true
Attachment #8765950 - Flags: review+
Attachment #8765950 - Flags: checkin?
needs sec-approval before landing btw
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.
Attachment #8765950 - Flags: checkin? → sec-approval?
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.
Attachment #8765950 - Flags: sec-approval? → sec-approval+
I'm confused by why 48 and ESR45 are marked as "unaffected" since your sec-approval? request says that everything is affected.
Flags: needinfo?(honzab.moz)
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.
Flags: needinfo?(honzab.moz)
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).
Flags: needinfo?(jhorak)
Flags: needinfo?(honzab.moz)
Apparently, Jan forgot about this bug..
Flags: needinfo?(honzab.moz)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/660eca9e31be
Status: ASSIGNED → RESOLVED
Closed: 4 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
We need an ESR45 and 49 patch for this.
(In reply to Al Billings [:abillings] from comment #35)
> We need an ESR45 and 49 patch for this.

Jan?
Flags: needinfo?(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.
Flags: needinfo?(jhorak)
Attached patch for 45 ESR. Copying previous positive review.
Attachment #8776938 - Flags: review+
Since 49 was transferred from central recently, attaching patch for beta.

Approval Request Comment
See comment 26
Attachment #8778836 - Flags: review+
Attachment #8778836 - Flags: approval-mozilla-beta?
Comment on attachment 8776938 [details] [diff] [review]
reset-state-after-cancel v2 patch [45esr version]

[Approval Request Comment]
See comment 26.
Attachment #8776938 - Flags: approval-mozilla-esr45?
Comment on attachment 8778836 [details] [diff] [review]
reset-state-after-cancel patch [beta]

Sec-high patch, ok to uplift to beta.
Attachment #8778836 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8776938 [details] [diff] [review]
reset-state-after-cancel v2 patch [45esr version]

Sec high, taking it to esr45.
Attachment #8776938 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
Flags: qe-verify+
Whiteboard: hidden while we figure out comment 16/18[fixed by backout of bug 890908][necko-active] → hidden while we figure out comment 16/18[fixed by backout of bug 890908][necko-active][adv-main49+][adv-esr45.4+]
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
QA Contact: cornel.ionce
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.