452781 - Allow specifying wildcard that matches all simple netbiosnames in network.automatic-ntlm-auth.trusted-uris

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Reto Egeter]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-GB; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-GB; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1 (.NET CLR 3.5.30729)

This is a problem on intranets.
Some companies are using a lot of Netbios only names for Intranet websites.
This is because with IE they are automatically added to the "Intranet Zone" and NTLM authentican can be used.

With Firefox I have to add each of these Netbios names manually.
Some mechanism to automate this would be best.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Mitch]

According to this page:
https://developer.mozilla.org/En/Integrated_Authentication

These are other kinds of authentication that at least let you use a domain or tld. I would agree that wildcards would be great, but at least let us put a top level domain rather than the 100 different sub-domains we have on our corporate (nationwide mobile phone carrier) intranet.

This is one of the reasons why we don't use Firefox as a standard. Our team has to keep a list of URIs.

Steps to Reproduce:
1. go to about:config
2. edit network.automatic-ntlm-auth.trusted-uris with "mycompany.com" or maybe "*.mycompany.com" or maybe "*.mycompany.tld" (top level domain)
3. go to a mycompany.com site and still get prompted for NT login and password

Expected:

1. go to about:config
2. edit network.automatic-ntlm-auth.trusted-uris with "mycompany.com"
3. go to a mycompany.com site and not get prompted for NT login and password.

Comment 2

[Mitch]

sorry, I used "tld" and domain interchangeably. It's early. I just meant domain, not tld.

Comment 3

[Richard van den Berg]

Mitch, your expected result in comment #2 is the way it works now. The strings in network.automatic-ntlm-auth.trusted-uris are matched with the current URL from the right. So adding mycompany.com should work.

I would however love to have a way to add all hostnames that do not have a domain to network.automatic-ntlm-auth.trusted-uris. Urls like:

http://sharepoint1/
http://sharepoint2/
http://wiki/
http://cmdb/

Comment 4

[Scott B]

The proxy auth code now allows the use of <local> to indicate domains that are non-fqdn; this usage should be expanded to allow <local> to be placed in the network.automatic-ntlm-auth.trusted-uris

Microsoft IE's default behavior for NTLM auth is to allow NTLM on all domains that are non-fqdn (those are the domains considered 'intranet security zone'), and right now, there's no easy way to replicate that in Firefox.

Comment 5

[Richard van den Berg]

I checked nsWindowsSystemProxySettings.cpp and "<local>" is replaced by "localhost;127.0.0.1" so this is not entirely the same thing.

I think it would be best to have a network.automatic-ntlm-auth.allow-non-fqdn and network.negotiate-auth.allow-non-fqdn preference to control this. I'll write a patch for this, as it is rather easy to implement.

Comment 6

[Richard van den Berg]

Attachment: Possible fix

This is my first Firefox patch, please be kind.

Comment 7

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

Comment on attachment 593058 [details] [diff] [review]
Possible fix

Thanks for the patch, Richard!

I think you want to use host.FindChar('.') != kNotFound instead of strchr.

I'll flag someone familiar with network code to provide feedback here.

Comment 8

[Honza Bambas (:mayhemer)]

Comment on attachment 593058 [details] [diff] [review]
Possible fix

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

In general, looks good!

Few nits:
- update to the latest mozilla-central code
- use bool instead of PRBool
- always have space after |if|
- as Gavin suggests, use host.FindChar('.') != kNotFound  ->  means "host is FQDN"

::: mozilla-release/extensions/auth/nsHttpNegotiateAuth.cpp
@@ +334,5 @@
>      return val;
>  }
>  
>  PRBool
> +nsHttpNegotiateAuth::TestNonFqdn(nsIURI *uri, const char *pref)

I'd not pass the pref here as an arg.  Just hard code it in the method.

@@ +347,5 @@
> +
> +    if(strchr(host, '.'))
> +        return PR_FALSE
> +    else
> +        return PR_TRUE

