32335 - basic auth must use realm

Status: VERIFIED FIXED

(Core :: Networking, defect, P2)

Description

[Gagan]

currently we are ignoring this.

Comment 1

[Warren Harris]

Ok, I'll bite. What is 'realm'?

Comment 2

[Warren Harris]

Moving to M17 which is now considered part of beta2.

Comment 3

[Gagan]

a web site can have "realms" to control access to different areas. Its a
mechanism to share access and is provided in the basic auth. We are ignoring it
currrently. 

Comment 4

[Gagan]

*** Bug 33606 has been marked as a duplicate of this bug. ***

Comment 5

[Richard Zach]

*** Bug 36376 has been marked as a duplicate of this bug. ***

Comment 6

[Gagan]

*** Bug 25868 has been marked as a duplicate of this bug. ***

Comment 7

[Gagan]

*** Bug 34661 has been marked as a duplicate of this bug. ***

Comment 8

[Linda Baliman]

Added Demain Turner to cc-list.  This is a duplicate of their bug 35753, the 
double-signon bug.

Comment 9

[msanz]

removing nsbeta2 because it is annoying but not something we would stop the beta 
for

Comment 10

[msanz]

removing nsbeta2 because it is annoying but not something we would stop the beta 
for. Linda, please let us know if this is different than having to log twice

Comment 11

[leger]

Putting on [nsbeta2-] radar. Not critical to beta2.

Comment 12

[Gagan]

*** Bug 40123 has been marked as a duplicate of this bug. ***

Comment 13

[Gagan]

*** Bug 38096 has been marked as a duplicate of this bug. ***

Comment 14

[bnb]

There are occasions when this is more than simply having to login twice. If the 
second directory is just used for <IMG>'s then mozilla doesn't prompt you for a 
username/password it just doesn't show the <IMG>'s.

e.g.

Put the HTML in /html/private/ and the images in /images/private/

and IMG tages in /html/private/ referencing /images/private don't work

Comment 15

[Gagan]

*** Bug 47986 has been marked as a duplicate of this bug. ***

Comment 16

[Gagan]

per triage team, nsbeta3+

Comment 17

[Gagan]

*** Bug 49168 has been marked as a duplicate of this bug. ***

Comment 18

[Dan Nunn]

*** Bug 50251 has been marked as a duplicate of this bug. ***

Comment 19

[Phil Peterson]

Gagan, PDT is wondering if there are any security holes here? Can this give away
the password for one realm to another? If so, it seems like a P1; if no there's
no vulnerability here, it seems like a P2 or P3.

Comment 20

[Gagan]

No security holes here. Since we just don't look at realms currently the problem
is that we land up reauthenticating (or failing) for a common realm case. Its P1
becuz it completely makes basic auth unusable on several auth sites.

Comment 21

[Michael La Guardia]

setting as P2 per PDT

Comment 22

[Gagan]

*** Bug 52138 has been marked as a duplicate of this bug. ***

Comment 23

[Phil Peterson]

Not holding PR3 for this. Marking nsbeta3- but adding rtm nomination since this 
apparently hurts enough to fix after PR3.

Comment 24

[Jason Summers]

The issue [e-mail] reported does not seem to be necessarily related 
to realms. AFAICT, Mozilla *never* asks the user for a password for an embedded 
image, even if the main document is not password-protected at all. It just 
silently fails to display the image.

Comment 25

[Gagan]

approving to work for rtm... ccing darin. 

Comment 26

[Darin Fisher]

Attachment: This patch adds support for HTTP realms.

Comment 27

[Darin Fisher]

This patch does not fix the problem reported by [e-mail].
That one is a separate issue... see bug 56031.

Comment 28

[Gagan]

this looks good except for one minor issue. The GetAuthRealm function should 
create a copy of the authrealm in the out parameter. Other than that r=gagan. 

Comment 29

[Darin Fisher]

Attachment: Revised patch with fix to nsHTTPChannel::GetAuthRealm()

Comment 30

[Darin Fisher]

Attachment: Revised patch to correctly check mPrompter _only if_ realm lookup fails.

Comment 31

[Gagan]

ok all this looks great. r=gagan and moving to a rtm+

Comment 32

[Phil Peterson]

This is a big patch, so in order to take this on the branch, we need testing 
verification that the bug is fixed, and no regressions introduced. QA, please 
report back when this info is available. Marking [rtm need info]

Comment 33

[Mike Shaver (:shaver emeritus)]

Should basic-auth realm code really be in the nsHTTPChannel, rather than in the
nsBasicAuth?  What do digest and NTLM auth do for realms?

Also, you seem to be prompting for username and password unconditionally: I'm
not sure that's the right thing, since there exist some auth mechanisms that
don't have to prompt the user.

Comment 34

