6 ms·

For those wondering how this works: In C, like most languages, all binary operators on numbers – including arithmetic as well as comparisons – 'natively' requi

by comex 7y ago

For those wondering how this works: In C, like most languages, all binary operators on numbers – including arithmetic as well as comparisons – 'natively' require two operands of the same type. When you pass operands of different types, the language usually converts one of the operands to the other's type. (Sometimes it converts both operands to a different type instead.) In this case, without the cast, the comparison will be performed by converting the `int` to `size_t`. The dangerous case is when `num` is negative. Since `size_t` is unsigned, the conversion will turn negative numbers into very high positive numbers, so `num > limit` will be true, and the function will safely reject the allocation. On the other hand, with `num > (int)limit`, no conversion occurs; the comparison is performed between two signed integers, and `num > limit` will be false if `num` is negative. It would be better to be more explicit. Perhaps use two comparisons (the compiler will optimize them into one if possible): if (num < 0 || num > limit) Or, better yet, make `allocatebufs` take `size_t` instead of `int` in the first place. Taking `int` is risky even without the comparison bug: if someone calls the function with a `size_t` on a platform where `size_t` is larger than `int` (i.e. 64-bit platforms), the upper bits will be truncated. For instance, if the passed-in value is 0x100000001, it will be truncated to 0x1, causing allocatebufs to return a much smaller buffer than expected. If you're wondering about the exact rules for what types get converted to what, they're very complicated, unnecessarily so. It mostly boils down to "larger type wins; in case of a tie, unsigned wins over signed". But there are exceptions. Here's the spec language: - http://c0x.coding-guidelines.com/6.3.1.8.html http://c0x.coding-guidelines.com/6.3.1.8.html - http://c0x.coding-guidelines.com/6.3.1.1.html http://c0x.coding-guidelines.com/6.3.1.1.html

Liquix 7y ago
Thank you for the informative and thorough explanation!
thegeomaster 7y ago
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.
alanfranz 7y ago
> Taking `int` is risky even without the comparison bug: That's the whole point of the article. Risky, but not wrong. A subtle way to introduce a backdoor.
- deleted 7y ago
  [deleted]
nayuki 7y ago
> When you pass operands of different types, the language usually converts one of the operands to the other's type. (Sometimes it converts both operands to a different type instead.) Indeed. The pathological example where both operands get converted to a different type is like this: Assume that ‹int› is 32 bits and ‹long› is 32 bits. Then ‹unsigned int› + ‹long› becomes ‹unsigned long›. This is because ‹long› has a higher rank than ‹int›, so the result must have the rank of long. But because ‹long› cannot hold all values of ‹unsigned int›, we must use ‹unsigned long›.
- comex 7y ago
  Yep. Also, if both operands have types smaller than int, they’ll both be converted to int as part of the “integer promotions” step. This is less likely to lead to surprising results since the conversion is value-preserving, but it’s inconsistent with the behavior of int-and-larger types.
- ohazi 7y ago
  I've heard a lot about NULL being considered a multi-billion-dollar mistake, but not a lot about the wishy-washy integer types hierarchy. The more explicit stdint types (e.g. uint32_t) are both safer and more portable. Modern languages like Rust seem to agree... (u8, i32, etc). The argument I've always heard was that you want your program to use the sizes that the architecture can handle most efficiently. But actually, I don't. I'd rather my program run correctly, the way I intended, even if that means it runs a little slower on a sparc or powerpc. The combinatoric explosion of size hierarchies is basically impossible for normal people to reason about or test for.
  - Gibbon1 7y ago
    Yeah things I would get rid of in c is 'int' and the wishy washy integer type non sense that goes with it. The whole thing was a big pain in the ass until pure 32 bit machines came along and everyone forgot. Now it's a pain in the ass again. I'd also add range types ala Ada.
    - noneeeed 7y ago
