5 ms·

Actually looking at the fix for this: https://github.com/curl/curl/pull/4166/commits/a42a763ebde64498f699af7938ad815cdea98e2d https://github.com/curl/curl/pull

Actually looking at the fix for this: https://github.com/curl/curl/pull/4166/commits/a42a763ebde64498f699af7938ad815cdea98e2d https://github.com/curl/curl/pull/4166/commits/a42a763ebde64... It seems the code is using a signed long. Unfortunately, in C, signed overflow is undefined behavior. Anything can happen, and you should not assume that it is just benign behavior like wraparound. Undefined behavior, in a seemingly unrelated area, can result in a security issue somewhere else in the program thanks to how C deals with undefined behavior. While I don’t think it is as severe as the CVE indicated, something that triggers undefined behavior in C is not to be dismissed as lightly as the article is doing.

ronsor 3y ago
In C, almost everything is undefined behavior. I think it's really hard to write a C program without it.
soraminazuki 3y ago
So anything that is undefined on paper automatically gets a near maximum score? That doesn't seem right. I'm aware there are cases of overzealous compilers removing conditional code based on the incorrect assumption that overflows can never happen, but this doesn't seem to be the case here.
lifthrasiir 3y ago
You are technically correct, but signed integer overflows are so pervasive in C that compilers only exploit them when ignoring them would be truly beneficial. In the case of LLVM for example signed integer overflows will merely produce a poison value, and won't produce the actual UB if it is either optimized out or only used by instructions that can't produce UBs by themselves [1]. Compilers also tend to use the UB as an excuse to optimize things, not to break things. We only care about the UB because a faulty optimization still can cause breakages. In this particular case the poison value will eventually reach the following condition [2]: /* if retry-max-time is non-zero, make sure we haven't exceeded the time */ if(per->retry_numretries && (!config->retry_maxtime || (tvdiff(tvnow(), per->retrystart) < config->retry_maxtime*1000L)) ) { So let's assume that `config->retry_maxtime * 1000L` was indeed always a poison value---what can reasonable optimizing compilers do? At worst they will probably evaluate the entire condition as false, because it would help the dead code elimination. Compilers can put a nasal demon in that case, but reasonable ones wouldn't. And since `config->retry_maxtime * 1000L` is not guaranteed to be a poison value all the time, this optimization is not even actually possible. The practical effect of this security bug is quite limited. [1] https://llvm.org/docs/LangRef.html#poison-values https://llvm.org/docs/LangRef.html#poison-values [2] https://github.com/curl/curl/blob/c2212c05aa99bb31e4b99b0b66fc1747b7d01be6/src/tool_operate.c#L473-L478 https://github.com/curl/curl/blob/c2212c05aa99bb31e4b99b0b66... (similar for `config->retry_delay`, omitted for brevity here)
- wizeman 3y ago
  > The practical effect of this security bug is quite limited. You are probably correct (although it's hard to say for sure that you've considered every single compiler optimization). But even if you are correct, the point of the parent poster still stands: > something that triggers undefined behavior in C is not to be dismissed as lightly as the article is doing. I agree with this. Notice how long your explanation was. The author of the article did not provide a similar explanation, or even an abbreviated one, in order to justify the claims that the bug does not have security implications. In fact, the author didn't even have to provide such an explanation, but he could have at least acknowledged the possibility, which could then be argued to be dismissable based on his (extensive) experience. I'm not saying that there are security implications or that the burden of proving that there aren't any is on the author of the article. I'm only agreeing with the parent poster that you cannot easily or simply claim that the bug is not a security vulnerability, without giving some consideration to the possible impact of UB, or at least acknowledging the possibility. Not acknowledging this possible impact can in fact reinforce misconceptions about the security implications of bugs in C code that lead to UB.
  - lifthrasiir 3y ago
    > The author of the article did not provide a similar explanation, or even an abbreviated one, in order to justify the claims that the bug does not have security implications. Curl already runs ubsan and fuzzer in its testing process [1]. They know what they are doing (you may disagree on the first principles of writing curl in C/C++, but it's not like that curl is not sufficiently aware of UBs), so you should have asked instead why this particular bug was not caught already. I believe the answer is that this bug occurred in the curl's CLI interface (src/tool_*.c), not in libcurl. The current fuzzer only runs against libcurl so this bug could have slipped in. But then it is even more clear that this bug can't be that serious, because libcurl does all the heavy lifting! Integer overflows themselves are not significant, they have to be paired with other mechanisms to be actually vulnerable (and Rust also agrees, whether overflow checks are enabled or not does not affect the memory safety). Unless reporters have figured out an ingenious way to exploit a single integer overflow, the OP's reaction is completely justified. [1] https://daniel.haxx.se/blog/2021/12/13/keeping-curl-safe/ https://daniel.haxx.se/blog/2021/12/13/keeping-curl-safe/

ronsor 3y ago

In C, almost everything is undefined behavior. I think it's really hard to write a C program without it.

soraminazuki 3y ago

So anything that is undefined on paper automatically gets a near maximum score? That doesn't seem right. I'm aware there are cases of overzealous compilers removing conditional code based on the incorrect assumption that overflows can never happen, but this doesn't seem to be the case here.

lifthrasiir 3y ago

You are technically correct, but signed integer overflows are so pervasive in C that compilers only exploit them when ignoring them would be truly beneficial. In the case of LLVM for example signed integer overflows will merely produce a poison value, and won't produce the actual UB if it is either optimized out or only used by instructions that can't produce UBs by themselves [1]. Compilers also tend to use the UB as an excuse to optimize things, not to break things. We only care about the UB because a faulty optimization still can cause breakages. In this particular case the poison value will eventually reach the following condition [2]: /* if retry-max-time is non-zero, make sure we haven't exceeded the time */ if(per->retry_numretries && (!config->retry_maxtime || (tvdiff(tvnow(), per->retrystart) < config->retry_maxtime*1000L)) ) { So let's assume that `config->retry_maxtime * 1000L` was indeed always a poison value---what can reasonable optimizing compilers do? At worst they will probably evaluate the entire condition as false, because it would help the dead code elimination. Compilers can put a nasal demon in that case, but reasonable ones wouldn't. And since `config->retry_maxtime * 1000L` is not guaranteed to be a poison value all the time, this optimization is not even actually possible. The practical effect of this security bug is quite limited. [1] https://llvm.org/docs/LangRef.html#poison-values https://llvm.org/docs/LangRef.html#poison-values [2] https://github.com/curl/curl/blob/c2212c05aa99bb31e4b99b0b66fc1747b7d01be6/src/tool_operate.c#L473-L478 https://github.com/curl/curl/blob/c2212c05aa99bb31e4b99b0b66... (similar for `config->retry_delay`, omitted for brevity here)

wizeman 3y ago
> The practical effect of this security bug is quite limited. You are probably correct (although it's hard to say for sure that you've considered every single compiler optimization). But even if you are correct, the point of the parent poster still stands: > something that triggers undefined behavior in C is not to be dismissed as lightly as the article is doing. I agree with this. Notice how long your explanation was. The author of the article did not provide a similar explanation, or even an abbreviated one, in order to justify the claims that the bug does not have security implications. In fact, the author didn't even have to provide such an explanation, but he could have at least acknowledged the possibility, which could then be argued to be dismissable based on his (extensive) experience. I'm not saying that there are security implications or that the burden of proving that there aren't any is on the author of the article. I'm only agreeing with the parent poster that you cannot easily or simply claim that the bug is not a security vulnerability, without giving some consideration to the possible impact of UB, or at least acknowledging the possibility. Not acknowledging this possible impact can in fact reinforce misconceptions about the security implications of bugs in C code that lead to UB.
- lifthrasiir 3y ago
  > The author of the article did not provide a similar explanation, or even an abbreviated one, in order to justify the claims that the bug does not have security implications. Curl already runs ubsan and fuzzer in its testing process [1]. They know what they are doing (you may disagree on the first principles of writing curl in C/C++, but it's not like that curl is not sufficiently aware of UBs), so you should have asked instead why this particular bug was not caught already. I believe the answer is that this bug occurred in the curl's CLI interface (src/tool_*.c), not in libcurl. The current fuzzer only runs against libcurl so this bug could have slipped in. But then it is even more clear that this bug can't be that serious, because libcurl does all the heavy lifting! Integer overflows themselves are not significant, they have to be paired with other mechanisms to be actually vulnerable (and Rust also agrees, whether overflow checks are enabled or not does not affect the memory safety). Unless reporters have figured out an ingenious way to exploit a single integer overflow, the OP's reaction is completely justified. [1] https://daniel.haxx.se/blog/2021/12/13/keeping-curl-safe/ https://daniel.haxx.se/blog/2021/12/13/keeping-curl-safe/