[Darin Fisher]

Attachment: Latest patch: minor changes over previous; tested on trunk.

Comment 35

[Mike Shaver (:shaver emeritus)]

I finally understand your patch.  Thanks to gagan for explaining things in small
words.

r=shaver

Comment 36

[Gagan]

I landed this fix on the trunk last night. 

Comment 37

[Matthias Versen [:Matti]]

I have posted a dupe of this bug (Bug 49168).
I have Build 20000101320 from Friday.
I have problems to access the page posted in the bug 49168.
(http://informer2.comdirect.de/login.html?doauth=true&callerPage=/de/my/homepage/main.html)

No Username/password dialog is coming up.
Is this another problem or is your fix not correct or i´m stupid :-)

Comment 38

[Darin Fisher]

Ok, I am seeing this problem too under Linux build 2000101509.
->investigating...

Comment 39

[Brendan Eich [:brendan]]

I'm reviewing late, but wonder who gave a= (super-review)?  No sign of mscott or 
a delegate giving approval.  Cc'ing mscott and scc for backup.

Looks like a leak here:

+    for (PRInt32 i = count-1; i>=0; --i)
+    {
+        nsAuth* auth = (nsAuth*) mAuthList->ElementAt(i);
+        
+        nsXPIDLCString authHost;
+        PRInt32 authPort;
+
+        auth->uri->GetHost(getter_Copies(authHost));
+        auth->uri->GetPort(&authPort);
+
+        if ((0 == nsCRT::strcasecmp(authHost, host)) &&
+            (0 == nsCRT::strcasecmp(auth->realm, i_Realm)) &&
+            (port == authPort))
+        {
+            *o_AuthString = nsCRT::strdup(auth->encodedString);
+            return (!*o_AuthString) ? NS_ERROR_OUT_OF_MEMORY : NS_OK;
+        }
+    }
+    return NS_ERROR_FAILURE;

mAuthList is an (nsCOMPtr to an) nsISupportsArray, and 
nsSupportsArray::ElementAt NS_ADDREFs its result param if non-null, as usual for 
XPCOM out parameters.  So fix by using an nsCOMPtr<nsAuth> here (it's ok to use 
a concrete type with nsCOMPtr if that type inherits singly from nsISupports, as 
nsAuth does), but take care to avoid an "extra" AddRef:

+        nsCOMPtr<nsAuth> auth = dont_AddRef(NS_STATIC_CAST(nsAuth*, 
mAuthList->ElementAt(i)));

Please fix the other two places that leak in this way.

Minor bloat here:

+            if (NS_SUCCEEDED(mURI->GetHost(getter_Copies(hostname))))
+            {
+                // TODO localize
+                message.AppendWithConversion(" at ");
+                message.AppendWithConversion(hostname);
+            }

Use message.Append(NS_LITERAL_STRINCT(" at ")) instead.

Nit-picking from here on, these problems are obviously less important than leaks 
and bloat, but they're still worth fixing for code quality:

+NS_IMETHODIMP
+nsHTTPChannel::GetAuthRealm(char** aAuthRealm)
+{
+    if (aAuthRealm)
+    {
+        *aAuthRealm = nsCRT::strdup(mAuthRealm);
+        return (*aAuthRealm) ? NS_OK : NS_ERROR_OUT_OF_MEMORY;
+    }
+    else
+        return NS_ERROR_NULL_POINTER;
+}

else after return is a non-sequitur, and the error return might better be the 
overindented case in this function:

+NS_IMETHODIMP
+nsHTTPChannel::GetAuthRealm(char** aAuthRealm)
+{
+    if (!aAuthRealm)
+        return NS_ERROR_NULL_POINTER;
+    *aAuthRealm = nsCRT::strdup(mAuthRealm);
+    return (*aAuthRealm) ? NS_OK : NS_ERROR_OUT_OF_MEMORY;
+}

Then the only foolish consistency change would be to use ! and put the NS_OK at 
the end, as you did elsewhere:

+            return (!*o_AuthString) ? NS_ERROR_OUT_OF_MEMORY : NS_OK;

Brace style in the modified parts of the patch is discordant with the file's 
prevailing style (which puts opening brace on same line as if, e.g.):

