6 ms·

I still don't understand what the security issue is. In case of num < 0, malloc will be called with a negative number, which will be converted to size_t, result

I still don't understand what the security issue is. In case of num < 0, malloc will be called with a negative number, which will be converted to size_t, resulting in a huge number which will most likely fail to allocate, returning NULL. Even if it doesn't, I don't see any way to exploit this further. What am I missing?

firebacon 7y ago
Agreed; the explanation does not clarify why the first snippet is considered "totally safe" and the second one is not. How does the second snippet introduce a new security issue? A negative value for num passed into the function will always be forwarded to malloc (after going through the size_t conversion), no?
- wahern 7y ago
  Without the cast, a negative value for num wouldn't have made it past the limit constraint as the operand `num` would've been implicitly promoted to size_t (an unsigned type) in the comparison. That promotion would've reliably caused the negative value to be outside the constraint range in this case (because C has well-defined rules for signed->unsigned conversions), or in any event (if limit was >SSIZE_MAX) preserving the intended semantics viz-a-viz malloc. With the cast of limit to int, num is never promoted. A negative value can then reach the malloc expression, where it will be implicitly promoted to unsigned anyhow--because malloc takes a size_t argument. Worse, the constraint check had the side-effect of preventing multiplicative overflow in the malloc expression, which means if an attacker can control the value of num, they can cause malloc to return a block of memory smaller than what the caller expected (though this is contingent on arithmetic bugs in caller code elsewhere in the hypothetical program). The real bug here is that allocatebufs takes an int argument rather than a size_t argument. And the real issue with C's arithmetic typing isn't promotions in normal expressions (promotion to unsigned almost always preserves the intended semantics of size constraints) so much as the ability to pass arguments of the wrong signedness to functions, which has the side-effect of hiding the fact that caller and callee aren't on the same page wrt value semantics and arithmetic limits. I'm on the fence regarding C's behavior here because almost everybody in the comments instinctively were okay with casting, just that they would've casted some other expression. But the real problem here was the type of the argument to allocatebufs. It should've never been signed because negative values for object sizes are non-sensical. But because everybody's instinct is to cast, what good does stricter arithmetic type checking do for callers? Especially when simply leaving C's existing promotion rules in place, particularly implicit promotion to unsigned, would've resulted in the correct behavior. This eagerness to simply cast arithmetic values to quiet compiler diagnostics is also a problem in other languages with formally stricter type checking.
  - deleted 7y ago
    [deleted]
  - firebacon 7y ago
    > Though this is contingent on arithmetic bugs in caller code elsewhere in the hypothetical program That was my point. The article claims to show how easy it is to introduce such a bug in the second snippet, but that isn't true. You need to introduce more bugs to get a security vulnerability.
bennofs 7y ago
The negative number is multiplied by 64, so it can then underflow and become positive again.
- a3_nm 7y ago
  Ah interesting... but then what could happen? Is the risk that the program could try allocating too much memory and crash? Calling this a "backdoor" seems excessive, no? -- at worst it's just a denial of service.
  - bennofs 7y ago
    If you take (1 << 63) + 1 as size (which is a large negative int64), multiply that by 64 and you get 64. So it will call malloc(64) which will succeed. If other parts do the correct unsigned comparison then you have a heap overflow.
    - firebacon 7y ago
      This makes sense, but it also implies that there is no actual backdoor in TFA. The code as shown in the article is not exploitable without assuming more (actually exploitable) bugs somwhere else in the callsite (which the article doesn't mention). Or maybe we haven't figured out the actual vulnerability yet...
      - bennofs 7y ago
        Well, the rest of the code is also correct on its own though. If I tell the function to allocate X bytes, it should either fail or return a buffer that has space for X bytes. If the function returns less than X bytes if X happens to be a large unsigned int (that is a negative value if interpreted signed) then that is a bug and is exploitable.
        firebacon 7y ago
        But that is not what happens in the code shown in TFA. Passing a large value into the method shown in the article will do nothing nefarious (assuming sizeof(size_t) >= sizeof(int)). It will either return a large allocation, or, more likely, fail because the amount of requested memory is too large. If you have a narrowing/casting bug somewhere else in your program, which BTW would produce an obvious warning, that would of course cause trouble as you have described. And while mixing signed and unsigned arithmetic for buffer sizes is, of course, a recipe for desaster, I think it's incorrect to claim that the allocatebufs method shown in TFA has a "backdoor" because of this. I feel that is a bit like saying memcpy has a backdoor because you might get your pointer arithmetic wrong when calling it.
jackewiehose 7y ago
This alone is not a backdoor but it might lead to one. This assumes a scenario where an attacker is in full control of "num". For example: A lot of protocols have a structure like "<SIZE><PAYLOAD>" where the sending side (the attacker) specifies SIZE and the receiving side allocates a buffer of that size). The code calling that function assumes it will never create a buffer that is larger then 256 bytes, so perhaps it is doing something like this: char buf[256]; strcpy (buf, allocatebufs (ATTACKER_SIZE)) If the limit-check fails and the supplied size is in a valid range for malloc, it would be a typical buffer overflow.
kbenson 7y ago
My favorite example to trot out of how C behavior can lead to security problems is a Cap'n'Proto problem a couple years back, reported here.[1] While not entirely the same, there are some some similarities. The article submitted in that discussion does a good job explaining the problem, and the discussion covers even more possible issues. It pays to be very careful how these checks are done in C. 1: https://news.ycombinator.com/item?id=14163111 https://news.ycombinator.com/item?id=14163111

