5 ms·

Integer Overflow into Information Disclosure (2017)

37 points by pcr910303 5y ago

CamperBob2 5y ago
There's one more potential gotcha in your example that has burned me a few times. You have a function that takes char *endptr and passes *endptr to isspace(). At least on the particular version of the MSVC compiler I usually use, char is signed by default, and isspace uses an ASCII lookup table internally, presumably 256 bytes long. So if anything passes a so-called "negative ASCII" value to your routine, memory will be accessed prior to the beginning of the table. Usually just causes a crash, but the implications could go well beyond that. Whenever I use any of the is...() functions, I've learned to cast the value to unsigned char to make sure underflow can't happen. It's caused a lot of bug reports from non-US-based users who end up with ANSI (extended or 'negative' ASCII) characters in various places. Obviously there are plenty of facepalms to go around here, ranging from the C standard's failure to disallow signed chars to certain compilers' adoption of them, to Microsoft's failure to protect their standard C library function inputs, to my negligence in not realizing the implications of the foregoing. Definitely something to watch out for in your own code. As I write this, it occurs to me that they might actually use a 128-byte LUT. So I should either stop using iswhatever() functions altogether or write my own wrapper for them....
- glandium 5y ago
  Isspace takes an int (signed) and its documentation says: The behavior is undefined if the value of ch is not representable as unsigned char and is not equal to EOF. IOW, you invoked undefined behavior. That's one of the problems with the C standard library: it expects callers to be much more careful than the language wants you to. So many footguns. It doesn't help that the implementations of the standard library assume the callers are doing their due, mostly for speed reasons that were valid 20+ years ago.
  - CamperBob2 5y ago
    Yep, that's what I meant by plenty of facepalms to go around. The entire class of functions is "unsafe at any speed." Code which seems perfectly safe on one compiler will blow up on another, literally without warning. I do wonder why they didn't have the function take an unsigned int, or at least do a simple bitwise AND with 0xFF before the table lookup.
    - radiospiel 5y ago
      Because of EOF being -1
      - Someone 5y ago
        Nitpick: it is only guaranteed to be unequal to any character and less than zero. (And no, I don’t know any implementation that has it unequal to minus one. I would love to see a devilish C compiler/standard library that would make all kind of standard-conforming, yet non-standard implementation choices (EOF not minus one, RAND_MAX a fairly small prime, etc. Goal would be to have no single major program run correctly on it)
      - CamperBob2 5y ago
        Good point, but if the standard defines EOF as a negative value while at the same time permitting implementations to support default-unsigned char types, then the sanity train obviously left the tracks at that time, or at some time prior to it.
  - tjoff 5y ago
    If isspace is a function designed to take a character. And if the default type of a character is incompatible with the argument to that function then that is arguably a bug in the specification. And, given that behavior, that the documentation fails to explicitly call it out is borderline malicious. Given that the authors even knew about it, which I guess is doubtful.
    - hvdijk 5y ago
      The documentation does call it out explicitly. The message you responded to pointed out where the documentation called it out. You're attacking the authors' intentions and competence when there is no reason for that.
      - tjoff 5y ago
        They do not explicitly call out that the systems default character is implementation specific and that the function as is might be incompatible with default behavior (on as it turns out the vast majority of systems). Just saying it needs to be able to fit an unsigned char and call it a day is hardly clear documentation when it isn't even obvious that the signness of the character type is implemention defined for most c-programers. It almost reads as a deliberate trap (of course it isn't, but imo it is not good documentation).
    - tialaramex 5y ago
      The C type system isn't really set up to deliver what you'd actually want for safety. The rational response is to not use C, but it took until relatively recently for people to really begin that migration. As another example, notice that abs(number) has Undefined Behaviour for INT_MIN or whatever C calls it.
      - tjoff 5y ago
        It is not, but then again a lot of people don't use a static type system at all. I don't consider abs() comparable. It is a direct consequence of underlying representation that you are kind of expected to know, the consequences are known and considered. Is it bad? Depends on context. isspace on the other hand, as described, just seems like bad design.
  - josefx 5y ago
    > mostly for speed reasons that were valid 20+ years ago. I think it is more likely the same issue Posix had, the official standard came after the implementations. So it had to unify all existing implementations under the same definition. Which means the standard could not require sane behavior if even a single implementation failed to validate its input or two implementations disagreed on the exact error.
    - pjmlp 5y ago
      Which is why modern POSIX support is never 100%. "POSIX has become outdated" 2016 paper, https://www.usenix.org/publications/login/fall2016/atlidakis https://www.usenix.org/publications/login/fall2016/atlidakis
