Incorrect error handling in httpGets2

[xypron]

Describe the bug

cups-browsed runs into an infinite loop in httpGets.

To Reproduce

The issues was sometimes observed in cups-browsed on Ubuntu 24.04. See https://bugs.launchpad.net/ubuntu/+source/cups-browsed/+bug/2049315.

Expected behavior

httpGets2 should not enter an endless loop if a connection error occurs or the webserver provides invalid data.

System Information:

• OS and its version: Ubuntu 24.04
• Application cups-browsed
• CUPS version 2.4.6

Additional context

Debugging showed negative values of http->used in httpGets.

Some error paths lead to negative numbers being added to http->used. The following diff should avoid this:

diff --git a/cups/http.c b/cups/http.c
index c154412b1..57db33ae3 100644
--- a/cups/http.c
+++ b/cups/http.c
@@ -1091,6 +1091,7 @@ httpGets2(http_t *http,                   // I - HTTP connection
            continue;
 
          http->error = WSAGetLastError();
+         return (NULL);
        }
        else if (WSAGetLastError() != http->error)
        {
@@ -1113,6 +1114,7 @@ httpGets2(http_t *http,                   // I - HTTP connection
            continue;
 
          http->error = errno;
+         return (NULL);
        }
        else if (errno != http->error)
        {

[zdohnal]

The discussion happens on the PR, setting the correct label.

[michaelrsweet]

[master c89dd90] Fix httpGets timeout handling (Issue #879)

[2.4.x cde48f0] Fix httpGets timeout handling (Issue #879)

[tillkamppeter]

Unfortunately, the fix for this bug is not actually working. In Ubuntu 24.04 the fix is applied but the bug still occurs. See

https://bugs.launchpad.net/ubuntu/+source/cups-browsed/+bug/2067918

and there expecially comment #10

https://bugs.launchpad.net/ubuntu/+source/cups-browsed/+bug/2067918/comments/10

[tillkamppeter]

Many users of Ubuntu 24.04 are still reporting this very same problem.

[jknockel]

Unfortunately, the fix for this bug is not actually working. In Ubuntu 24.04 the fix is applied but the bug still occurs.

Are you sure that the fix has been applied in Ubuntu 24.04? I looked at the source (via apt-get source) and did not see the changes applied.

[michaelrsweet]

@tillkamppeter Can you respond to @jknockel ?

[tillkamppeter]

Sorry for the late reply. I was very busy, had some days off, a conference, and now there was also the vulnerability.

I have checked now and the upstream introduction of the fix was in 2.4.8 and 24.04 has only 2.4.7, and I see no distro patch in 2.4.7 which backports the fix. Probably the fix was too late for the release of 24.04 LTS.

We need to somehow test it, in 24.10 (which has CUPS 2.4.10 which contains it) and perhaps also a 2.4.7 with backported fix in a PPA for 24.04, to check whether one could do an SRU.

[tillkamppeter]

I got somebody reporting to still have the cups-browsed issue on Ubuntu 24.10 with CUPS 2.4.10:

https://bugs.launchpad.net/ubuntu/+source/cups-browsed/+bug/2049315/comments/29

As CUPS 2.4.10 contains the changes described in this issue report, it seems that the change does not fix the issue.

[tillkamppeter]

For me it also looks like that the patch is without effect, as if the added return (NULL); in line 1245 (2.4.x branch) had not been there, the return (NULL); in line 1254 had been reached, so presence or absence of the patch had the same result. Am I right?

[michaelrsweet]

@tillkamppeter According to the bug report, http->used is going negative, which should never happen. Are there any patches being applied to cups/http.c for the Debian/Ubuntu CUPS release?

[tillkamppeter]

I have checked now and the Ubuntu package of CUPS does not apply any patches to cups/http.c. The relevant parts of the file are identical with the original (only difference is your _cupsSetError() one-liner change. due to the fact that Ubuntu uses 2.4.10 and not .11).

I also had a look through the code of cups/http.c. It is not possile with it to make http->used negative and with the data structure being opaque there is no direct access fot external programs which just use libcups. Looks rather like an uninitialized data structure used somewhere.

[tillkamppeter]

It is easy to see when reviewing the code of httpGets() that if, when the function is called, http->used is negative, nothing of the code inside the outer while loop, while (lineptr < lineend), does anything: while (http->used == 0) {...] -> for (bufptr = http->buffer, bufend = http->buffer + http->used; lineptr < lineend && bufptr < bufend;) {...} (with http->used < 0 there is bufptr > bufend) -> http->used -= (int)(bufptr - http->buffer); if (http->used > 0) {...} (http->used does not get changed here) -> if (eol) {...} (for loop did not run, so no EOL found), So we have a perfect infinite loop not logging anything.

The code of cups-browsed (daemon/cups-browsed.c) creates the http variable always with httpConnect2(host, port, NULL, AF_UNSPEC, encryption, 1, 3000, NULL) which should create a valid http_t data structure or NULL, where in the latter case httpGets() returns right in the beginning.

So only thing which could cause http->used be negative is something like a buffer overflow, http being overwritten.