+        // Skip prompting if we already have an authentication string.
+        if (!authString || !*authString)
+        {
+            // Delay this check until we absolutely would need the prompter
+            if (!mPrompter) {
+                NS_WARNING("Failed to prompt for username/password: 
nsHTTPChannel::mPrompter == NULL");
+                return NS_ERROR_FAILURE;
+            }

Why not follow "When in Rome" and avoid mixing styles?  It's recommended at 
http://www.mozilla.org/hacking/reviewers.html and it helps readability and 
ownership, no matter whose style is being upheld (if you are taking ownership, 
revise the whole file's style; if you have a share in ownership, get buy-in and 
make style changes; otherwise the prevailing style should be observed).

Also, tabs in files are evil (because people and programs on various platforms 
cannot agree on what tabstops they advance to), and violate the Emacs modeline 
comment.  It looks like some of these were already there before your patch, but 
could you fix 'em all anyway?

@@ -2315,10 +2374,21 @@
                                {
                                        rv = 
GetRequestHeader(nsHTTPAtoms::Authorization,
                                                        
getter_Copies(authString));
-                                       pEngine->SetAuthString(mURI, 
authString);
+#ifdef DEBUG_darin
+                    fprintf(stderr, "\n>>>>> Auth Accepted!! [realm=%s, 
auth=%s]\n\n", mAuthRealm, (const char*) authString);
+#endif
+                                       if (mAuthRealm)
+                                               
pEngine->SetAuthStringForRealm(mURI, mAuthRealm, authString);
+                    else
+                                               pEngine->SetAuthString(mURI, 
authString);
                                }

Thanks,

/be

Comment 40

[Mike Shaver (:shaver emeritus)]

I feel guilty about getting us to this point.  My very first review stopped when
I (thought I) saw problems with non-basic-auth, but when those problems were
explained away by gagan, I didn't return to finish the job, and gave an r=
inappropriately.  Even if I didn't point out style woes -- which I should have
-- there's no excuse for missing the leaks and string-conversion bloat.

I promise to do a better job with your next patch, Darin, once you address
brendan's concerns.

Comment 41

[Darin Fisher]

Using the latest CVS trunk (2000-10-16 12:19 PST), I've located the
source of the problem described in bug 49168.  The behavior is different
than that originally reported because of this patch.

The problem we are now seeing results when the site, mentioned in bug 49168,
responds with a 301 redirection after being properly authenticated.  The new URL
does not "match" any previously authenticated URLs, so we do not send an
authentication header with the request for the new URL.  (We are expecting
the server to respond with 401, if authentication is required).  The servers
response for the new URL is 301 (not 401) with a location back to the original
URL.  Thus, mozilla drops into an infinite loop.

Netscape 4.X does not have this problem.  Although, I haven't been able to
confirm it from the source, it seems 4.X is sending the authentication acquired
after the first 401 on successive 301 redirects, thus avoiding the loop.

Despite this problem, I thought we were already checking for infinite redirects.
Anybody know how this could slip through?  Perhaps, it is due to the intermitent
401?  I'll have to look at the code more carefully.

Brendan: thanks for pointing out those improvements to the patch.

Comment 42

[Gagan]

This is clearly a bad HTTP implementation that we have unintentionally
encouraged with 4.*. A redirect should NOT carry original auth info. Possible
security hazards are scary. Our best bet in our current scenario is to simply
add the check for this goofy cyclic redirect. Ruslan has put in some code to
detect that.

Thanks for the great review brendan (since I had reviewed it, we "converted"
shaver's review to a super-review) My bad! I will try and be more careful next
time.

Comment 43

[Darin Fisher]

Attachment: Revised patch to be applied to trunk.

Comment 44

[Darin Fisher]

Attachment: Revised patch to be applied to branch.

Comment 45

[Darin Fisher]

The patches I just posted fixes the leaks, bloat, and (some of the) style
problems pointed out by Brendan.

Comment 46

[Brendan Eich [:brendan]]

+        nsAuth* auth = NS_STATIC_CAST(nsAuth*, mAuthList->ElementAt(i));
+

Why not use an nsCOMPtr?  Adding NS_RELEASEs is busy-work that someone will
later break by adding a control flow transfer around the necessary release,
without adding yet another release.  I feel pretty strongly about this, and so
do other reviewers.  Mozilla leaks too much memory, and not because of any one
leak.  Manual reference counting tends to result in more leaks that you get with
systematic use of smart pointers.

If you are taking ownership (you're asserting your brace style, but maybe other
netwerk hackers don't care about consistency there -- as a reader and potential
writer of netwerk code, I do), I hope you get around to using nsCOMPtrs in other
files, e.g. in nsHTTPHandler.cpp.

nsHTTPHandler::NewChannel in particular needs a cleanup.  Bogus indentation and
gratuitous gotos are telltales; so is the NS_ADDREF you removed.  When using
nsCOMPtrs, you do need to ADDREF the out param as you set it (this is a Mozilla
pattern, an ADDREF that people expect to see).  Without it, the reviewer has to
track the earlier ADDREF back to the ElementAt call, and check for necessary
releases (which you added).

Perhaps scc can say more, but his fine docs under
http://www.mozilla.org/projects/xpcom/, linked to under
http://www.mozilla.org/hacking/reviewers.html#rules-and-tips, are worth reading
if you haven't.

/be

Comment 47

[Scott MacGregor]

Hey Darin, this patch looks pretty good to me. One suggestion I had is the same
as Brendan's. You really want to leverage a nsCOMPtr with the nsAuth ptrs you
are getting from that nsISupportsArray. I know it may look a little strange
because you are dealing with an implementation class and not the interface but
it's perfectly legal as long as your object inherits singly from nsISupports.

This would clean up a lot of the NS_RELEASES you added and make the code more
leak proof if someone else comes along and modifies the code introducing another
return path. So there were a couple of spots were you should leverage that.

I'm not sure you need to clean up the constructor for nsHTTPHandler before
checking this patch in since you aren't introducing more gotos, or poor
identation stuf. It would certainly be great if someone would clean it up but I
think that's orthoganal to your changes. Or maybe I misunderstood Brendan's
comment and he really was pointing out something in your patch and not existing
code.

Oh one more thing. As a convience to you, if you made mAuthRealm a nsCString
instead of char * then you wouldn't have to worry about initializing it to null
and freeing the memory in the dtor of nsHTTPChannel.

Comment 48

[Darin Fisher]

Gagan and I discussed using nsCOMPtr's in nsAuthEngine.  However, the fact of
the matter is that without an IID for the nsAuth class you can't do this.
Instantiating a nsCOMPtr requires the GetIID method, which appears in interface
header files through the NS_DEFINE_STATIC_IID_ACCESSOR(iid) macro. We decided
that it would be less evasive to just do the NS_RELEASE's instead of defining a
new IID for the nsAuth _class_.

Ultimately, I don't think a nsISupportsArray is the correct container here.  The
only reason nsAuth derives from nsISupports is so that it can be used with
nsISupportsArray.  Perhaps, nsVoidArray would be more appropriate?  Then, we
would not have to mess around with reference counting at all.

Overall, there are many things that need to be cleaned up in this code. But, the
point right now (as I understand it) is to touch the code in the least evasive
way in order to fix the problems at hand.  Once we get past 6.0, then we will
have the opportunity to do a clean sweep.

Is (a) switching to nsVoidArray, (b) defining a new IID, or (c) littering the
code with NS_RELEASE's the correct thing to do at this time?

Comment 49

[Scott Collins]

The fact of the matter is, you are not allowed to get back to an implementation
class from an |nsISupports|, so plainly you need to do one of two things:

  (a) don't use an |nsISupportsArray|, don't do reference counting

  (b) make a new interface that supports everything you want, and use that

In this case, I agree with your assesment.  Follow (a).  Don't inherit from
|nsISupports|, don't use |nsISupportsArray|.  |nsVoidArray| may be the best
solution we have for you at the moment, though really a templated class a la
|std::vector<T>| would be best, and all casting could be avoided.  If you still
needed to manage lifetime (which you might avoid by storing objects instead of
pointers and exploiting value and copy semantics) you might use |auto_ptr| or
something home-grown.

Clearly, this code is stretching to use |nsISupportsArray| and having to twist
in unfortunate ways to do so.  It's trying to walk a middle line between the two
options above, and doing so very poorly.  Ignore the change, for the moment, I
wouldn't have let the original code in, for its wrong use of XPCOM.

Comment 50

[Darin Fisher]

Attachment: Revised patch to be applied to the Trunk, modified to use nsVoidArray.

Comment 51

[Darin Fisher]

After discussing this with Gagan, we decided that it would be best to switch
from using nsISupportsArray to nsVoidArray in the implementation of
nsAuthEngine.  The required changes to the code are minimal, since both
nsISupportsArray and nsVoidArray have similar interfaces.  The only big
difference is in nsAuthEngine::Logout, which must traverse the lists and
individually call delete on each nsAuth pointer.

I've tested this code on the trunk, and I believe that it is "safe" for
inclusion.  I'll shortly post a patch against the branch.

Comment 52

[Peter Trudelle]

PDT says rtm++, please land on branch ASAP.

Comment 53

[Brendan Eich [:brendan]]

Looks ok to me, mscott has dibs on sr=.

/be

Comment 54

[Darin Fisher]

Attachment: Revised patch to be applied to the branch.

Comment 55

[Scott MacGregor]

Darin, your patch against the branch looks good to me. I like the nsVoidArray
change.

sr=mscott

Comment 56

[Gagan]

All fixes checked in all relevant places... and all auth sites could 
authenticate happily ever after. Thanks Darin. 

Comment 57

[Tom Everingham]

verified on branch
WinNT4 2000102008
Linux rh6 2000102009
Mac os9 2000102008

needs verified on trunk 

Comment 58

[Tom Everingham]

verified on trunk
WinNT4 2000123120
Linux rh6 2000123106
Mac os9 2001010209