91968 - HTTP authentication stores rejected user/pass combos in password manager

Status: VERIFIED FIXED

(Core :: Networking: HTTP, defect)

Description

[Robin Green]

If you log on to a site using HTTP AUTH authentication (i.e. not web-form-based
authentication), using an incorrect user/password combination, and the server
rejects it, the user/password combination is still saved by Password Manager.
See test URL above. 

I was instructed to file this under networking by [e-mail] in bug 91953.

Related to bug 62363.

Comment 1

[Keyser Sose]

Marking NEW.

Comment 2

[Nivedita (gone)]

That means that the Password Manager should store the username/password  
on integrity of the combination depending  upon the OK response of the request 
using it. If the specification of PasswordManager says that only the  valid 
username and password need to be stored then we would need to do the validation 
else the current behaviour is fine. Please convey your thoughts on above.

Comment 3

[Robin Green]

Yes, what you said. I cannot see why it would be useful to store invalid
username/password combinations.

Comment 4

[Nivedita (gone)]

Attachment: Patch file for rejecting the storage of invalid username and password in password manager

With the application of this patch the invalid username and password
combinations will not be stored by the password manager. This achieved by the
adding two new interfaces:
a) fecthing the username and password
b) storing the valid username and password

The first interface fetches us the username and password from the user,which is
used by the later if the authentication request succeeds and the user has
wishes to store them. The username and password data is freed on the successful
storage or in failed conditions.

Comment 5

[neeti]

cc'ing darin

Comment 6

[Darin Fisher]

the user enters his username and password.  we accept that as correct until the
server tells us otherwise.  doing so makes sense since there could be multiple
parallel requests that all require the same authentication.  in other words, we
optimize for the case in which the user has successfully entered the correct
username and password.

okay, but what if that username and password are incorrect.  currently, password
manager remembers the incorrect values and prompts the user with those values
when the auth prompt is subsequently displayed.  one reason why this might be
nice is if the user mistyped their username.  they could see right away that
they are being reprompted because of a typo.  however, i tend to agree that we
should consider this a bug and fix it.

as far as the patch is concerned it seems overly complicated.  take a look at
how FTP handles failed logins.  it uses the observer service to communicate w/
the password manager.  see nsFtpConnectionThread.cpp:1106

Comment 7

[Nivedita (gone)]

Attachment: patch file for rejecting the storage of invalid username and password using the observer event "login-failed"

Notifying the wallet service for the "login-failed" event in case of
authentication failure in http request.

Comment 8

[Nivedita (gone)]

The patch (id=65946) is as a result of disussion between Darin, Vinay and 
nivedita. 

Rick, can you please verify if the patch is in sync with the integration of 
other components involved.

Comment 9

[Darin Fisher]

nivedita:

actually, i have some problems with this patch:

1) indentation is inconsistent, but more importantly...

2) no need to store mAuthData.  this just bloats the http channel.  instead, you
could notify "login-failed" observer(s) just before calling
PromptUsernameAndPassword.  this way you would not have to generate |domain| twice.

3) your patch breaks "login-failed" for FTP.

4) why are you trying to decode the realm from |someData|?  this should be
unnecessary.  you should be able to use |someData| directly.  or, am i mistaken?

Comment 10

[Nivedita (gone)]

Attachment: patch file incorporating the comments given by Darin

patch file incorporating the comments given by Darin.

Comment 11

[Darin Fisher]

cc'ing morse.

morse: i just stumbled on nsIPasswordManager.idl in netwerk/base/public.  it
appears to be implemented by wallet.  can it be used to solve this bug?  we want
to be able to remove a user/pass combo without depending on wallet at compile time.

Comment 12

[Darin Fisher]

nivedita: you still should not be using mAuthData.  that adds 64+ bytes to
nsHttpChannel.  it does not need to be a member variable.  it should be
allocated on the stack.

Comment 13

[Stephen P. Morse]

darin wrote:
> morse: i just stumbled on nsIPasswordManager.idl in netwerk/base/public.  it
> appears to be implemented by wallet.  can it be used to solve this bug?  we
> want to be able to remove a user/pass combo without depending on wallet at
> compile time.

Exactly.  And that is what this patch is using.  There is no compile-time 
dependency on wallet in this patch -- it is using an obsserver exactly as it 
should be doing.