Hmm.. missing semicolons :)

::: mozilla-release/modules/libpref/src/init/all.js
@@ +1048,5 @@
>  // system's NTLM implementation to silently authenticate the user with their
>  // Window's domain logon.  The trusted-uris pref follows the format of the
>  // trusted-uris pref for negotiate authentication.
>  pref("network.automatic-ntlm-auth.allow-proxies", true);
> +pref("network.automatic-ntlm-auth.allow-non-fqdn", true);

I'd call this 'non-fqdn-trusted' to emphasize we want to auto neg with them and that the functionality is the same as fqdn names would be added to 'trusted-uris'.

::: mozilla-release/netwerk/protocol/http/nsHttpNTLMAuth.cpp
@@ +226,5 @@
> +        PRBool val;
> +        if (NS_FAILED(prefs->GetBoolPref(kAllowNonFqdn, &val)))
> +            val = PR_FALSE;
> +        LOG(("Default credentials allowed for non-fqdn: %d\n", val));
> +        if (val == PR_TRUE)

Better as:

if (NS_SUCCEEDED(prefs->....(..&val)) && val) {
  LOG(...)
  return true;
}

Comment 9

[Richard van den Berg]

Attachment: Possible fix

Thanks for the review. I've followed all suggestions except for the name of the prefs. Your suggestion 'non-fqdn-trusted' is inconsistent with the 'allow-proxies' prefs. I believe 'allow-non-fqdn' more clearly describes the effect of the prefs.

Comment 10

[Honza Bambas (:mayhemer)]

Comment on attachment 595550 [details] [diff] [review]
Possible fix

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

Found some issues.

r-

