1030426 - network.negotiate-auth.allow-insecure-ntlm-v1-https allows sending NTLMv1 credentials in plain to HTTP proxies

Status: VERIFIED FIXED

(Core :: Networking: HTTP, defect)

Description

[Honza Bambas (:mayhemer)]

Attachment: ntlm-plain-proxy-block.patch

The nsIHttpAuthenticableChannel.isSSL attribute returns true also when only authenticating to a plain HTTP proxy.

Since we need to uplift the patch up to beta and maybe even release, I go the simple way: I ignore "allow-insecure-ntlm-v1-https" value when doing a proxy authentication.  We have introduced https proxy support only recently, so at least on beta (where https proxy support has not arrived yet) we can well assume that |proxyAuth == true| means authenticating with a plain HTTP proxy.

Hence, this trivial patch.  

For HTTPS with NTLM proxying we have to invent some new mechanism and probably modify some interfaces.

According testing... setup squid for NTLM auth is really hard.  I can try with my WS2003 server and authenticating ISA server installed on it or fool my local IIS server to behave as a proxy.

Comment 1

[Nick Lowe]

Great catch!

Comment 2

[Honza Bambas (:mayhemer)]

As usual with NTLM - no luck with testing...  We will have to land and ask users to test this for us.

Comment 3

[Patrick McManus [:mcmanus]]

Comment on attachment 8446209 [details] [diff] [review]
ntlm-plain-proxy-block.patch

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

I think the patch is fine - thanks. It would be helpful to put a todo comment in there about https proxying

But I think before it gets backported we need to check with some of the original filers. I have a nagging fear having watched the dups go by that ntlm auth to a proxy is one of the bigger use cases.

I'm not sure that backporting something to release we don't even have the infrastructure to really test well is something I would endorse. The existing state of affairs is unfortunate - but its a choice made based on a security bug and doesn't impact a huge population. I would hate to make a mistake in one of these patches and immediately impact a lot more people without at least some beta air time. (admittedly beta has proven to be not very useful for ntlm).

wrt https - yes on aurora now. There is actually a problem with 407 auth and it now that I was looking at today. (haven't filed the bug yet).

Comment 4

[Honza Bambas (:mayhemer)]

Attachment: v1.1

https://hg.mozilla.org/integration/mozilla-inbound/rev/4620f17be6bf

Comment 5

[Honza Bambas (:mayhemer)]

Anthony, can you please help here again with yet one more round of testing when this gets out to Nightly?  Thanks!

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4620f17be6bf

Comment 7

[Honza Bambas (:mayhemer)]

Hi Patrick and Boris, I'd like to ask you once again for testing this one more patch.  It's already been few days on Nightly (http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/), before we uplift I want to make sure nothing has been screwed up for you.

Thanks!

(Anthony is off)

Comment 8

[Patrick Weiden]

(In reply to Honza Bambas (:mayhemer) from comment #7)
> Hi Patrick and Boris, I'd like to ask you once again for testing this one
> more patch.  It's already been few days on Nightly
> (http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/),
> before we uplift I want to make sure nothing has been screwed up for you.
> 
> Thanks!
> 
> (Anthony is off)

Hey Honza,

I re-tested the current version of Nightly [timestamp "02-Jul-2014 11:25"] and everything works perfectly (with both prefs having their corresponding default value) for me!

Cheers,
Patrick

Comment 9

[Boris Glawe]

Hi Honza,

I'm accessing the same environment/server from the same client platform as Patrick (we're colleagues). Thus I didn't test it on my own, but I can confirm, that Patrick successfully tested the nightly build.

Best regards

Boris

Comment 10

[Honza Bambas (:mayhemer)]

Comment on attachment 8446508 [details] [diff] [review]
v1.1

Thanks!  Asking approval based on comment 8 and 9.

Approval Request Comment
[Feature/regressing bug #]: 1023748
[User impact if declined]: credentials can be sent to proxies through NTLMv1 weak schema (actually exposed to any eavesdropper)
[Describe test coverage new/current, TBPL]: comment 8
[Risks and why]: zero, we have recently disabled the NTLMv1 altogether, this is a complementary patch to re-enable it for SSL end-servers (tested this has not been broken)
[String/UUID change made/needed]: none

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8446508 [details] [diff] [review]
v1.1

I don't think we will do for a new release of 30. However, I am taking it for 31 because it is going to be an ESR release.

Comment 12

[Honza Bambas (:mayhemer)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #11)
> Comment on attachment 8446508 [details] [diff] [review]
> v1.1
> 
> I don't think we will do for a new release of 30. However, I am taking it
> for 31 because it is going to be an ESR release.

Yep, then also please drop the a-release? in bug 1023748, if that has not been decided by internal channel different way.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5c2429980323
https://hg.mozilla.org/releases/mozilla-beta/rev/b41f50d00655

Comment 14

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Verified on Firefox 33 based on comment 8 and comment 9.