Hacker Newsnew | past | comments | ask | show | jobs | submitlogin

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?


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.


> 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.




Guidelines | FAQ | Lists | API | Security | Legal | Apply to YC | Contact

Search: