Bug 5179: Correction of CVE 2021-28116 fix

[yadij]

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.

The safe use of exceptions required some refactoring to reduce scope of temporary variables and std::unique_ptr to destruct temporary memory in a clean manner.

Refactoring to STL types and even better list management is left out of scope. Initial review implies that would touch a lot of other WCCP logic. This is (attempting, approximately) minimal change for the bug fix.

Todo:

• test and find runtime issues
• audit the WCCP2_I_SEE_YOU router list parse for similar issues

[amk1969]

I testing this code against a Cisco router. There are some troubles... With hash assignment, one cache is ignored and a broken assignment packet is sent to router. This change fixes it, not sure how to propose with github...

--- a/src/wccp2.cc
+++ b/src/wccp2.cc
@@ -1849,7 +1849,7 @@ wccp2AssignBuckets(void *)
                 if (num_caches) {
                     int cache;
 
-                    for (cache = 0, cache_list_ptr = &router_list_ptr->cache_list_head; cache_list_ptr->next; cache_list_ptr = cache_list_ptr->next, ++cache) {
+                    for (cache = 0, cache_list_ptr = &router_list_ptr->cache_list_head; cache_list_ptr; cache_list_ptr = cache_list_ptr->next, ++cache) {
                         /* add caches */

For mask assignment, one cache/router works, but I fear that in case more get added one gets ignored as well. Need to run more instances, hopefully tomorrow.

BTW, would it be possible to make mask configurable and not hardcoded as 0x1741? Less bits set in the mask saves resources at the router side and makes smaller packets.

[amk1969]

The diff from yesterday is not sufficient. Running two caches and two routers again results in malformed packet:

    Assignment Info
        Type: Assignment Info (6)
        Length: 300
        Assignment Key IP Address: 192.168.1.4
        Assignment Key Change Number: 4
        Number of Routers: 2
        Router IP: 192.168.2.1
        Router IP: 192.168.1.1
        Number of WC: 2
            WC IP: 192.168.1.4 id: 0
            WC IP: 0.0.0.0 id: 1
[Malformed Packet: WCCP]

The IP of second cache is not output, first 4 buckets are interpreted as IP address and there are not enough buckets. Also the buckets themselves are all 0, while half should be going to the other cache.

Further troubles noticed: There is trouble with address comparison in hash mode. cache with IP 192.168.1.3 logged: 2022/02/13 11:33:16.788 kid1| 80,5| wccp2.cc(1575) wccp2HandleUdp: I am not the lowest ip cache - not assigning buckets The other created bad assignment above.

In the mask assignment mode, the lower IP created it, but it sends different assignment to each router - one router is instructed to redirect everything to one cache, other to the other. This is not how it should work.

I also notice that the wccp assignment packets are created for each router separately. This can be optimized - actually all routers/caches should get the same wccp messages, the protocol supports also multicast.

In all HIA messages the web cache view contains Number of Web Caches=0. This does not lead to problem with operation, but it is confusing when checking the packets.

[rousskov]

@amk1969, thank you for testing this and suggesting fixes! Clearly, a lot more needs to be done before this code becomes production-worthy. I do not plan to resume working on WCCP myself until summoned, but GitHub should notify you when there are new PR commits.

[yadij]

To make some more progress I have now rebased this PR on recent master branch and solved several of the compile issues being identified by GCC-12.

One GCC warning revealed bad bitmask handling of flags that would have resulted in attempted mask assignments being rejected as invalid or interpreted as hash assignment. Hash may have worked due to a fluke overlap of flags meanings. I have verified that the correct flag bits are being used now - but not able to test how this alters the protocol negotiation. This may be related to the strange router 0.0.0.0 appearance and WC=0 mentioned by https://github.com/squid-cache/squid/pull/970#issuecomment-1038020144 (eg mask assignment bucket(s) being interpreted as hash assignment buckets)

Three GCC errors remain all of type: "writing to an object of type ‘struct wccp2_cache_list_t’ with no trivial copy-assignment" I hope to have more time to look into implementing copy assignment properly in a few days.

[callumapplsys]

Updating the PR here as I noticed there was some activity on the thread I started on the squid-users mailing list. I was able to modify this patch to get it to build without the three GCC errors referring to the wccp2_cache_list_t struct, and talk successfully to the Cisco router, at least initially.

However I had to comment out lines 2584 to 2596 in wccp2.cc which is the "Swap if we need to" section of the wccp2SortCacheList function. I'm trying to understand why the cache list needs to be sorted to attempt to fix it. From section 3.9 of this WCCP spec, it suggests that the lowest IP cache is selected as the designated cache for a given service group. Is that what this code is for? Is it possible that given my single router setup I would not see the consequences of this change? See https://datatracker.ietf.org/doc/html/draft-param-wccp-v2rev1-01#section-3.9

There are also two other issues I have noted so far in testing:

• In doing a packet capture I can see the HERE_I_AM, and I_SEE_YOU messages as expected, but the REDIRECT_ASSIGNMENT message is being flagged as corrupted in Wireshark. It says "Malformed packet (exception occurred)".
• When exiting Squid there is an invalid pointer which causes it to abort:

2023/08/23 11:36:51| FD 12 Closing WCCPv2 socket
free(): invalid pointer
Aborted

Running on Ubuntu 20.04.6 LTS with a GRE tunnel to a Cisco 2821