4 ms·
Mixing signed and unsigned arithmetic operands. Not doing trivial overflow checking before arithmetic. Using results from said arithmetic as indices or starti
by clarry 13y ago
Mixing signed and unsigned arithmetic operands.
Not doing trivial overflow checking before arithmetic.
Using results from said arithmetic as indices or starting points for memory writes.
My gut says that any application using this library to process untrusted input is exploitable.
Of course, it was never advertised as being secure :-)
Which is unusual, as many "better strings" libraries make claims about the security of the traditional way of string handling (and then go on to do it wrong).
Edit: To give a concrete example, someone here recommended BString just a few days ago. That library has a "security statement" in which it claims to prevent overflow type attacks by using signed integers instead of size_t, and then checking whether the result is negative after arithmetic. But signed overflow is undefined behavior in C, and these are not guaranteed to wrap around. And yes such things have been exploited. I don't know how likely or easy bstring is to "own" but in any case it's doinnit wrong.
And yes I checked the code, it does what it claims to do..
- adobriyan 13y ago> prevent overflow type attacks by using signed integers instead of size_t, and then checking whether the result is negative after arithmetic Whoa! As if checking for overflow with unsigned is impossible...
- antirez 13y agoHello clarry, I don't think there are APIs that if not grossly misused will lead to security issues in general. There is one issue I'm aware of that perhaps is not easily exploitable but surely is unexpected behavior, that is, overflow when you reach a 2GB string: that requires a check in sdsMakeRoomFor() for sure. Note that this could be fixed by using uint64_t type for example in the header, however in the original incarnation of SDS inside Redis this was not possible for memory usage concerns. In the standalone library I believe it is instead a great idea, since 2GB is no longer an acceptable limit. From the point of view of the security the most concerning function looks like sdsrange(), there are sanity checks in place to avoid that indexes received from the outside can lead to issues, but I'll do a security review in the future in order to formally check for issues.
- eliteraspberrie 13y agoIf it's any reassurance, I had a look at it, and although there is unnecessary signed/unsigned arithmetic, I could not find anything exploitable. Change the types of variables representing an object size, length, array index, and so on to size_t (from int) and it will be fine.
- asveikau 13y agoMight be acceptable for an individual project where you can make statements about the sizes that will be used ahead of time, but for a general purpose library your statement is absolutely false. The issue is that your size requirement can overflow size_t, malloc gets passed a smaller value than you really intended, then if you try to use what you think you allocated (remember, it succeeded with a smaller size), it's a derefence outside the bounds of the allocation. Changing the size or type of the length variable won't solve that. This is a common and well known vulnerability pattern - it is disappointing to see folks so dismissive of it.
- eliteraspberrie 13y agoI'm aware of arithmetic overflow, thanks. It was the subject of my research (which will eventually be open source, watch www.peripetylabs.com/publications/ if you're interested). See my sizeop library in the meantime: https://bitbucket.org/eliteraspberries/sizeop https://bitbucket.org/eliteraspberries/sizeop I am not dismissive of the problem. I just know from experience that the chances of code like that being fixed at all, let alone correctly, is near zero.
- asveikau 13y agoI think when doing malloc(n * m) or similar the most cautious thing to do would be to check for overflow even if you don't think it's exploitable. Especially for a library. Witness for example that OpenBSD's calloc does an overflow check. I often leave this out of my own code, but not without feeling somewhat guilty about it.
- clarry 13y ago
- helmut_hed 13y agoI just started working at a place where size_t is being cast to "int" all over the code, with similar reasoning. Thanks for the ammunition.
- rurounijones 13y ago> My gut says that any application using this library to process untrusted input is exploitable Well, it is extracted from Redis so that should be easy to test