::: extensions/auth/nsHttpNegotiateAuth.cpp
@@ +144,5 @@
>          proxyInfo->GetHost(service);
>      }
>      else {
> +        bool allowed = TestNonFqdn(uri);
> +	if (!allowed)

No tabs please, use only spaces.

@@ +347,5 @@
> +
> +    if (host.FindChar('.') != kNotFound)
> +        return false;
> +    else
> +        return true;

No else after return.  You can also just:

return host.FindChar('.') == kNotFound;


However, if the host is an IPv6 address, it will be qualified as non-fqdn too, and that is very bad.

You have to add a check the host is not an IP address when there is not any dot in the name.

There is PR_StringToNetAddr function for the check.  If it returns PR_SUCCESS, then the string is an IP address and you have to return false.

::: modules/libpref/src/init/all.js
@@ +1035,5 @@
>  // This list controls which URIs can support delegation.
>  pref("network.negotiate-auth.delegation-uris", "");
>  
> +// Allow SPNEGO by default when challenged by a local server.
> +pref("network.negotiate-auth.allow-non-fqdn", true);

By default this has to be false.  This pref is on-demand, it is not safe to have it on by default.  Same as the list of trusted URIs it has to be set by user.

@@ +1067,5 @@
>  // system's NTLM implementation to silently authenticate the user with their
>  // Window's domain logon.  The trusted-uris pref follows the format of the
>  // trusted-uris pref for negotiate authentication.
>  pref("network.automatic-ntlm-auth.allow-proxies", true);
> +pref("network.automatic-ntlm-auth.allow-non-fqdn", true);

Same here

Comment 11

[Richard van den Berg]

Attachment: Possible fix

Thanks again for the review. I've applied the suggested changes.

About the default being false: on Windows this functionality is on by default for IE and Chrome, see http://dev.chromium.org/developers/design-documents/http-authentication . I can live with the default being false for Firefox though.

Comment 12

[Richard van den Berg]

Attachment: Possible fix

Now also uses PR_StringToNetAddr for NTLM authentication.

Comment 13

[Honza Bambas (:mayhemer)]

Comment on attachment 597347 [details] [diff] [review]
Possible fix

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

I'll find someone to have a second opinion on the default value for these preferences.  I know IE defaults to true, unless setup for "restricted sites".  We may do the same, I just feel we shouldn't.

::: extensions/auth/nsHttpNegotiateAuth.cpp
@@ +347,5 @@
> +    if (NS_FAILED(uri->GetAsciiHost(host)))
> +        return false;
> +
> +    // return true if host does not contain a dot and is not an ip address
> +    return host.FindChar('.') == kNotFound && PR_StringToNetAddr(host.get(), &addr) != PR_SUCCESS;

80 chars per line please, probably just wrap as following:

return host.FindChar('.') == kNotFound &&
       PR_StringToNetAddr(host.get(), &addr) != PR_SUCCESS;

::: netwerk/protocol/http/nsHttpNTLMAuth.cpp
@@ +226,5 @@
>      nsCOMPtr<nsIURI> uri;
>      channel->GetURI(getter_AddRefs(uri));
> +
> +    bool isNonFqdnTrusted = false;
> +    if (IsNonFqdn(uri)) {

Add check for |uri| (non-null).

@@ +228,5 @@
> +
> +    bool isNonFqdnTrusted = false;
> +    if (IsNonFqdn(uri)) {
> +        if (NS_FAILED(prefs->GetBoolPref(kAllowNonFqdn, &isNonFqdnTrusted)))
> +            isNonFqdnTrusted = false;

And maybe you should check the pref first before you even call IsNonFqdn (performance) and also you may just return when it is found trusted.

Comment 14

[Richard van den Berg]

Attachment: Possible fix

I've added all suggestions except the last one. I don't want to leave CanUseDefaultCredentials() without logging, so IsNonFqdn() needs to be called to be able to log that we allowed NTLM using the default credentials based on the host being non-fqdn.

Comment 15

[Honza Bambas (:mayhemer)]

(In reply to Richard van den Berg from comment #14)
> Created attachment 597431 [details] [diff] [review]
> Possible fix
> 
> I've added all suggestions except the last one. I don't want to leave
> CanUseDefaultCredentials() without logging, so IsNonFqdn() needs to be
> called to be able to log that we allowed NTLM using the default credentials
> based on the host being non-fqdn.

You can add an extra log before return ;)

Comment 16

[Richard van den Berg]

I think I see what you mean. Something like this?

    bool allowNonFqdn = false;
    if (NS_FAILED(prefs->GetBoolPref(kAllowNonFqdn, &allowNonFqdn)))
        allowNonFqdn = false;
    if (allowNonFqdn && uri && IsNonFqdn(uri)) {
        LOG(("Host is non-fqdn, default credentials are allowed\n"));
        return true;
    }

Comment 17

[Honza Bambas (:mayhemer)]

Yep, that's it.  You just don't need to init allowNonFqdn to false.

Comment 18

[Richard van den Berg]

Attachment: Possible fix

Check pref before testing uri in nsHttpNTLMAuth.cpp

Comment 19

[Richard van den Berg]

Attachment: Possible fix

Forgot to label it as a patch.

Comment 20

[Honza Bambas (:mayhemer)]

Boris, Brian: the patch introduces a pref to allow ntlm and negotiate autologon using Windows SSPI for non-fqdn hosts (i.e. hosts in local intranet w/o dots in the name).

I proposed the pref should be off by default.  However, IE in default setting allows this on by default.

As this could be potentially dangerous (credential leak), I believe it needs user explicit consent to allow.

Comment 21

[Boris Zbarsky [:bzbarsky]]

I'm not sure I know enough about this stuff to have an opinion....

Comment 22

[Jason Duell]

cc'ing Jim Mathies, who's been doing a lot of NTLM stuff and might have an opinion.

Comment 23

[Jim Mathies [:jimm]]

I would default this to false for now. Admins that distribute fx can customize this, and individual users will figure it out. If after a number of releases we don't see issues with the use of the feature, we can flip it over.

Comment 24

[Scott B]

I would be another good enterprise feature if this also worked automagically for the current local [Active Directory] domain as well as non-fqdn.

So if you're logged into the "CORPORATE.LOCAL" domain, that domains that end with "CORPORATE.LOCAL" should also NTLM auth; without having to add *.CORPORATE.LOCAL to the allowed list.  This can be done with configs right now, but the corporate friendly thing to do is to do it automatically.

Comment 25

[Richard van den Berg]

Scott B: I agree that allowing NTLM/SPNEGO for the current local domain would be great, but that is another use case. This bug is about the non-fqdn case. That is not possible with the current FF functionality. As you mention, domains can be added manually, so your request is to automate that for the local domain. Please open a new bug for that request.

Comment 26

[Richard van den Berg]

@Jim: I'm fine with leaving this off by default. Is the current version of the patch acceptable?

Comment 27

[Jim Mathies [:jimm]]

Comment on attachment 597442 [details] [diff] [review]
Possible fix

Honza has already reviewed this pretty completely - seeking his final r+ on the code.

Comment 28

[Honza Bambas (:mayhemer)]

Comment on attachment 597442 [details] [diff] [review]
Possible fix

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

Thanks for the patch!

r=honzab with last few details addressed.

::: extensions/auth/nsHttpNegotiateAuth.cpp
@@ +348,5 @@
> +        return false;
> +
> +    // return true if host does not contain a dot and is not an ip address
> +    return host.FindChar('.') == kNotFound &&
> +           PR_StringToNetAddr(host.get(), &addr) != PR_SUCCESS;

As I'm thinking of it, we should also add a check the |host| is not empty: !host.IsEmpty()

Instead of host.get() do host.BeginReading() please.

::: netwerk/protocol/http/nsHttpNTLMAuth.cpp
@@ +133,5 @@
> +        return false;
> +
> +    // return true if host does not contain a dot and is not an ip address
> +    return host.FindChar('.') == kNotFound && 
> +           PR_StringToNetAddr(host.get(), &addr) != PR_SUCCESS;

As well here.

Comment 29

[Richard van den Berg]

Attachment: Possible fix

Please update the documentation at https://developer.mozilla.org/En/Mozilla_internal_string_guide regarding the use of get(). In the current FF code get() is used 6734 times, and BeginReading() 137 times. get() simply returns BeginReading() which should be preferred.

Comment 30

[Richard van den Berg]

Attachment: Possible fix

Test !host.IsEmpty() first. Simplied setting |allowed| in nsHttpNegotiateAuth.cpp

Comment 31

[Honza Bambas (:mayhemer)]

Comment on attachment 598187 [details] [diff] [review]
Possible fix

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

Thanks.

r=honzab

Let's get it in.

Comment 32

[Honza Bambas (:mayhemer)]

Comment on attachment 598187 [details] [diff] [review]
Possible fix

https://hg.mozilla.org/integration/mozilla-inbound/rev/479abf6b3598

Comment 33

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/479abf6b3598

Thanks for the patch! Join us on irc (#developers on irc.mozilla.org) and we'll find some other things for you to work on if you are interested? :-D

Comment 34

[Jim Mathies [:jimm]]

Should this get some sort of support/dev-doc-needed keyword, or someone cc'd in so it gets published?

Comment 35

[c610165]

Sorry for the dumb question, but when will this feature make it into a released version of Firefox? I assume I have to compile Firefox myself currently in order to use this feature, correct?

Comment 36

[Boris Zbarsky [:bzbarsky]]

This will be in Firefox 13.

You can get a build with this feature today by downloading a Beta (soon to be released as Firefox 13) or Aurora (soon to be released as Firefox 14) build from http://www.mozilla.org/en-US/firefox/channel/#beta or http://www.mozilla.org/en-US/firefox/channel/#aurora respectively.

Comment 37

[Josh Matthews [:jdm]]

Presuming this blocking removal was accidental.

Comment 38

[Richard van den Berg]

Now that this patch has landed in FF 13, it would be great if it could be included in FF 10 ESR. The whole reason for this patch is to accommodate corporate users.

Comment 39

[Lukas Blakk [:lsblakk] use ?needinfo]

[Triage Comment]

The FF13 train has already left the station so I'm tracking this for the release alongside FF14 on July 17th.  Please nominate for ESR landing approval.

Comment 40

[Lukas Blakk [:lsblakk] use ?needinfo]

bumping this back to tracking nomination - too late for 14.

Comment 41

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Richard van den Berg from comment #38)
> Now that this patch has landed in FF 13, it would be great if it could be
> included in FF 10 ESR. The whole reason for this patch is to accommodate
> corporate users.

If this is still wanted for ESR, it needs to be nominated for uplift to the ESR branch - see https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more details.

Comment 42

[Richard van den Berg]

Yes, this is till wanted for ESR. Who can do the uplift and set the appropriate tracking flags? Personally I cannot set status-esr10 to affected or checkin-pending, or set the approval-mozilla-esr10 flag at all.

Comment 43

[Lukas Blakk [:lsblakk] use ?needinfo]

Approving for ESR, please go ahead and land to that branch as per the wiki intructions in comment 41.

Comment 44

[Lukas Blakk [:lsblakk] use ?needinfo]

Setting checkin-needed, this needs to land to http://hg.mozilla.org/releases/mozilla-esr10/ this week.

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr10/rev/4d94b20d0322

Comment 46

[Ioana (away)]

Mozilla/5.0 (Windows NT 6.1; rv:10.0.7) Gecko/20100101 Firefox/10.0.7 (20120824135927)

In about:config I have:
network.automatic-ntlm-auth.allow-non-fqdn;true
network.negotiate-auth.allow-non-fqdn;true
network.automatic-ntlm-auth.trusted-uris;http://wiki/

When going to http://wiki/, the page gets opened but I am not logged in nor do I get any error messages regarding wrong credentials or anything else.

This is the same behavior as on Firefox 10.0.5 ESR, where the first two prefs in the list above don't exist. 

Perhaps I didn't understand this bug fix right. What I am supposed to see different after setting the above settings in about:config?

Comment 47

[Richard van den Berg]

Ioana, can you post a NSPR log file to see what is going on?

Some info on NSPR logging here:
http://www.mozilla.org/projects/netlib/http/http-debugging.html

Please set at least NSPR_LOG_MODULES=negotiateauth:5

Comment 48

[Richard van den Berg]

Is your wiki site using NTLM or negotiate authentication? Just to be sure, set NSPR_LOG_MODULES=nsHttp:3,negotiateauth:5,NTLM:5

Comment 49

[Ioana (away)]

Attachment: NSPR log file

The prefs were set as in comment 46 in a previous session. The log file information is only for opening the browser and going to http://wiki/.

Please let me know if you need me to take any other steps.

Comment 50

[Richard van den Berg]

The site in the log does not use NTLM or negotiate (SPNEGO) authentication. It probably uses a custom form on the site for authentication. Using the NTLM/SPNEGO scheme the server sends a "401 Unauthorized" HTTP response and sets the WWW-Authenticate HTTP header. None of this is present in your trace. NTLM/SPNEGO authentication is typically found on Microsoft Windows servers, but there are open source implementations as well. See http://msdn.microsoft.com/en-us/library/ms995330.aspx

Comment 51

[Ioana (away)]

It seems we don't have here the pre-requisites to verify this bug. Richard, if you do have access to the necessary, please try to verify it. If somebody else can help, that would be appreciated. Thanks

Comment 52

[Richard van den Berg]

I can confirm that this patch is included in ff 10.0.7esr build1 and that it is working as expected.