114451 - non-sequential "nonce count" values (digest authentication) in consecutive requests

Status: RESOLVED INCOMPLETE

(Core :: Networking, defect, P5)

Description

[An-Cheng Huang]

When using a proxy (squid in my tests) with digest auth, in some cases 
the nonce count values in mozilla's requests are not sequential.  Here
is what I saw using squid with digest auth (note that workarounds are
needed to use squid's digest implementation, see comments of bug 15860
for details):

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 (~100) requests 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...

Comment 1

[neeti]

http

Comment 2

[Darin Fisher]

fixing this may be non-trivial.  it would mean delaying the addition of
authorization request headers, which is currently done when a channel is created
in order to allow the creater of the channel complete control over the outgoing
headers.

An-Cheng: how critical do you think this problem is?  what does IE do?

Comment 3

[An-Cheng Huang]

And I don't know whether this problem also occurs when accessing a server
(instead of a proxy).  I'll do some testing with apache.

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...

Since squid implements the increment-by-one policy, non-sequential nonce count
values will cause requests to be rejected.  apache has not implemented
nonce count checking (according to its docs), so this is not a problem (yet)

Comment 4

[An-Cheng Huang]

darin: I personally don't think this is too critical.  As mentioned, apache
is not checking the nonce count yet, and it seems that no one has noticed the
atoi() problem in squid's nc checking for quite some time, so probably not
that many people are using such features anyway.  I'll try to do some testing
with apache and IE...

Comment 5

[Darin Fisher]

