Closed Bug 15860 Opened 25 years ago Closed 23 years ago

[RFE] Digest authentication?

Categories

(Core :: Networking: HTTP, enhancement, P3)

Product:
Core
Component:
Networking: HTTP
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla0.9.7

People

(Reporter: anders, Assigned: darin.moz)

References

Details

(Keywords: relnote, testcase)

Attachments

(3 files, 16 obsolete files)

Test site for basic and digest authentication--http://digest-test.agranat.com/
2.62 KB, text/html
Details
v1.7
65.98 KB, patch
darin.moz
: review+
rpotts
: superreview+
Details | Diff | Splinter Review
fixes SEGV when interacting with squid (or probably any other proxy for that matter)
550 bytes, patch
Details | Diff | Splinter Review 
Mozilla should support the "digest" authentication scheme, in addition to
"basic" authentication.
Basic authentications has a LOT of shortcomings, among them that it sends
cleartext passwords. It also has no way of forcing a browser to "forget" a
password without restarting it.
Digest authentication solves most of these problems and has been available in
the Apache web server for years...

Digest authentication is specified in RFC 2617.
Status: NEW → ASSIGNED
Target Milestone: M16
2 months and no comment yet from the developers? As  Mozilla nears feature lock
for beta, I am worried that once again Mozilla/Netscape is ignoring yet another
security enhancment for God know what reason.
At least it got assigned to Milestone 16, whatever that is...
Bulk move of all Necko (to be deleted component) bugs to new Networking

component.
You might want to use the new MD5 implementation from Alladin instead of the
reference implementation.  The C code is a lot more modern and the license is
much more liberal.  Here is the Freshmeat entry:
http://www.freshmeat.net/appindex/1999/05/04/925811006.html
And since it has already been determined that export regs do not apply to
message signing (authentication) algorithms, there is no problem inplementing
this in Mozilla.
As if it weren't bad enough, SeaMonkey currently seems to use Basic
authentication even when server sends a Digest challenge only. So it is as
broken as Netscape 4.7... Oh well, M$ IE 5 doesn't behave correctly either,but
at least it is possible to use the digest auth there. Face it, guys: Sending
passwords out in the clear is as bad a security hole as they can be.
What kind of crap is this that my votes are removed from this bug? I guess 
Mozilla couldn't handle that people wanted certain bugs fixed and voted more 
than one vote to them.
Jerry, please take it to an appropriate forum, not here.
Keywords: beta2
Moving to M17 which is now considered part of beta2.
Target Milestone: M16 → M17
Whiteboard: 1w
Keywords: nsbeta2
Putting on [nsbeta2-] radar. Not critical to beta2.  Unless someone can put to a 
top site and a reprodicible testcase.
Whiteboard: 1w → [nsbeta2-]1w
The days of cleartext passwords are numbered, basic auth just doesn't cut it.
The plumbing is there for m5d digest authentication, why not just use it.

The patch above supports RFC 2069 and much of RFC 2617's additions.
Really, only two things are missing: support for "auth-int" quality of
protection extension, and client-side nonce counting. Additionally, the
client nonce generation could be better. These are all optional extensions
onto the digest auth mechanism, though, and are very unlikely to be widely
used. Basically, this provides digest auth for the vast majority of current 
server implementations.
The digest patch doesn't include nsNetwerkMD5.[cpp|h], therefore mozilla
doesn't compile after applying the patch.
I accidentally posted the wrong file originally (just the patch).
The new attachment is a tar.gz with the new files and the patch.
Keywords: patch
shaver: can you review this? I'd like an extra pair of eyes to go over this. 
thx.
Target Milestone: M17 → M18
The patch currently posted is getting a bit old, so I'll post a new patch to 
resolve any merge problems with the current CVS code. Also, the code changes in 
nsHTTPChannel::Authenticate() will change slightly if my patch for bug 44041 is 
approved, which is, arguably, more important than this one (since this is an 
enhancement and 44041 is an actual bug).
Target Milestone: M18 → Future
The tests available through http://digest-test.agranat.com/ test both basic and
digest authentication, both for single realms and multiple realms.  M18 fails 3
out of 4 tests, since it doesn't have digest authentication support yet nor does
it support multiple realms, a feature that would probably be part of the
Password Manager.

The site was apparently set up by a coauthor of RFC 2617.

M$ IE 5.5 can pass all the tests available through http://digest-
test.agranat.com/, but M18 can't.
->neeti
Assignee: gagan → neeti
Status: ASSIGNED → NEW
We've had a patch here since June, could we please get it reviewed, super-
reviewed, and examined by the module owner? It would be a shame to have Justin's
work go unloved... 

Justin: Thanks for the patch!
Keywords: correctness, testcase
Blocks: 61681
*** Bug 58418 has been marked as a duplicate of this bug. ***
Depends on: 65092
No longer depends on: 65092
Blocks: 65092
Just for the record: Apache supports disgest, squid (a popular http cache) will 
support digest in squid 2.5 (the code is available via CVS). In squid the cache 
supports the client side nonce, and process's  auth-info headers correctly. It 
does this in acceleration and proxy modes. There is a good chance that it will 
support message integrity as well by the time 2.5 is released. 