c-shubh 5y ago
(2017)

37 points by pcr910303 5y ago

There's one more potential gotcha in your example that has burned me a few times. You have a function that takes char *endptr and passes *endptr to isspace(). At least on the particular version of the MSVC compiler I usually use, char is signed by default, and isspace uses an ASCII lookup table internally, presumably 256 bytes long. So if anything passes a so-called "negative ASCII" value to your routine, memory will be accessed prior to the beginning of the table. Usually just causes a crash, but the implications could go well beyond that. Whenever I use any of the is...() functions, I've learned to cast the value to unsigned char to make sure underflow can't happen. It's caused a lot of bug reports from non-US-based users who end up with ANSI (extended or 'negative' ASCII) characters in various places. Obviously there are plenty of facepalms to go around here, ranging from the C standard's failure to disallow signed chars to certain compilers' adoption of them, to Microsoft's failure to protect their standard C library function inputs, to my negligence in not realizing the implications of the foregoing. Definitely something to watch out for in your own code. As I write this, it occurs to me that they might actually use a 128-byte LUT. So I should either stop using iswhatever() functions altogether or write my own wrapper for them....

glandium 5y ago
Isspace takes an int (signed) and its documentation says: The behavior is undefined if the value of ch is not representable as unsigned char and is not equal to EOF. IOW, you invoked undefined behavior. That's one of the problems with the C standard library: it expects callers to be much more careful than the language wants you to. So many footguns. It doesn't help that the implementations of the standard library assume the callers are doing their due, mostly for speed reasons that were valid 20+ years ago.
- CamperBob2 5y ago
  Yep, that's what I meant by plenty of facepalms to go around. The entire class of functions is "unsafe at any speed." Code which seems perfectly safe on one compiler will blow up on another, literally without warning. I do wonder why they didn't have the function take an unsigned int, or at least do a simple bitwise AND with 0xFF before the table lookup.
  - radiospiel 5y ago
    Because of EOF being -1
    - Someone 5y ago
      Nitpick: it is only guaranteed to be unequal to any character and less than zero. (And no, I don’t know any implementation that has it unequal to minus one. I would love to see a devilish C compiler/standard library that would make all kind of standard-conforming, yet non-standard implementation choices (EOF not minus one, RAND_MAX a fairly small prime, etc. Goal would be to have no single major program run correctly on it)
    - CamperBob2 5y ago
      Good point, but if the standard defines EOF as a negative value while at the same time permitting implementations to support default-unsigned char types, then the sanity train obviously left the tracks at that time, or at some time prior to it.
- tjoff 5y ago
  If isspace is a function designed to take a character. And if the default type of a character is incompatible with the argument to that function then that is arguably a bug in the specification. And, given that behavior, that the documentation fails to explicitly call it out is borderline malicious. Given that the authors even knew about it, which I guess is doubtful.
  - hvdijk 5y ago
    The documentation does call it out explicitly. The message you responded to pointed out where the documentation called it out. You're attacking the authors' intentions and competence when there is no reason for that.
    - tjoff 5y ago
      They do not explicitly call out that the systems default character is implementation specific and that the function as is might be incompatible with default behavior (on as it turns out the vast majority of systems). Just saying it needs to be able to fit an unsigned char and call it a day is hardly clear documentation when it isn't even obvious that the signness of the character type is implemention defined for most c-programers. It almost reads as a deliberate trap (of course it isn't, but imo it is not good documentation).
  - tialaramex 5y ago
    The C type system isn't really set up to deliver what you'd actually want for safety. The rational response is to not use C, but it took until relatively recently for people to really begin that migration. As another example, notice that abs(number) has Undefined Behaviour for INT_MIN or whatever C calls it.
    - tjoff 5y ago
      It is not, but then again a lot of people don't use a static type system at all. I don't consider abs() comparable. It is a direct consequence of underlying representation that you are kind of expected to know, the consequences are known and considered. Is it bad? Depends on context. isspace on the other hand, as described, just seems like bad design.
- josefx 5y ago
  > mostly for speed reasons that were valid 20+ years ago. I think it is more likely the same issue Posix had, the official standard came after the implementations. So it had to unify all existing implementations under the same definition. Which means the standard could not require sane behavior if even a single implementation failed to validate its input or two implementations disagreed on the exact error.
  - pjmlp 5y ago
    Which is why modern POSIX support is never 100%. "POSIX has become outdated" 2016 paper, https://www.usenix.org/publications/login/fall2016/atlidakis https://www.usenix.org/publications/login/fall2016/atlidakis

c-shubh 5y ago

(2017)