I don't yet understand yet why this patch requires modifications to the 
nsWalletService, so I'm about to look into that.  I should have been copied on 
this report long before now. :-(

Comment 14

[Darin Fisher]

morse: no the patch is not using nsIPasswordManager directly.  instead it is
using the observer interface via the "login-failed" event.  the problem is that
HTTP needs to be able to clear only a specific username from the password database.
using the observer interface makes this difficult.  nsIPasswordManager would
make this easier.  can that be used to achieve the same thing as the
observer-based solution?

Comment 15

[Stephen P. Morse]

My comment was confusing.  That's what I meant to say -- it is using the 
observer-based interface and not the compile-time one.

So you are suggesting using the compile-time one instead so that you can 
specify the username instead of having wallet decipher it.  Yes, that sounds 
clean.  Much cleaner than putting in all that special-case code into wallet 
which the patch was doing (I've now read it and understand it).  And the 
compile-time dependency is not on the extensions directory so you should be OK.

Comment 16

[Darin Fisher]

morse: thanks for clearing this up!

Comment 17

[Nivedita (gone)]

Attachment: patch file making use of nsIPasswordManager for deleting the rejected username/password

Using nsPasswordManager for deleting the invalid username/password. 

Darin,
I have retained the nsAutoString mAuthData, for storing the username, as it is
needed for deletion of this specific user, for the given realm. Passing null as
username, does not delete any user. RemoveUser method compares the passed
username against the username list for the given realm. Hence if the username
is passed null, the comparision fails, hence ends up in not deleting the user. 


In nsHttpChannel, we need to remember the username to pass it down to the
nsPasswordManager, to successfully delete the invalid user. Hence, mAuthData is
essential. We could however reduce the bloat by using PRUnichar instead of
nsAutoString.

Comment 18

[neeti]

nivedita

Comment 19

[Darin Fisher]

nivedita: what about using mUser or mProxyUser instead?  however, if you must
introduce a new string member variable, do not use nsAutoString... use nsString
or nsSharableString instead.  nsAutoString bloats nsHttpChannel by 128 bytes
independent of the actual string length.  nsAutoString should only ever be
allocated on the stack.

Comment 20

[Nivedita (gone)]

Attachment: patch file incorporating the comments given by Darin

mUser or mProxyUser does not store the username which is required to be passed
down to RemoveUser. mUser or mProxyUser has the data picked up from the passed
URL, through the method invocation GetUserPassFromURI, hence does not contain
the relevant data for us. 

I have changed the member variable mAuthData from nsAutoString to nsString.

Comment 21

[Darin Fisher]

mUser and mProxyUser do contain the information you desire.  they are set by the
call to both GetUserPassFromURI and PromptForUserPass.  also it should be ok to
RemoveUser on the user:pass taken from the URI, so no need to even distinguish
these.

Comment 22

[Nivedita (gone)]

Attachment: patchFile incorporating Darin's comments

Incorporated comments given by Darin. Passing the  data present in
mUser/mProxyUser to RemoveUser.

Comment 23

[Darin Fisher]

Comment on attachment 68117 [details] [diff] [review]
patchFile incorporating Darin's comments

>Index: protocol/http/src/nsHttpChannel.cpp

>+            nsCAutoString domain;
>+            domain.Assign(host);
>+            domain.Append(':');
>+            domain.AppendInt(port);
>+
>+            domain.Append(" (");
>+            domain.Append(realm);
>+            domain.Append(')');
>+
>+            nsresult rv;
>+            nsCOMPtr<nsIPasswordManager> passWordManager = do_GetService(NS_PASSWORDMANAGER_CONTRACTID, &rv);
>+            if (passWordManager)
>+                passWordManager->RemoveUser(domain.get(), entry->User());

Can you move this code into a subroutine?  I think GetCredentials() is getting
to be too long.

For example:

  void nsHttpChannel::ClearPasswordManagerEntry(const char *host, PRInt32 port,
const char *realm);

otherwise, this looks very good.  sr=darin with that change.  please attach a
new patch.  thx!

Comment 24

[Darin Fisher]

one more nit:  there's no reason to compute the domain if the password manager
doesn't exist, so move the domain computation into the if (passwordManager) check.

Comment 25

[Nivedita (gone)]

Attachment: patch file incorporating the comments given by Darin

Incorporated comments given by Darin. Put the patch in a helper function

Comment 26

[Darin Fisher]

Comment on attachment 68280 [details] [diff] [review]
patch file incorporating the comments given by Darin

>Index: protocol/http/src/nsHttpChannel.h

>+    void     ClearPasswordManagerEntry(const char *host, PRInt32 port,const char *realm, const PRUnichar* user);

this patch looks great, but please clean up the whitespace in the argument
list.

with that, sr=darin

Comment 27

[Nivedita (gone)]

Attachment: patch file with white space in the argument list

I have added the missed out white space in the argument list of the function
declaration of ClearPasswordManagerEntry. This is in sync with the existing
function declarations in the class, which have a white space after each
argument in the argument list.

Comment 28

[Darin Fisher]

Comment on attachment 68292 [details] [diff] [review]
patch file with white space in the argument list

sr=darin

nice work on this patch by the way :-)

Comment 29

[Stephen P. Morse]

Comment on attachment 68292 [details] [diff] [review]
patch file with white space in the argument list

r=morse

Comment 30

[Nivedita (gone)]

Fix committed into cvs by Vinay for me.

Checking in src/nsHttpChannel.cpp;
/cvsroot/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp,v  <--  nsHttpChann
el.cpp
new revision: 1.88; previous revision: 1.87
done

Comment 31

[John Unruh]

Verified using in-house server aka on Win2000, Linux and OSX