I expect that the exposure and demand for rfc 2617 compliance will increase 
with both these popular products supporting it. I tested mozilla 0.7 (sorry not 
the snapshot - no time) with squid sending _only_ a digest authentication 
challenge and it failed.
Open Networking bugs, qa=tever -> qa to me.
QA Contact: tever → benc
Can we get reviews, please, so this can get into 0.9.2?
Component: Networking → Networking: HTTP
Keywords: nsbeta2 → mozilla0.9.2, review
the code will have to be brought up to date since much of the http core arch
has changed.  who owns this patch?
The above patch (for 0.9.2 release) provides the HTTP Digest Authentication
(partially).  It is based on Justin's patch from June 2000.
The following are my testing results.
 
What works:
Digest authentication with Apache 1.3.20/mod_auth_digest, which uses
"MD5" and "auth" only (no MD5-sess and auth-int)
 
What doesn't work:
1. The test site in the previous attachment, which uses MD5-sess.
I guess the problem is with MD5-sess, but since Apache doesn't
support MD5-sess yet, I can't really test it.
2. Justin's patch doesn't support auth-int.
3. I did not test Proxy-Authentication/-Authorization at all, so it might
not work.

Here are some implementation issues:
1. The NS_HTTPDIGESTAUTH_CID defined in build/nsNetCID.h is copied from
the NS_DIGESTAUTH_CID from Justin's patch, though I have no idea how the
CID is generated.
 
2. Digest authentication uses a new authorization request header for every
request, since A2 includes the request-uri.  The original
nsHttpChannel::AddAuthorizationHeaders() uses the credential in the
authCache.  This works for Basic authentication, but not Digest.
It should be possible to check whether it is Digest, and if it is,
generate a new credential in AddAuthorizationHeaders().
However, since I wanted to reuse existing code as much as possible,
I changed AddAuthorizationHeaders() so that it doesn't add Authorization
if the credential is for Digest.  This will result in a 401 response,
which will be handled by ProcessAuthorization().
 
3. GetCredentials() needs to get the username/password from the authCache
so that we don't prompt the user for every request, but we can't get the
username/password from the cached credential.  I modified the nsHttpAuthCache
to store the A1 part for Digest credentials.  A new function
generateCredentialsFromA1() is added to nsIHttpAuthenticator.idl to handle
this case.

If you look at my patch, you'll probably notice that I'm not an experienced
C++ programmer and I've never worked on Mozilla until a few days ago when
I tried to set up Apache with digest authentication.
So I'm sure much of this can be written in a much better way.
However, since this "bug" has been around for a long time and Justin's
patch has been there for more than a year, I just hope this can at least
be a starting point of a good implementation.
I forgot to mention that the attachment is tar.gz, and the tests were done on
a Linux box (RH7.1).
current patch is tar.gz in case anyone is wondering.

This is the second (third?) implementation of MD5 i've seen in a week.

First my traditional licensing gripe: If you aren't a netscape employee you 
have no business using NPL, please use MPL instead, the change is very simple, 
please see http://www.mozilla.org/MPL/boilerplate-1.1/
as an additional comment, please use the Contributors: section unless you have 
some specific need (the only two instances I know of are IBM and another thing 
which I hope to squish), an entry in contributors can have a parenthetical 
explaining what someone added.

Personally I'm opposed to Contributors because I'd prefer to rely on 
CVSBlame/CVSLog + Bugzilla, but if you need to clutter the code w/ the 
reference the contributors section is appropriate.

