Fix Auth::User::absorb() IP list transfer logic

[MegaManSec]

Detach IP from source list before reattaching to destination and
decrement ipcount. This prevents list corruption and counter mismatch.

[MegaManSec]

Note: It seems that here there may be a null pointer deref, and this code block will just continuously loop if it ever happens? (I do not see how from->ip_list.head will advance) (I also don't understand why new_ipdata is not removed from the list (afaict).)
I did not investigate further.

[yadij]

FWIW; these dlinkList need replacing with a std::list or more appropriate container. That conversion alone gets rid of a lot of risky logic.

[yadij]

"good enough for now".

[rousskov]

I agree that official code is buggy as claimed in the first paragraph of PR description. AFAICT, the ipcount increment bug originates in 2012 commit 742a021 while the list node moving bug goes all the way to 2010 commit 56a49fd.

I also share grave concerns expressed in #2194 (comment)

I can also add a speculation that the affected code never runs because if it were to run, ipcount would become inflated, and Auth::User::clearIp() would assert() during Auth::User object destruction.

In summary, both found and !found branches probably represent broken, untested, and currently unused code that will cause a lot of problems when/if it suddenly starts being used after some other code changes!

The absorb() method is called from src/auth/negotiate and src/auth/ntlm code. The calls only have cached_user as a precondition AFAICT. It may be very useful to know why this code never runs today (if it indeed never runs). @yadij, do you know whether this part of authentication code might run in some deployment environments today and, if not, why not?

PR description: Prevents ... and counter underflow.

@MegaManSec, did you mean "overflow" (from excessive increments)? Or perhaps there is both overflow and underflow here, depending on the corrupted list? Replace with "wrong ipcount levels" or something like that?

[MegaManSec]

PR description: Prevents ... and counter underflow.
@MegaManSec, did you mean "overflow" (from excessive increments)? Or perhaps there is both overflow and underflow here, depending on the corrupted list? Replace with "wrong ipcount levels" or something like that?
Indeed, I really meant an underflow here:

squid/src/auth/User.cc

Lines 150 to 152 in f9ba4e1

	/* catch incipient underflow */
	assert(ipcount);
	-- ipcount;

I think "counter mismatch" is a better phrase. I will update.

[yadij]

@rousskov; I may have missed something, but AFAIK this code runs in all deployments using authentication. When the user logs in a second time it merges the second login details into the pre-existing details. That should be true even if the user-IP caching is disabled.

FWIW, I have now opened PR #2221 to replace this dlinkList entirely.

[rousskov]

Amos: I may have missed something, but AFAIK this code runs in all deployments using authentication.

To me, it looks like this absorb() code is supposed to run in many deployments using NTML and negotiate authentication (other schemes do not call absorb() AFAICT). Your answer partially supports that theory AFAICT. However, the bugs and concerns discussed in this PR suggest (but do not guarantee) that at least some critical parts of this code never run at all (e.g., because they would trigger an infinite loop or assertions if they do run). This is a big red flag for accepting PRs that improve this code -- if this code does not run, then those improvements are effectively untested.

FWIW, I have now opened PR #2221 to replace this dlinkList entirely.

I will continue this discussion there. If my "NTML and negotiate authentication" assumption is correct, then the changes in this PR can go in without testing if you are OK with it -- just clear this PR for merging.

[yadij]

Recheck now that our CI issues with OpenBSD are fixed.

[squidadm]

queued for backport to v7

[squidadm]

queued for backport to v7

[squidadm]

queued for backport to v7

[squidadm]

queued for backport to v7

[squidadm]

queued for backport to v7

[squidadm]

queued for backport to v7

[MegaManSec]

@yadij @rousskov i think the bot is broken

[rousskov]

i think the bot is broken

I assume you are referring to software that left repeated "queued for backport to v7" comments. I do not disagree with your assessment but cannot help with software that leaves those comments, and there are serious disagreements over how backporting should be automated. Anubis (i.e. the merge bot I am responsible for) does not do backporting (yet?).