6 ms·

RabbitMQ integer overflow that leads to heap memory corruption

tonyg 7y ago
Note this is for one of the client libraries - the C library, to be specific - not the server (written predominantly in Erlang, a memory-safe language).
- DyslexicAtheist 7y ago
  afaict, the issue is in amqp_handle_input()[1]. The fix was > "add additional input validation to prevent integer overflow when parsing a frame header" Wouldn't this be a bug in the MQ and not in the C library? [1] https://github.com/alanxz/rabbitmq-c/commit/fc85be7123050b91b054e45b91c78d3241a5047a https://github.com/alanxz/rabbitmq-c/commit/fc85be7123050b91... EDIT: sorry I just realized this is an internal rabbit MQ (C) library, I misunderstood the reference to "C library" thinking it meant the systems g/libc
  - tonyg 7y ago
    Nope. It's the C library's problem: who is to say which AMQP implementation is at the other end of the socket?
    - DyslexicAtheist 7y ago
      thanks for clarifying, my bad (I misread the meaning of C library)
- tonyg 7y ago
  (Furthermore, checking the git blame, it turns out this bug is my fault, nearly a decade ago. Decoding a size_t out of the packet and then adding a small constant (7) to it turns out to be able to overflow size_t ... sigh. Programming in C should be against some kind of Geneva Convention.)
  - stinos 7y ago
    So it's a typical 'oh a size_t is big enough I'm never going to have numbers that big' bug? Not that I blame you, I've written enough of those..
    - tonyg 7y ago
      More of a "oh yeah, that's right, C doesn't have integers, just machine words, and it doesn't help you avoid errors when you use machine words to simulate integers" bug. Overflow? You get to keep both pieces.
  - asveikau 7y ago
    > Programming in C should be against some kind of Geneva Convention Possible and likely in languages other than C. It's just going to hit an out of bounds error when you index the buffer, instead of potentially overwriting an unrelated allocation.
    - tonyg 7y ago
      More that it'd either properly promote from a 64-bit size to a big integer size, or trap on overflow. But either way, yes, a trap is kind of the point. And there are definitely other languages almost equally as toxic as C, yeah.
      - asveikau 7y ago
        I think in this beating a dead horse you fail to see some issues here. If you are going to allocate memory based on the size of a network packet's 32 bit quantity, in any language, that means your connection peer can make you allocate a contiguous 4GB. You already have a bug there. Many languages that readers here consider saviors to big bad C do not make integer overflow fault. It's not as simple a matter as you suggest because there are real tradeoffs. There are also algorithms that rely on wrapping around that are working as intended. Making it fault all the time also means that trap, if not handled, is a bug that may tear down your process, just as that runtime out of bounds exception represents a serious bug too. So another thing to consider. I just tire of hearing that switching your language will make it so you never write any bugs again. A crashing bug in C often would be a crashing bug in another language. The difference is that memory safety gives you better failure modes that don't lead to serious problems like remote code execution. You can make that argument reasonably without pretending the new language will make you infallible and the old language is responsible for all evil in the world.
hinkley 7y ago
I figured this was some counter wrapping around but this is a size field, for what looks like data? Who is transporting 2GB files over RabbitMQ? I did not think that was a thing reasonable people would do.

Note this is for one of the client libraries - the C library, to be specific - not the server (written predominantly in Erlang, a memory-safe language).

DyslexicAtheist 7y ago
afaict, the issue is in amqp_handle_input()[1]. The fix was > "add additional input validation to prevent integer overflow when parsing a frame header" Wouldn't this be a bug in the MQ and not in the C library? [1] https://github.com/alanxz/rabbitmq-c/commit/fc85be7123050b91b054e45b91c78d3241a5047a https://github.com/alanxz/rabbitmq-c/commit/fc85be7123050b91... EDIT: sorry I just realized this is an internal rabbit MQ (C) library, I misunderstood the reference to "C library" thinking it meant the systems g/libc
- tonyg 7y ago
  Nope. It's the C library's problem: who is to say which AMQP implementation is at the other end of the socket?
  - DyslexicAtheist 7y ago
    thanks for clarifying, my bad (I misread the meaning of C library)
tonyg 7y ago
(Furthermore, checking the git blame, it turns out this bug is my fault, nearly a decade ago. Decoding a size_t out of the packet and then adding a small constant (7) to it turns out to be able to overflow size_t ... sigh. Programming in C should be against some kind of Geneva Convention.)
- stinos 7y ago
  So it's a typical 'oh a size_t is big enough I'm never going to have numbers that big' bug? Not that I blame you, I've written enough of those..
  - tonyg 7y ago
    More of a "oh yeah, that's right, C doesn't have integers, just machine words, and it doesn't help you avoid errors when you use machine words to simulate integers" bug. Overflow? You get to keep both pieces.
- asveikau 7y ago
  > Programming in C should be against some kind of Geneva Convention Possible and likely in languages other than C. It's just going to hit an out of bounds error when you index the buffer, instead of potentially overwriting an unrelated allocation.
  - tonyg 7y ago
    More that it'd either properly promote from a 64-bit size to a big integer size, or trap on overflow. But either way, yes, a trap is kind of the point. And there are definitely other languages almost equally as toxic as C, yeah.
    - asveikau 7y ago
      I think in this beating a dead horse you fail to see some issues here. If you are going to allocate memory based on the size of a network packet's 32 bit quantity, in any language, that means your connection peer can make you allocate a contiguous 4GB. You already have a bug there. Many languages that readers here consider saviors to big bad C do not make integer overflow fault. It's not as simple a matter as you suggest because there are real tradeoffs. There are also algorithms that rely on wrapping around that are working as intended. Making it fault all the time also means that trap, if not handled, is a bug that may tear down your process, just as that runtime out of bounds exception represents a serious bug too. So another thing to consider. I just tire of hearing that switching your language will make it so you never write any bugs again. A crashing bug in C often would be a crashing bug in another language. The difference is that memory safety gives you better failure modes that don't lead to serious problems like remote code execution. You can make that argument reasonably without pretending the new language will make you infallible and the old language is responsible for all evil in the world.

hinkley 7y ago

I figured this was some counter wrapping around but this is a size field, for what looks like data? Who is transporting 2GB files over RabbitMQ? I did not think that was a thing reasonable people would do.