As to MD5 itself... I think NSS has an implementation (lord/wtc?) and think you 
should probably use it.  Otherwise you run the risk of mucking w/ our security 
encapsulation which allows people to build mozilla in places that forbid 
encryption (i'm not sure MD5 checksums count, if they don't then please cite 
something -- i'm tired).

mozilla code generally groups all includes together.

please split all if (...) return ...; statements across two lines to aid 
debugging. (you do it right in nearly all instances...)

don't use tabs. [follow modelines if the exist]

is there a reason to use this logic:

+    if (!entry) {
+        if (creds) {
+            entry = new nsEntry(path, realm, creds, a1);
+            if (!entry)
+                return NS_ERROR_OUT_OF_MEMORY;
+            mList.AppendElement(entry);
+        }
+        // else, nothing to do
+    }
+    else if (!creds) {
+        mList.RemoveElementAt(i);
+        delete entry;

w/o reading the code carefully,
if (creds) {
  if (entry) {
    ...
  } else {
    ...
  }
} else {
  ...
}
seems like a better arrangement

!PL_strncasecmp(creds.get(), "digest ", 7);
is there a reason not to use scc's string compare functions?

+	nsCString A1;
+	A1.AssignWithConversion(user.get());
+	A1 += ":";
+	A1 += realm.get();
+	A1 += ":";
+	A1.AppendWithConversion(pass.get());
I think you should probably rewrite this (scc, advice?)

+	// now need to store A1, too
+    //return authCache->SetCredentials(host, port, path, realm.get(), 
creds.get());
+    return authCache->SetCredentials(host, port, path, realm.get(), 
+		                                 creds.get(), A1.get());
changing api signatures is always a concern, but better sooner than post 1.0 i 
suppose. 

Foo::Bar::Baz is taboo, we don't use namespaces. 
http://www.mozilla.org/hacking/portable-cpp.html#dont_use_namespace

your endian handling worries me, comments from nspr people (wtc)?

statics usually make me worry about threadsafeness

		authString += "\", algorithm=";
		if (algorithm & ALGO_MD5_SESS)
			authString += "MD5-sess";
		else
			authString += "MD5";
why not
		authString += "\", algorithm=MD5";
		if (algorithm & ALGO_MD5_SESS)
			authString += "-sess";
the pattern a+="cstring";a+=nsCString(something);... bothers me, i'm sure scc 
will have advice.  I think 
a+=NS_LITERAL_CSTRING("cstring")+something+NS_LITERAL_CSTRING("anotherCstring")
+somethingElse/*+...*/; is probably better.

Instead of long // blocks of code, please use #if 0 or #ifdef 
FIXME_HTTP_DIGESTAUTH so that lines which aren't changed when it's fixed don't 
mess up cvs blame.
for long comment blocks /* */ would be nice...

nsHttpDigestAuth::ExpandToHex looks like a function which should already exist 
in our string routines...

Thanks for working on this.

cid's can be generated by connecting to irc.mozilla.org and /msg mozbot uuid.
Whiteboard: [nsbeta2-]1w
The above patch (tar.gz file) includes fixes for a few problems mentioned
in the previous comment:
1. tabs are eliminated.
2. long // blocks of code from Justin's patch are changed to "#if 0"
3. long // coment blocks from Justin's patch are changed to /* */
 
Sorry about the licensing issue.  I created nsHttpDigestAuth.h|cpp by copying
nsHttpBasicAuth.h|cpp, and that's why the NPL is there.  I didn't even notice
that it is NPL until you mentioned it.  However, since much of the code came
from Justin's patch (nsDigestAuth.h|cpp), which also used the NPL, I don't
think I have the right to change it (?).
 
As for the MD5 implementation, the nsNetwerkMD5.h|cpp in my patch were copied
from Justin's patch without any changes (I guess this is why you mentioned
the use of the "Contributors", since these two files don't use it).
I didn't look at how it works or its legal implications.  If there is an
"official" implementation somewhere in the source tree, I can certainly use
it.  But my goal was to make Justin's patch work under the new netwerk code,
and I wanted to re-use as much existing code as possible and make as few
modifications as possible.  Many of the things you mentioned are a result of
this, so I probably shouldn't change them.  I list these things below:
 
1. scattered includes in nsNetModule.cpp: I was just following how
nsHttpBasicAuth was included there

2. "if (...) return...": I don't usually use this.  The ones you see are
a result of copy-and-paste.  I fixed those in nsHttpDigestAuth.cpp, but
there are a lot of them in the original nsHttpChannel.cpp, and I don't think
I should change those.
 
3. the logic you quoted from nsHttpAuthCache.cpp: it's an exact copy of
the original function, so I don't know why it's written that way.
 
4. the PL_strncasecmp: I copied it from Justin's code.  I don't know
what's the "right" way to do it.  (I don't even know who scc is...)
 
5. the nsCString concat thing: I know it's really ugly.  I copied it
from somewhere (I forgot), and I don't know what's the "right" way to
do it.  Please advise.
 
6. "Foo::Bar::Baz" in nsHttpAuthCache.cpp: I didn't change the definition.
I only added more functions to those classes.
In addition to these issues, my greater concern is that my approach to
extend the AuthCache to support digest authentication was simply a hack.
I'm sure there is a better way to extend the AuthCache interface.
The same problem applies to the nsIHttpAuthenticator interface.
(For example, the nsHttpChannel probably shouldn't have to look at the
challenge/credential and take different actions according to the
authentication type.)
Tried the latest patch on the trunk on linux 2.4/SuSE. Had to tweak it a bit to
get it compiling (add a mising method to nsHttpDigestAuth.h).

On the testsite http://digest-test.agranat.com/ the digest tests still fail. The
code seems to not call the prompt box for user/password.


Sorry I forgot to mention that the new patch is still for 0.9.2 release.
In my own testing, it works with Apache/mod_auth_digest, but the
digest test on "http://digest-test.agranat.com" failed.  However,
it did prompt me for username/password, and an authorization header was
generated.  According to the test trace, the server says "authorization
forgery", which probably means the calculation of the "response" field
is wrong (since the other fields are correct).  As I mentioned previously,
my guess is that there is a bug in the MD5-sess part of the patch, which
I haven't looked into.  (Apache/mod_auth_digest only supports MD5 currently,
and I actually fixed a few bugs in th MD5 part by debugging from both sides.)
 