there could actually be bigger problems... i don't think we currently guarantee
any order for outgoing HTTP requests.  this means that a server might see a
nonce count that is not even strictly increasing.  and, fixing this is probably
non-trivial :-(

Comment 6

[Darin Fisher]

-> 0.9.9

Comment 7

[Darin Fisher]

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

Comment 8

[Jeremy M. Dolan]

I filed a duplicate, and I do not have a proxy.

Comment 9

[Darin Fisher]

good point... this bug doesn't require a proxy server.  it's just that An-Cheng
noticed the problem while using a proxy server (squid).

Comment 10

[rbtcollins]

I've generated a patch for squid to handle the nonce count correctly. I haven't 
tested it (short on time) but it builds corectly.

An-Cheng, can you confirm that this fixes the access denied behaviour from 
squid when Mozilla does it's non monotonically incrementing behaviour (you'll 
need to use 
nonce_strictness off
to test that.

I still haven't address the other squid bug that's been reported - the quoting 
issue - but I intend to soon.

Comment 11

[rbtcollins]

Attachment: Patch for squid to allow interoperation with Mozilla's quierky nonce count creation

*cough* - forgot to attach to my last comment.

Comment 12

[rbtcollins]

Sorry folk, the previous patch is badly broken. I'm about to post a corrected 
one that also address's the quoting/non quoting of qop and algorithm. Can 
someone with edit attachment rights delete/obsolete the old patch

Comment 13

[rbtcollins]

Attachment: Patch for Squid 2.5 PRE /HEAD@24th December to interoperate with mozilla's nonce count handling

As promised, one patch. This has been tested with IE 6, and includes the quoted
parameter fix. YMMV. Feedback appreciated - I'll hold off putting this into the
production CVS until I get at least one confirmation that this works for
mozilla too.

Cheers,
Rob

Comment 14

[An-Cheng Huang]

darin: it seems that bug 116177 is related to the "nextnonce" field of the
AuthenticationInfo header, not the nonce count issue in this bug, so maybe
that bug should be reopened?

Jeremy: are you referring to the "Next nonce" test at the site
"http://digest-test.agranat.com/"?  It had been inaccessible for some time
when the digest auth patch went into mozilla, but it now appears to be up
and running again.  I don't think mozilla currently handles the
AuthenticationInfo header, so the nextnonce test is not supposed to work...

by the way, in the "Stale nonce" test at that site, mozilla prompts for
user/password twice.  Although it still passes the test, there should
probably be only one prompt (IE 5.5 only prompts once).  This is probably
due to the fact that mozilla currently does not look at the "stale" field
in WWW-Authenticate.  Maybe this should be a new bug?

Comment 15

[An-Cheng Huang]

I tested IE 5.5 with squid and looked at how its nonce counts look like.
It turns out that, for every request, IE starts with one that does not
include the Proxy-Authorization request header (according to tcpdump),
i.e.:

    IE               Squid
            GET
      -------------->
            407
      <--------------

        GET w/ Auth
      -------------->
            200
      <--------------

Note that this is for _every request_...  Since it's using a new nonce for
every request, the nonce count is always 00000001, so there is no nonce
count issue at all.  It doesn't even need to keep track of the nonce count.

Maybe we can do this as well (or something like that) for servers/proxies
that do check the nonce count?  (If a real fix isn't feasible...)

Comment 16

[An-Cheng Huang]

rbtcollins: yes, the patch works with squid-2.5.PRE2.  I didn't test
"nonce_strictness", though.  (Since if we're going to follow IE's approach,
anything will work.  Even the atoi wouldn't be a problem...)
I think an option to turn off nc checking would be useful.  Apache has
this option, even though they haven't implemented the nc checking yet...

Comment 17

[Darin Fisher]

An-Cheng: IE's approach is very very inefficient.  I think we can do better
without too much hassle... just not sure on the time frame yet :-(

Comment 18

[Darin Fisher]

there seems to be a problem with the spec.  it requires that the nonce count
strictly increases, but it also allows multiple connections to the same auth
domain.  w/ the way TCP/IP works, it's impossible to guarantee the order in
which packets on two different socket connections arrive at the server.  so,
perhaps this means we need to limit ourselves to no more than _one_ digest auth
connection per domain.  this should work and be fairly performant.  afterall,
the HTTP/1.1 spec also suggests using only one persistent connection per server.

i'm not sure at this point if this'll make it for moz 0.9.9

Comment 19

[An-Cheng Huang]

The one digest connection limit would work, though the problem of
``not increment-by-one'' still remains.  But, I don't think this
is an urgent problem...  Maybe it can be addressed after 1.0?

By the way, since moz is not handling the AuthenticationInfo header
right now (I think), should bug 116177 be reopened (as I mentioned
in comment 14)?  It's probably not an urgent problem, either,
since the RFC itself doesn't recommend the use of ``nextnonce''
(performance consideration).

Comment 20

[Darin Fisher]

An-Cheng: i can fix the problems on mozilla's end so that it always increments
by one... that shouldn't be too difficult.  the complex part is enforcing only
one connection, so... targeting post moz 1.0.

also, i've reopened bug 116177... thx for be vigilant on that one ;-)

Comment 21

[Andrew Hagen]

An article in a trade publication claims that IE 5.0 and later uses a
non-standard method for digest authentication. This method is said to be
incompatible with standards-based http servers like Apache. Thus, comparing Moz
to IE might not be entirely profitable in this instance.

http://www.eweek.com/article/0,3658,s=702&a=24177,00.asp

Comment 22

[Darin Fisher]

mass futuring of untargeted bugs

Comment 23

[Darin Fisher]

unfortunately i don't think i'm going to have time to work on this in time for
mozilla 1.1 :(

Comment 24

[Darin Fisher]

starting to look like this might not make moz 1.2 ... depends a lot on how far i
get with the NTLM work, since i think that work is going to require similar
infrastructure as needed to fix this bug.  NTLM is bug 23679.

Comment 25

[Darin Fisher]

-> moz 1.3

Comment 26

[Darin Fisher]

this is going to require some complex changes to the HTTP auth implementation.

-> future

Comment 27

[Robert Collins]

Just a heads up, in squid's bugzilla, bug #543 fixes a problem wherein a
successful auth lead to no further fails, even if the auth details where wrong
(blush!).

As a result, mozilla will request the user to re-enter their details on many
requests, because squid rejects incorrect nonce-counts (and now they get
detected correctly).

This may cause some grief :}.

Comment 28

[Robert Collins]

Oh, I should mention - the nonce_strictness patch will hopefully go in soon, to
ameliorate the impact of the mozilla bug. It really should be fixed though :}.

Lastly, as you can get an arbitrary number of nonces from an auth domain (just
send a non authed request to get a new challenge), you can trivially use a
single nonce on a single connections, thus guaranteeing safe inc-by-1..

Comment 29

[An-Cheng Huang]

If you are talking about IE 5.5's implementation I described in comment 15,
then I'm not sure what you meant by "guaranteeing safe inc-by-1" (since there
is no increment at all).  Anyway, I don't think it's a good way of
implementing this feature.  In fact, since you may as well get rid of the
nonce count in this approach, it doesn't really implement the nonce count.

By the way, have you tested the squid nonce-count checking implementation with
browsers that really increment the nonce count (which can be seen in tcpdump
traces)?  Since apache has not implemented nonce-count checking, it would be
good if we can see two independently-developed implementations interoperate
with each other (so that we have a reference point when fixing this).

darin: I think the NTLM bug is far more important than this one.  If (as you
said) work on NTLM can help solve this problem, that will be great.  Otherwise,
I think "future" is the right target.

Comment 30

[Henrik Nordstrom]

Regarding stricly increasing nonce count:

Due to request reordering which may happen due to TCP/IP and proxies the only
way to guarantee this is to never have more than one concurrent requests for the
same nonce. This includes both multiple connections and pipelined requests.

Note on pipelining and request order: pipelining is hop-by-hop, not end-to-end,
the only guarantee you have is that the replies will be sent over the connection
in the same order you sent the requests. There is no guarantee the requests will
be received or processed in the same order by the final destination.

Comment 31

[Darin Fisher]

Henrik: yeah that was my conclusion as well.  it might explain why IE chooses to
always be challenged for digest auth :-/  i'm not sure which approach is better.
 clearly extensive use of digest auth will seriously hurt page load performance
(when nonce count must be strictly increasing).

how common is strictly is nonce count ordering enforced by servers and proxies
these days?  what is squid's policy?

Comment 32

[Robert Collins]

It's important to note that RFC 2617 allows more than one outstanding nonce -
i.e. one per connection made to the origin. Proxies in the middle MUST NOT
reorder replies to requests recieved on a single connection, and SHOULD NOT use
more connections to an origin than are being made to it. So if mozilla grabs one
nonce for each connection, and then strictly increases within that connection as
each request is put onto the network, request reordering is unlikely to occur
*for that nonce*.

Request reordering is a problem for more than just strictly monotonic
incrementing nonces: It appears as a replay attack if it occurs, whether >1
increments are allowed or not.

Squid requires strict monotonic increments on each issued nonce , but will issue
a new nonce for each TCP connection. I suggest that mozilla get challenged once
on each TCP connection, and then use that nonce for that connection alone. That
will perform well for HTTP/1.1 as the issue of dropping persistent connections
on dynamic content has been resolved. (Note that squid today is still HTTP/1.0
and does not support TE - it's on our TODO).

Rob

Comment 33

[Darin Fisher]

thanks for the info robert.  sounds to me like we are going to have to make a
drastic change to our authentication architecture (once again) in order to
support this.  the biggest problem for us is that our connection management
lives entirely on a background thread, but the authentication modules live
entirely on the main thread.  only the transaction objects cross thread boundaries.

today, mozilla most likely spews out of order requests over multiple
connections, with no regard for strictly monotonically increasing nonce counts.
 we should probably do something drastic like require a challenge per
transaction (so that we never reuse a nonce) or try to somehow limit ourselves
to only one connection.  though, with even one connection we could get into
situations where things would be out of order.  if a connection happens to be
dropped before a request is sent, we'll requeue the transaction, but the
transaction will go to the tail of the queue.  the only quick solution for gecko
is to require a challenge per transaction, which is really going to hurt perf :-(

Comment 34

[Darin Fisher]

bringing this bug in... short of the quick fix i just mentioned, i don't see
this getting resolved for 1.4 final sadly.

Comment 35

[Darin Fisher]

i doubt i will have an opportunity to get this fixed for the 1.6 milestone. 
patches would definitely be appreciated.

Comment 36

[Anatoly Vorobey]

I'm seeing a variant of this bug, in which Mozilla always sends nonce count
value of 1 twice, after getting a new challenge from the server. This happens
when working against a server without any proxies; I've discovered it while
testing my custom implementation of Digest authentication. In consecutive
requests against the same server, Mozilla will issue Authorization headers with
a sequence of nonce counts being: 1, 1, 2, 3, 4.... instead of the proper 1, 2,
3, 4... I'm using Mozilla Firebird 0.7 .

I'm attaching a patch which seems to fix the bug. Note I have zero familiarity
with Mozilla source or XPCOM, I'm only going by common-sense logic; I did
recompile and test Mozilla with this fix, and it appears to work perfectly. The
fix passes sessionStateGrip instead of sessionState to authCache->SetAuthEntry
at the end of nsHttpChannel::GetCredentials, because it's sessionStateGrip that
has the correct metadata (=nonce count in digest auth) and not sessionState,
after "sessionStateGrip.swap(sessionState);" has been issued a few lines before,
right after the call to digest auth's GenerateCredentials().

Comment 37

[Anatoly Vorobey]

Attachment: Solve the problem of nonce count 1 occurring twice

Comment 38

[An-Cheng Huang]

Anatoly: your observation is similar to bug 209116 (comment14).
However, a lot has changed in the 1.187 revision of nsHttpChannel.cpp
due to bug 224749, so I'm not sure whether our observations still apply...

Comment 39

[Anatoly Vorobey]

An-Cheng: thanks for drawing my attention to this. You're right, it's the same bug .

I've investigated current CVS trunk. This bug is actually worse there. In the
current trunk code, the nonce count field in the digest auth is *always* sent as
1 - never incremented. In Firebird 0.6 and 0.7, it was: 1, 1, 2, 3, 4... , but
now it's 1, 1, 1, ... This leads to the request being denied by (discriminating)
implementations of Digest, and a new server nonce being issued with the stale
field set to true. The end result is that (nearly) every request has to be
repeated twice.

The code was shifted around in nsHttpChannel.cpp, but the actual bug remained
the same, except now it's inside the new function
nsHttpChannel::GenCredsAndSetEntry . Note that after calling
auth->GenerateCredentials() with the temporary variable ss (into which the
metadata is saved) it does "sessionState.swap(ss);" . After this point
sessionState contains the correct new metadata, while ss contains the old one
(perhaps null). Yet it is (incorrectly) ss, not sessionState, that is used in
the call to authCache->SetAuthEntry at the end of this function to save the
updated credentials.

I'm uploading a patch against current trunk code to fix this bug. Fully tested
and verified (on the current trunk tree built as Firebird) that it produces the
correct nonce count sequence. Please review, etc.

Comment 40

[Anatoly Vorobey]

Attachment: Fix the nonce count problem (checked in on trunk)

Comment 41

[Darin Fisher]

Comment on attachment 138796 [details] [diff] [review]
Fix the nonce count problem (checked in on trunk)

r+sr=darin

Comment 42

[Darin Fisher]

Comment on attachment 138796 [details] [diff] [review]
Fix the nonce count problem (checked in on trunk)

this patch has been checked  in on the trunk.  this does not fix this bug
however.  the problem still remains that we could end up sending the nonce
counts out of order.  (see previous comments for details.)

thx for discovering and fixing this particular regression!

Comment 43

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 44

[Andrei Purice]

Marking this as Resolved > Incomplete since the last real activity on this issue was 18 years ago and it might not be relevant anymore.
Feel free to re-open it if it's not the case and the issue is still relevant.