firebacon 7y ago

Agreed; the explanation does not clarify why the first snippet is considered "totally safe" and the second one is not. How does the second snippet introduce a new security issue? A negative value for num passed into the function will always be forwarded to malloc (after going through the size_t conversion), no?

wahern 7y ago
Without the cast, a negative value for num wouldn't have made it past the limit constraint as the operand `num` would've been implicitly promoted to size_t (an unsigned type) in the comparison. That promotion would've reliably caused the negative value to be outside the constraint range in this case (because C has well-defined rules for signed->unsigned conversions), or in any event (if limit was >SSIZE_MAX) preserving the intended semantics viz-a-viz malloc. With the cast of limit to int, num is never promoted. A negative value can then reach the malloc expression, where it will be implicitly promoted to unsigned anyhow--because malloc takes a size_t argument. Worse, the constraint check had the side-effect of preventing multiplicative overflow in the malloc expression, which means if an attacker can control the value of num, they can cause malloc to return a block of memory smaller than what the caller expected (though this is contingent on arithmetic bugs in caller code elsewhere in the hypothetical program). The real bug here is that allocatebufs takes an int argument rather than a size_t argument. And the real issue with C's arithmetic typing isn't promotions in normal expressions (promotion to unsigned almost always preserves the intended semantics of size constraints) so much as the ability to pass arguments of the wrong signedness to functions, which has the side-effect of hiding the fact that caller and callee aren't on the same page wrt value semantics and arithmetic limits. I'm on the fence regarding C's behavior here because almost everybody in the comments instinctively were okay with casting, just that they would've casted some other expression. But the real problem here was the type of the argument to allocatebufs. It should've never been signed because negative values for object sizes are non-sensical. But because everybody's instinct is to cast, what good does stricter arithmetic type checking do for callers? Especially when simply leaving C's existing promotion rules in place, particularly implicit promotion to unsigned, would've resulted in the correct behavior. This eagerness to simply cast arithmetic values to quiet compiler diagnostics is also a problem in other languages with formally stricter type checking.
- deleted 7y ago
  [deleted]
- firebacon 7y ago
  > Though this is contingent on arithmetic bugs in caller code elsewhere in the hypothetical program That was my point. The article claims to show how easy it is to introduce such a bug in the second snippet, but that isn't true. You need to introduce more bugs to get a security vulnerability.

bennofs 7y ago

The negative number is multiplied by 64, so it can then underflow and become positive again.

a3_nm 7y ago
Ah interesting... but then what could happen? Is the risk that the program could try allocating too much memory and crash? Calling this a "backdoor" seems excessive, no? -- at worst it's just a denial of service.
- bennofs 7y ago
  If you take (1 << 63) + 1 as size (which is a large negative int64), multiply that by 64 and you get 64. So it will call malloc(64) which will succeed. If other parts do the correct unsigned comparison then you have a heap overflow.
  - firebacon 7y ago
    This makes sense, but it also implies that there is no actual backdoor in TFA. The code as shown in the article is not exploitable without assuming more (actually exploitable) bugs somwhere else in the callsite (which the article doesn't mention). Or maybe we haven't figured out the actual vulnerability yet...
    - bennofs 7y ago
      Well, the rest of the code is also correct on its own though. If I tell the function to allocate X bytes, it should either fail or return a buffer that has space for X bytes. If the function returns less than X bytes if X happens to be a large unsigned int (that is a negative value if interpreted signed) then that is a bug and is exploitable.
      - firebacon 7y ago
        But that is not what happens in the code shown in TFA. Passing a large value into the method shown in the article will do nothing nefarious (assuming sizeof(size_t) >= sizeof(int)). It will either return a large allocation, or, more likely, fail because the amount of requested memory is too large. If you have a narrowing/casting bug somewhere else in your program, which BTW would produce an obvious warning, that would of course cause trouble as you have described. And while mixing signed and unsigned arithmetic for buffer sizes is, of course, a recipe for desaster, I think it's incorrect to claim that the allocatebufs method shown in TFA has a "backdoor" because of this. I feel that is a bit like saying memcpy has a backdoor because you might get your pointer arithmetic wrong when calling it.

jackewiehose 7y ago

This alone is not a backdoor but it might lead to one. This assumes a scenario where an attacker is in full control of "num". For example: A lot of protocols have a structure like "<SIZE><PAYLOAD>" where the sending side (the attacker) specifies SIZE and the receiving side allocates a buffer of that size). The code calling that function assumes it will never create a buffer that is larger then 256 bytes, so perhaps it is doing something like this: char buf[256]; strcpy (buf, allocatebufs (ATTACKER_SIZE)) If the limit-check fails and the supplied size is in a valid range for malloc, it would be a typical buffer overflow.

kbenson 7y ago

My favorite example to trot out of how C behavior can lead to security problems is a Cap'n'Proto problem a couple years back, reported here.[1] While not entirely the same, there are some some similarities. The article submitted in that discussion does a good job explaining the problem, and the discussion covers even more possible issues. It pays to be very careful how these checks are done in C. 1: https://news.ycombinator.com/item?id=14163111 https://news.ycombinator.com/item?id=14163111