So the first problem is that something was changed after 0.9.2 so the patch
doesn't prompt the user any more.  The second problem is the MD5-sess.
My code can be changed to MPL.

I copied the MD5 code from the mailnews module long ago. I could not figure
out whether MD5 was provided in NSS, nor how to use NSS, so I just followed 
mailnews' lead. The are other authentication methods which need various 
encryption and hashing algorithms (SHA, DES, etc). So knowing how to access 
those implementations in NSS would be good to know? Any places in particular to 
look for documentation/examples?



+relnote - we shoudl release note that this didn't make it into current versions
yet.
Keywords: relnote
After staring at the RFC for a while, I found that H() denotes the hex string
of the MD5 hash, not the binary data.  This caused the problem in the MD5-sess
part of the patch, which uses H().  The above patch fixes this problem, and my
testing shows that it passes the first two digest tests on the test site
"http://digest-test.agranat.com".  (The third test uses the authentication-info
header...)
I applied the new patch on the trunk and this time I applied the *whole* patch.
Now I'm getting the user/password box, but I'm not getting past that, it returns
every time unless I cancel. I will check if patch has done the right thing ...

I hate patch ... okay the code also works on the trunk as it does on the 0.9.2
branch described by pach.
timeless@mac.com wrote:
> As to MD5 itself... I think NSS has an implementation
> (lord/wtc?) and think you should probably use it.