i_cant_speel 7y ago
Great explanation. I'm confused why the author would just assume their readers would know this without any kind of explanation.
- derpherpsson 7y ago
  It made me stop and think about it for a while. It feels like I learned on a deeper level than if s/he had just explained it. . . . But the takeaway here is, I think, not to make us better C programmers. I think the takeaway is that C sucks because it just smiles at esoteric bugs like this. We need to replace it, or forever suffer from crashes and RCEs, et.c.
AceJohnny2 7y ago
> in case of a tie, unsigned wins over signed As a career C programmer, this drives me crazy. I kind of get it from a object storage size perspective, but the side-effects are awful. But of course, it's too late to fix it. Better just burn it all down and switch to something completely different. Oh hey, Rust!
- pornel 7y ago
  TBH, Rust's other extreme of never converting any numeric type anywhere is also annoying, and leads to overuse of casts and/or accidental truncation. Fortunately, memory allocation in Rust is easier, and the whole `malloc(elem * size)` chore just doesn't exist.

by comex 7y ago

Liquix 7y ago

Thank you for the informative and thorough explanation!

thegeomaster 7y ago

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.

alanfranz 7y ago

> Taking `int` is risky even without the comparison bug: That's the whole point of the article. Risky, but not wrong. A subtle way to introduce a backdoor.

deleted 7y ago
[deleted]

nayuki 7y ago

> When you pass operands of different types, the language usually converts one of the operands to the other's type. (Sometimes it converts both operands to a different type instead.) Indeed. The pathological example where both operands get converted to a different type is like this: Assume that ‹int› is 32 bits and ‹long› is 32 bits. Then ‹unsigned int› + ‹long› becomes ‹unsigned long›. This is because ‹long› has a higher rank than ‹int›, so the result must have the rank of long. But because ‹long› cannot hold all values of ‹unsigned int›, we must use ‹unsigned long›.

comex 7y ago
Yep. Also, if both operands have types smaller than int, they’ll both be converted to int as part of the “integer promotions” step. This is less likely to lead to surprising results since the conversion is value-preserving, but it’s inconsistent with the behavior of int-and-larger types.
ohazi 7y ago
I've heard a lot about NULL being considered a multi-billion-dollar mistake, but not a lot about the wishy-washy integer types hierarchy. The more explicit stdint types (e.g. uint32_t) are both safer and more portable. Modern languages like Rust seem to agree... (u8, i32, etc). The argument I've always heard was that you want your program to use the sizes that the architecture can handle most efficiently. But actually, I don't. I'd rather my program run correctly, the way I intended, even if that means it runs a little slower on a sparc or powerpc. The combinatoric explosion of size hierarchies is basically impossible for normal people to reason about or test for.
- Gibbon1 7y ago
  Yeah things I would get rid of in c is 'int' and the wishy washy integer type non sense that goes with it. The whole thing was a big pain in the ass until pure 32 bit machines came along and everyone forgot. Now it's a pain in the ass again. I'd also add range types ala Ada.
  - noneeeed 7y ago

i_cant_speel 7y ago

Great explanation. I'm confused why the author would just assume their readers would know this without any kind of explanation.

derpherpsson 7y ago
It made me stop and think about it for a while. It feels like I learned on a deeper level than if s/he had just explained it. . . . But the takeaway here is, I think, not to make us better C programmers. I think the takeaway is that C sucks because it just smiles at esoteric bugs like this. We need to replace it, or forever suffer from crashes and RCEs, et.c.

AceJohnny2 7y ago

> in case of a tie, unsigned wins over signed As a career C programmer, this drives me crazy. I kind of get it from a object storage size perspective, but the side-effects are awful. But of course, it's too late to fix it. Better just burn it all down and switch to something completely different. Oh hey, Rust!

pornel 7y ago
TBH, Rust's other extreme of never converting any numeric type anywhere is also annoying, and leads to overuse of casts and/or accidental truncation. Fortunately, memory allocation in Rust is easier, and the whole `malloc(elem * size)` chore just doesn't exist.