curl: CVE-2021-22925: TELNET stack contents disclosure again

Summary:

CVE-2021-22898: TELNET stack contents disclosure (#1176461) issue was recently reported for curl and it was addressed in curl 7.77.0:

https://curl.se/docs/CVE-2021-22898.html
https://github.com/curl/curl/commit/39ce47f219b09c380b81f89fe54ac586c8db6bde
https://hackerone.com/reports/1176461

However, the fix applied is not correct and does not completely address the issue. It helps in cases when long environment variable name is used ('a'*256 + ',b'), but not when the name is short and only the value is long ('a,' + 'b'*256, which is the example mentioned in the curl project advisory).

Steps To Reproduce:

Follow the steps form #1176461, only use NEW_ENV option with short name and long value, such as:

$ curl telnet://127.0.0.1:23 -t NEW_ENV=`python -c "print('a,' + 'b'*256)"`

Supporting Material/References:

When parsing NEW_ENV option value with short name and long value, sscanf() returns 2, as it writes to both varname and varval, even though the data in varval is truncated. Hence such variable is not skipped and is added to the temp[] buffer. However, the len counter which tracks the amount of data that was already written to temp[] is not updated based on the data written to the buffer in the msnprintf()call, but rather based on the length of the original unparsed data that is stored in tmplen. The relevant code is here:

https://github.com/curl/curl/blob/curl-7_77_0/lib/telnet.c#L926-L929

When value stored in varval is truncated, len is increased too much and a chunk of uninitialized memory is created in temp[]. The len should only be incremented by strlen(varname) + strlen(varval) + 2.

I wonder if the original fix should be preserved or re-worked. In addition to not fixing the info leak problem properly, it also causes certain valid option values to be ignored and not sent to a server any more. Rejected values are of the forms NEW_ENV=a or NEW_ENV=a,. At least the second one seems like an obviously valid way to set variable a to an empty string. RFC 1572 defines that environment variable can be sent with empty value and hence NEW_ENV=a, should remain supported. It also defines that variable can be sent with no value, making NEW_ENV=a a valid option as well. Note that curl prior to 7.77.0 actually did handle NEW_ENV=a that way, but it looks more like an unintended side effect of how len was incremented by tmplen, as the empty value part was written to temp[] and only subsequently overwritten. As the telnet protocol support in curl is not likely to be used widely these days, possibly only to interact with some legacy systems, it seems reasonable to prefer a fix that changes behaviour as little as possible.

Impact

Leak of an uninitialized stack memory.

Report #1176461 and the matching curl advisory provide some estimates on how much data can be leaked. I believe the amount of leaked data is smaller and is less than a half of the temp[] size. The reason for that is in the check_telnet_options() where option arguments are truncated to 255 characters, and at least half of that must part of the defined variable name or value.

https://github.com/curl/curl/blob/curl-7_77_0/lib/telnet.c#L799-L800