You should not call the MD5 code in NSS directly.
You should call PSM's MD5 interface, which is declared
in mozilla/caps/idl/nsISignatureVerifier.idl.
(http://lxr.mozilla.org/seamonkey/source/caps/idl/nsISignatureVerifier.idl#54)

> your endian handling worries me, comments from nspr
> people (wtc)?

I am sorry that I can't review the patch so I have no
comments to add regarding the endian handling.
i don't see how we could use that interface, mstoltz?
Depends on: 92054
Is this how to use the interface?

  nsresult rv;
  nsCOMPtr<nsISignatureVerifier> verifier =
           do_GetService(SIGNATURE_VERIFIER_CONTRACTID, &rv);

It seems to work (limited testing with the Hash* functions), but
is this the "right" way to do it?

Regarding the endian handling: the only piece of code that involves
endian handling is the MD5 implementation (as far as I can see).
So if the official implementation is used, there shouldn't be any
problems.
RELNOTE for NS 6.1:
Digest authentication is not supported in this release.
The above new patch (tar.gz) is for the trunk (2001-08-21).  According
to my own testing, it passed all four tests on the test site
"digest-test.agranat.com" (though I'm not sure why it actually passed
the fourth one).  Also:
1. It now uses the "official" MD5 implementation (using the method I
described in my previous comment).
2. The authentication scheme is now transparent to the nsHttpChannel
(i.e. it doesn't have to look at "basic"/"digest" any more).
3. There is no longer an extra round-trip overhead for subsequent
requests.
4. The license of the two new files is changed to MPL.  (However, I
don't know how to fill in the "fields", so I left that paragraph
untouched.)
 
Some old problems probably still apply, though.
(E.g. string handling.)
OK, what's it going to take to get this in?  Can someone summarize in small
words what the status is on this patch?  Is it "done" and ready for review?
Status of the patch:
On my machine, it passes all tests on the digest auth test site, and 
it also works with apache/mod_auth_digest.  That's pretty much all 
I can test by myself.  I don't know if anyone else has had time to 
look at it.
I did some tests on an older version of the patch on linux and it passed all
tests on the given testside it was supposed to pass.
Have you tested digest proxy-authentication?
And also tested that when presented with multiple challenges - say basic, digest
in that order, that digest is used?
No, I haven't tested anything other than what I mentioned above.
 
For proxy-auth: it would be great if someone who has a proxy can test this.
 
For multi-challenge: it would be great if someone who has a server that does
multi-challenge can test this.  (However, this is handled by the original
code, i.e. not part of the patch.)
> For proxy-auth: it would be great if someone who has a proxy can test this.

Squid version 2.5 has primitive digest support. (It doesn't support trailers,
and thus cannot do message body authentication. It also doesn't support two way
authentication - no time to do that yet). I don't have a build environment for
mozilla, and squids build environment is _much_ smaller than mozilla's, so I'll
pass the task to someone else. Unless someone wants to provide me with a mozilla
binary with this patch in it.

> For multi-challenge: it would be great if someone who has a server that does
> multi-challenge can test this.  (However, this is handled by the original
> code, i.e. not part of the patch.)


Oh, ok. Well I think apache does multi challenge, just pick and choose the
order. If it doesn't then squid definately doesn, just choose the order the
auth_param lines occur in.
The issue of presenting multiple authentication methods to the client, and 
choosing an appropriate one is covered by bug 44041. It should work with proxy 
authentication, too, but I never tested that case. I don't know if anyone else 
has either.

the changes to the existing code seem large.

an-cheng: can you summarize the changes made to the existing code and explain
why they are necessary?  ie. what was not provided by the nsIHttpAuthenticator
interface as well as the code in nsHttpAuthCache and nsHttpChannel?
The main thing is that digest auth needs to generate a new credential for
every request (with a different URI), since the request URI is used in
the calculation.  Therefore, storing the credential in the AuthCache is
not enough; username and password (or the hash of them) also need to be
cached.  As a result, for each subsequent request, basic auth will simply
use the cached credential, while digest auth uses the cached user & pass
to generate a new credential.

To make this transparent to the nsHttpChannel, a function
NewCredentialRequired is added to the nsIHttpAuthenticator interface so 
that HttpChannel can query the interface to see whether to use the cached
credential or to generate a new credential, instead of looking at the
challenge string to determine the auth scheme.  This is the largest
change in nsHttpChannel.

nsHttpAuthCache is changed to also store the username, password, and
challenge.  The majority of the changes (in terms of line count) in this 
patch is here.  The reason for the high line count is that I simply 
overloaded all relevant functions, so there are essentially two copies of 
each function.  In fact, if the old AuthCache interface is not used
anywhere else, it can be removed.
nsHttpChannel is the only consumer of nsHttpAuthCache, so please cleanup the
unused functions (BTW: you can use lxr.mozilla.org to verify dependencies).

also, i would prefer NewCredentialsRequired (instead of NewCredentialRequired),
since this would be more consistent with the existing "grammar."
The above patch cleans up nsHttpAuthCache (now it has the new interface).
Also, the function name is changed to NewCredentialsRequired.
*** Bug 98497 has been marked as a duplicate of this bug. ***
the above patch contains some minor changes to make it work with the current
trunk.

by the way, it seems that the test site "digest-test.agranat.com" is no longer
accessible, so I could only test the patch with apache/mod_auth_digest
(1.3.20).
Keywords: mozilla1.0
This patch has been languishing for too long. It should be checked in or
rejected. Whose door needs to be banged on?
Darin, you're "it" -- feel free to reassign to a delegate who will not let this
patch rot.  If it should wait till after 1.0, make that case.  Given how long it
has waited, and the standards compliance angle, if we can take the patch without
spending lots of followup or support effort, I'm in favor of it.

Thanks,

/be
Assignee: neeti → darin
This is an internet standard, and I'd be interested in hearing reason why it
should be deferred out of 1.0.

My case for inclusion 1.0 or earlier is made here:
http://www.packetgram.com/pktg/docs/edit/july.html
ok, i've skimmed the patch. it looks good for the most part... there is some
cleanup that needs to be done.  i'll have some review comments once i get a
chance to do a more thorough reading of the patch.

-> 0.9.7 (hopefully)
Target Milestone: Future → mozilla0.9.7
Summary: Digest authentication? → [RFE] Digest authentication?
this latest patch cleans up a lot of the auth cache stuff.  it doesn't touch
nsHttpDigestAuth.{h,cpp} other than to replace a few prints with LOG
statements.

i still need to run this patch through some tests.
Attachment #10103 - Attachment is obsolete: true
Attachment #10580 - Attachment is obsolete: true
Attachment #43081 - Attachment is obsolete: true
Attachment #43173 - Attachment is obsolete: true
Attachment #43329 - Attachment is obsolete: true
Attachment #46731 - Attachment is obsolete: true
Attachment #48257 - Attachment is obsolete: true
Attachment #53418 - Attachment is obsolete: true
Attachment #55673 - Attachment is obsolete: true
this latest patch is ready to be reviewed.
Status: NEW → ASSIGNED
+    if (!node) {
+        // only create a new node if we have a real entry
+        if (!creds && !user && !pass && !challenge)
             return NS_OK;

Do basic auth requests have a challenge?

+  // this is random, and should be changed
+  nsCAutoString cnonce("0123456789abcdef"); 

OK, should it be changed?


Ideally, "cnonce" should be a random string.  The current approach works
(i.e. it does not violate the RFC), but by having the same cnonce for
all requests it defeats the purpose of using the cnonce.

In addition, "nonce_count" is currently always "00000001".  This will fail
if the server checks this field.  The reason that it works now is that,
in apache 1.3.20, mod_auth_digest only checks nonce_count if:
(AuthDigestNcCheck is On (default Off)) &&
(the SHMEM_MM feature is On (default Off)).  To implement this correctly,
nonce_count needs to be a per-nonce counter (i.e. incremented for each
request that uses a particular nonce).

a basic auth challenge looks like: basic realm="foo"

in other words, the challenge the value of the WWW/Proxy-Authenticate header.
An-Cheng: do you think its important that we get the cnonce and nonce_count
right before landing this patch?
I don't really know the criteria for landing a patch...

In terms of "working code", the patch will work for most people that use
apache.  In fact, the AuthDigestNcCheck directive is marked as "not
implemented yet" in apache documentations, so it's not supposed to be
turned On.  I haven't had a chance to test the patch with squid, IIS,
or any other servers, so I don't know what their defaults are.

In terms of correctness, the cnonce is not "wrong" (since it is supposed
to be anything chosen by the client).  The nonce_count probably violates
the RFC, but since its purpose is "to allow the server to detect request 
replays", this may not be important if the servers don't check nonce_count.

Additionally, there are a few unimplemented features (auth-int QoP is the 
most obvious one, but mod_auth_digest hasn't implemented it, either), so 
this is going to be a partial implementation of the RFC anyway...
With a fixed nonce-count, Mozilla will fail to interoperate with servers which
correctly implement the protocol specification.  Thus we should not land the
patch until nonce-count is correctly implemented.

yeah, i agree with that.  we should get nonce_count correct before landing this
patch.

An-Cheng: are you going to implement nonce_count, or should i take a stab at it?
perhaps one way to support nonce-count would be to have an extra nsISupports
opaque parameter stored in the auth cache with each entry.  nsIHttpAuthenticator::
GenerateCredentials could take a corresponding inout nsISupports parameter.

digest auth could use a nsISupportsPRUint32 for this and simply increment the
stored value (the nonce-count) each time GenerateCredentials is called.  sound good?
Darin: that's also my first thought.  At the moment I am not able to 
implement it, so it would be great if you can do it.  Also, the
"XXX bug"'s in nsHttpDigestAuth.cpp can be removed now (I put them there
to remind myself where I modified the original code).  Thanks.
By the way, the cnonce should be much easier to implement, probably even
2 or 3 lines of code, e.g. get a random number (there must be an RNG
somewhere in the source tree...), convert it to a hex string, and use it
as the cnonce.  I think this is sufficient (but I am not a security
expert...).
An-Cheng: thanks for the info... i'll work on a patch.
1- storing an extra nsISupports parameter in the auth cache.  digest auth uses
this to store the nonce-count.	it's incremented each time GenerateCredentials
is called.

2- cnonce is calculated using the rand() system call... is this ok/sufficient?
from what i could tell, rfc 2617 isn't very specific about how to calculate a
cnonce.
Attachment #57739 - Attachment is obsolete: true

    
    

    
  


  
minor cleanup

        Attachment #57901 -
        Attachment is obsolete: true

    
    

    
  
ok, the latest patch is ready to be reviewed.

    
    

    
  
Comment on attachment 57976 [details] [diff] [review]
v1.4 removed old XXX bug? comments

trigger happy or something :/

        Attachment #57976 -
        Attachment is obsolete: true

    
    

    
  
+                                 nsXPIDLString &user,
+                                 nsXPIDLString &pass)

is it a good idea to be passing XPIDLStrings as function parameters?           
                    



    
    

    
  
blizzard's right -- nsXPIDLString and nsXPIDLCString should be used only as
types of local variables, for automatic management of PRUnichar** and char** out
param result storage when calling XPCOM methods.

/be

    
    

    
  
I tested the v1.4 patch with squid-head-200111140000, and it didn't work.
After looking into it, I found two places in squid's implementation that
differ from my intepretation of RFC 2617:

1. squid assumes the "qop-value" in Authorization Request Header is quoted,
but that seems to be wrong (see sections 3.2.2 and 3.5).

2. squid assumes the "algorithm" in Authorization Request Header is
present, but if we assume the definition of this field is the same as
that in section 3.2.1, it is optional and defaults to "MD5" (see also
section 3.5).  (Again, squid assumes the algorithm value is quoted,
which seems to be wrong.)

After slightly modifying the squid implementation
(authenticateDigestDecodeAuth in auth_digest.c), the patch seems to
successfully authenticates with squid.  (Note that I also tried IE 5.5,
and it works fine without the modifications...)

However, there is another problem:
after the authentication, mozilla segfaults!  The reason that I think the
authentication is successful is that I can see the TCP_MISS/200 following
the TCP_DENIED/407 in squid's access.log, and I also see the actual 200
response in tcpdump.  I tried a few different sites, and the results are
the same (segfault).  Of course, mozilla works fine if no authentication
is required...

    
    

    
  
> 1. squid assumes the "qop-value" in Authorization Request Header is quoted,
> but that seems to be wrong (see sections 3.2.2 and 3.5).

3.5 shows qop-value as being quoted and not quoted. Hmm that is unclear isn't 
it. However section 3.2.1 shows quoted values for this, and 3.2.2.1 explicitly 
unquotes the value before use. Perhaps we should seek clarification from the 
WWW?

> 2. squid assumes the "algorithm" in Authorization Request Header is
> present, but if we assume the definition of this field is the same as
> that in section 3.2.1, it is optional and defaults to "MD5" (see also
> section 3.5).  (Again, squid assumes the algorithm value is quoted,
> which seems to be wrong.)

As for this being present, thats a bug, and I will rectify in squid pre 2.5 
being release. However, for quotation the same applies as for the qop 
quotation. 

> After slightly modifying the squid implementation
> (authenticateDigestDecodeAuth in auth_digest.c), the patch seems to
> successfully authenticates with squid.  (Note that I also tried IE 5.5,
> and it works fine without the modifications...)

Because, in this case, IE is closer to the standard.

I can't comment on the mozilla segfault though. 

    
    

    
  
> > 1. squid assumes the "qop-value" in Authorization Request Header is quoted,
> > but that seems to be wrong (see sections 3.2.2 and 3.5).
>
> 3.5 shows qop-value as being quoted and not quoted. Hmm that is unclear isn't
> it. However section 3.2.1 shows quoted values for this, and 3.2.2.1
> explicitly
> unquotes the value before use. Perhaps we should seek clarification from the
> WWW?

Actually, I think the definitions are quite clear...

From 3.2.1: (Authenticate Response Header)
    qop-options       = "qop" "=" <"> 1#qop-value <">
    qop-value         = "auth" | "auth-int" | token

From 3.2.2: (Authorization Request Header)
    message-qop      = "qop" "=" qop-value

So from these definitions, the qop-options in Authenticate should be quoted
(note the explicit <"> that represents an actual quote), since there may
be more than one qop-value.  On the other hand, the message-qop in
Authorization should not be quoted, since qop-value is unquoted.
Also, from the description of qop in section 3.2.2 (page 11,
second paragraph, 4th line): "Note that this is a single token, not a
quoted list of alternatives as in WWW-Authenticate."

This is also confirmed by 3.5 (examples), where the qop in Authenticate
is a quoted list, and the qop in Authorization is an unquoted token.

> > 2. squid assumes the "algorithm" in Authorization Request Header is
> > present, but if we assume the definition of this field is the same as
> > that in section 3.2.1, it is optional and defaults to "MD5" (see also
> > section 3.5).  (Again, squid assumes the algorithm value is quoted,
> > which seems to be wrong.)
>
> As for this being present, thats a bug, and I will rectify in squid pre 2.5
> being release. However, for quotation the same applies as for the qop
> quotation.

Again, from 3.2.1: (Authenticate Response Header)
    algorithm         = "algorithm" "=" ( "MD5" | "MD5-sess" |
                         token )

Since there are no explicit quotes (i.e. <">), the value should be an
unquoted token.  (If in doubt, see the ABNF definition in RFC2616 section 2.)

> > After slightly modifying the squid implementation
> > (authenticateDigestDecodeAuth in auth_digest.c), the patch seems to
> > successfully authenticates with squid.  (Note that I also tried IE 5.5,
> > and it works fine without the modifications...)
>
> Because, in this case, IE is closer to the standard.

Which standard?

    
    

    
  
blizzard, brendan: yeah, there's no reason for me to be passing around
nsXPIDLString's... will make the change.

    
    

    
  
> Actually, I think the definitions are quite clear...

...
>Also, from the description of qop in section 3.2.2 (page 11,
>second paragraph, 4th line): "Note that this is a single token, not a
>quoted list of alternatives as in WWW-Authenticate."

Granted. Thanks for the pointers. (It's been some time since I reviewed this in 
any detail.)

...

>> Because, in this case, IE is closer to the standard.
>Which standard?

RFC 2617. Can I get your patch for squid if you are willing to contribute it, 
as it will save some time.


    
    

    
  
rbtcollins: I don't really have a patch, as I only hacked it for my
purpose (testing this mozilla patch).  What I did was: use the 4 chars
after "qop=" for qop-value, and default to "MD5" if algorithm is NULL.
For a real patch for squid, you probably need more than this.

Also, could you let us know when you make the changes in squid?
(So that it will be easier to test this patch and figure out what's
causing the segfault.)

    
    

    
  
Comment on attachment 57977 [details] [diff] [review]
v1.4 removed old "XXX bug?" comments

Not sure I understand why we need allocateExtra on the http authenticator
interface.  

Is nsCString to expensive to use for all of your string allocation (mPath,
mRealm, ect.)



Fix this:
+#include "caps/nsISignatureVerifier.h"

There is alot of parsing in this code.	We should really run Purify over this
and bang on it.

In the GenerateCredentials function, there are alot of TODO statements.  Should
these be addressed before you land this?

    
    

    
  


  
This patch fixes the "caps/nsISignatureVerifier.h" issue.

The TODO's are for the not-yet-implemented "auth-int" QoP option,
which provides message integrity in addition to authentication.
As far as I know, apache and squid haven't implemented it, but
I don't know about other browsers/servers...

    
    

    
  
FYI, for interop with IIS, you should accept *all* Digest challenge parameters
quoted and unquoted: IIS quotes some which shouldn't be quoted (or possibly the
other way round, can't remember). (that is sensible "be lenient in what you
accept behaviour anyway)

    
    

    
  
Yes, this patch does accept all parameters quoted/unquoted (this is from
Justin's original patch).

    
    

    
  


  
this patch is ready to be reviewed.

        Attachment #57977 -
        Attachment is obsolete: true

        Attachment #58578 -
        Attachment is obsolete: true

    
    

    
  
Sorry; I checked through my archives: the thing IIS5 requires is that you *send*
the qop= and algorithm= values quoted in the request header; otherwise it gives
a 500 response. So in effect every parameter is quoted except nc; the v1.6 patch
doesn't seem to do this.

Despite the fact that this isn't what 2617 says, it seems to work interoperably
(having tested against Apache/mod_auth_digest, and several of the commercial
WebDAV servers which support digest).



    
    

    
  
what a lovely dilema... if we implement RFC2617 then we most likely won't work
with IIS (and possibly other servers), whereas if we go against RFC2617, then we
most likely will cause problems for yet another set of servers and/or yet to be
written servers.  my inclination therefore is to implement RFC2617, and then
file a bug to support IIS.

to support IIS we would first need a testcase.  we could add code to
nsHttpDigestAuth that sniffs the Server response header.  if it finds a broken
web server such as IIS, it could then send out a modified set of credentials.

so, i think for the first revision of digest auth support in mozilla, we should
simply land our implementation of RFC2617.  later on we can add workarounds as
need be on a per server basis.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v1.7
        — 
          — Splinter Review
      

    


  
cleanup patch per comments from dougt:

1- canReuseCredentials -> areCredentialsReusable
2- extra -> meta data
3- remove printfs
4- do this the other way around, so that you only call Length once:

 +  nsCRT::memcpy(contents, username.get(), username.Length());
 +  len = username.Length();

        Attachment #59269 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 59959 [details] [diff] [review]
v1.7

got an r=dougt on this.

        Attachment #59959 -
        Flags: review+

    
    

    
  
This patch looks good...

i did notice that in nsHTTPDigestAuth::CalculateHA1(...) 'contents' can leak if 
MD5Hash(...) fails...

-- rick


    
    

    
  
rick, thanks for the review... i'll fix that leak before checking in.

    
    

    
  
landed v1.7 patch on the trunk!

thanks to everyone who made this possible :)
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED

    
    

    
  


  
the problem was that for a proxy the "path" in nsHttpAuthEntry is null,
so => SEGV on strlen() in nsHttpAuthNode::GetAuthEntryForPath().

Note that from what I could tell, as of 200112090001 the quote handling
has not changed in squid, so I still used the hack I mentioned above.
I've also found another issue in squid's digest implementation: it uses
atoi() on the nonce count (see authDigestNonceIsValid()), which is actually
a hex string as specified in RFC 2617.	So the result was that the first
10 requests will work fine, but the 11th will be rejected because of atoi().
Changing it to strtol(,,16) resolves the issue (I think).

However, while testing with squid, I found a problem regarding how the nonce
count is incremented in mozilla.  I'll describe it in the next comment.

    
    

    
  
When using a proxy (squid in my tests), sometimes the nonce count values
in mozilla's requests are not sequential.  Here is what I saw using
squid with digest auth (with the hacks mentioned above):

1. clear disk/mem cache, restart mozilla
2. go to kernel.org
   => 7 requests generated (user/password asked on the first one)
   => works fine, i.e. nc values sequential (e.g. 1 to 7)

3. restart mozilla
4. go to kernel.org
   => 2 requests ("/" and "/ubar/ubar.png")
   => nc values not sequential.  if "/" has nc value X, then
      "/ubar/ubar.png" always has nc (X + 2)
5. reload
   => 7 requests with sequential nc values, but the first one is
      like (X + 7)

So it seems that GenerateCredentials is always invoked even if an object
is cached and no request is sent...
Note that the above is with the cache pref set to "Automatically".
If I set it to "Every time...", there are always 7 requests, so this is
not an issue (I guess).

Also note that if I go to cnn.com, the requests (~100) will have
non-sequential nc values, even if I clear disk/mem cache or set cache
pref to "Every time...".  So there may be other additional/deeper issues...

Maybe this should be a new bug?

    
    

    
  
By the way, since the purpose of the nonce count is to prevent replay,
the current implementation is sufficient (if the problem I described above
is what I guess it is), as long as the server/proxy enforces a
"strictly-increasing" policy, instead of an "increment-by-one" policy.
Of course, we might be asking for too much...

    
    

    
  
An-Cheng: the crash was fixed the day after this patch landed.  for the other
problem, how about filing a new bug?  this bug is long enough ;-)

    
    

    
  
oops, haven't updated my tree for some time...
filed bug 114451 for the nonce count issue

    
  
QA Contact: benc → tever

    
  
QA Contact: tever → httpqa

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: