Bug 5179: Correction of CVE 2021-28116 fix #953
Conversation
|
Can one of the admins verify this patch? |
squid-anubis
added
the
M-failed-description
| @@ -1476,6 +1486,9 @@ wccp2HandleUdp(int sock, void *) | |||
|
|
|||
| SetField(cache_identity, ptr, router_view_header, router_view_size, | |||
| "malformed packet (truncated router view info cache w/o assignment hash)"); | |||
| if (cache_identity->bits & 6 >> 1 != 0) { | |||
| fatalf("Assignment type mismatch in router view"); | |||
There was a problem hiding this comment.
This needs to throw instead. The core problem behind CVE 2021-28116 was letting values supplied by routers abort Squid.
There was a problem hiding this comment.
My understanding of the problem was that a broken/fake packet could access memory beyond its size.
Initially I used Must3 like at other places, but the outcome was not good (throw after some struct updated caused list of caches corrupted leading to segfault during processing of next packet) or infinite loop of negotiation attempt and ignored message, like the current situation.
I preferred to let the cache stop entirely over continuing with wccp disabled. It should happen during initial setup only. If that is not a good approach, I would also reinitialize wccp prior exception and force negotiation from scratch.
There was a problem hiding this comment.
The code parsing received traffic should not reach fatal(). In the vast majority of cases, when throwing breaks things, it is not throwing that should be blamed -- the bug is elsewhere (and should be fixed elsewhere). Moreover, IIRC, this code already throws, even before this PR changes. Thus, even if this PR avoids throwing, the post-throwing problems you have observed will probably continue to exist; you are not really eliminating them.
It should happen during initial setup only
If not today, then tomorrow that "initial setup" event might happen ten days after the Squid instance has started (e.g., as a result of a reconfiguration).
I would also reinitialize wccp prior exception and force negotiation from scratch.
Please handle exceptions when/after catching them, not when/prior to throwing them. Adding a try/catch may be necessary -- I have not checked.
There was a problem hiding this comment.
Ah. The design of the logic is that a thrown exception when handling a packet aborts handling only that packet. Any information taken out of it should only be saved after validation of that piece of information.
Since WCCP sends a bunch of settings Squid is able to accept one piece (eg return method) while rejecting others (eg router list) provided the one used is valid regardless of the rejected piece. When there is any dependency situation between packet values they should be kept in local variables until it is all validated. Not added to the operational config as separate updates.
yadij
changed the title
|
OK to test |
squid-anubis
removed
the
M-failed-description
yadij
added
the
S-waiting-for-author
|
@amk1969 please indicate whether you are able to do the requested changes. If not I will pick this up in the next few days and push towards merge. We will still need you to assist with testing the result either way. Thank you. |
Thanks in advance |
|
@amk1969, I have finally done this. Since I am reluctant to touch your repository "master" branch I have opened PR #970 with the combo of our parts. Please test #970 when you can.
|
|
Comments from testing added to your PR #970. |
The CVE changes did not handle received WCCP packets correctly.
This update allows to operate again, to the level squid
implemented WCCPv2 before. Tested with one cache and one router,
in both hash and mask assignment modes.