8 ms·

Heap memory corruption in GitHub's Markdown table parsing extension

83 points by hyfen 5y ago

pjmlp 5y ago
Another integer overflow bites the dust.
tines 5y ago
I am a C++ fanatic---template metaprogramming is a beautiful thing---but I've come to believe that software that handles untrusted user input should never be written in C or C++. It's too difficult to write correct software by hand, memory safe languages are really the only way.
- endorphine 5y ago
  Is this a vulnerability that would be impossible kn6, let's say, Rust?
  - roblabla 5y ago
    Yes. Heap Memory Corruption is a type of memory safety issue that's impossible in Safe Rust. (As usual, this depends on any unsafe code and the compiler being bug-free, but that's supposed to be much easier to prove since the "scope" of things to check for correctness is much reduced).
  - steveklabnik 5y ago
    This seems to be the patch: https://github.com/github/cmark-gfm/commit/cf7577d2f74289cb83de0a652afc1a8b08a37036 https://github.com/github/cmark-gfm/commit/cf7577d2f74289cb8... Integer overflow can happen in Rust, but it's well-defined, not undefined. This helps. Bounds checking is part of indexing, and so even if an index overflows, the check should happen, and panic. "impossible" is a strong word, but it would be significantly less likely in Rust. If you did the same thing as you did in C, with unsafe, then it could happen. But there's not a lot of reason to 99.9999% of the time, as it's the more difficult and less ergonomic option.
    - phillmv 5y ago
      the actual commit fix has some comments that may be useful for understanding: https://github.com/github/cmark-gfm/commit/ac80f7b56522ffa158e1f0c14a611ffccacd4027 https://github.com/github/cmark-gfm/commit/ac80f7b56522ffa15...
      - steveklabnik 5y ago
        I’m in my phone now but they cut two different patches to two different releases, I suspect I linked to one and you the other. Harder to double check that when I’m not at a computer, though that does have far better comments and I should have linked to it, thank you. I basically picked one at random.
        phillmv 5y ago
        no worries, it's the same patch to two different releases; you linked to the merge commit & I linked to the fix commit. source: i cut the releases ;)
        steveklabnik 5y ago
        Ah ha! I should have realized. Thank you for your hard work, this kind of thing is never easy.
    - schemescape 5y ago
      Is this unsigned integer overflow? Isn’t that well defined in C++ as well? Edit: I didn’t research where the corruption comes from in this bug. Edit again: it looks like the source file is actually C and not C++.
      - tines 5y ago
        Yep, well-defined in C++, but the resulting out-of-bounds accesses and all that are not well-defined.
  - pornel 5y ago
    Rust programs don't call `malloc` directly, so the problem of overflow in malloc size calculation is mitigated by never needing to write such code (Rust programs use something like Vec, which is a safe abstraction that reliably (re)allocates as much as required.) Rust's lack of implicit numeric conversions pushes authors towards using usize (size_t) for everything. So in Rust you'd be more likely to have a denial of service due to supporting 2^64 columns. If you tried to carelessly use u16 for the number of columns, you'd more likely have an application level bug like incorrect page rendering, or in the worst case a panic (equivalent of an uncaught C++ exception, which may be a program-stopping bug, but not a vulnerability).
  - olliej 5y ago
    Unexpected overflow faults in most modern safe languages (rust, swift, presumably go?) by default - they generally use different operators or functions for when overflow is ok.
- bob1029 5y ago
  Does there actually exist any practical way to ensure user input does not cause mischief when authoring C/C++ programs at scale? Are memory-safe languages the only answer?
  - pjmlp 5y ago
    Yes, the security standards like MISRA and AUTOSAR basically castrate C and C++ into subsets similar to those languages.
    - tialaramex 5y ago
      Somebody call the Lockpicking Lawyer to shove a paperclip in these "security standards". They're flimsy attempts to excuse still doing something that's a bad idea (programming safety critical software in respectively C and C++) by promising to try harder to achieve the impossible standards needed by humans programming these languages. And I do mean flimsy. Here's a fun example from a random copy of the AUTOSAR guidelines I found online labelled 17-03. AUTOSAR says if I have two 8-bit signed integers and I add them, that might overflow which is bad. So, what if I simply check that they're both less than 100, no more overflow? "Correct" says the AUTOSAR guide this is apparently OK. Huh. Signed 8-bit integer. 99 + 99 = -58. This is probably not what the person who purchased your car thought the answer was, I hope whatever accident you just caused isn't fatal.
      - pjmlp 5y ago
        I agree, but our opinion has zero value for whom calls the shots on such industries.
  - tialaramex 5y ago
    Much worse than that, even memory-safe languages like (safe) Rust, and the inevitable suggestion of AUTOSAR and so on aren't the answer. To properly answer your demand for a "practical way to ensure user input does not cause mischief" you want a drastically less capable language which cannot even in principle express the programs that should not exist, that's exactly what WUFFS is for. https://github.com/google/wuffs https://github.com/google/wuffs This sort of bug can't happen in WUFFS because you can't express the idea "corrupt the heap memory" even if you desperately wanted to. The tell-tale sign of such languages is that they are not general purpose languages, because those are able to express a wide variety of stupid things you don't want to do.
esprehn 5y ago
This seems like a good opportunity to use wasm on the server to sandbox the processing of user provided content. Of course they could also try rewriting in a safer language, but given that this already exists and handles all their content, wasm might be a simple defense in depth protection.
- pjmlp 5y ago
  WASM doesn't protect against heap corruption, because bounds checking doesn't apply inside a linear memory segment.
  - olliej 5y ago
    I think the point was that you can’t corrupt the containing process, and wasm separates code from data (Harvard arch?) so you don’t get arbitrary code exec. Of course if you process output of the wasm in a trusted environment the compromised wasm could generate something that compromises the host, but the same applies to using separate processes and IPC
    - pjmlp 5y ago
      You don't need to compromise the host, or trigger RCE, that is the fallacy of WebAssembly security sales pitch. It suffices to find a way to corrupt it's internal state and via this attack vector influence its behaviour. Which yes, boils down to common attacks to separate processes and IPC.
      - esprehn 5y ago
        Do you have a POC of such an attack? If true that would mean web browsers would be vulnerable executing wasm because you can intentionally feed it a program with out of bounds access.
        pjmlp 5y ago
        Compile heartbleed into WebAssembly. Many that talk about how great the security sandbox is never looked into the security section of the standard. https://webassembly.org/docs/security/ https://webassembly.org/docs/security/ See memory safety and mitigations.
HNHatesUsers 5y ago

83 points by hyfen 5y ago

Another integer overflow bites the dust.

I am a C++ fanatic---template metaprogramming is a beautiful thing---but I've come to believe that software that handles untrusted user input should never be written in C or C++. It's too difficult to write correct software by hand, memory safe languages are really the only way.

endorphine 5y ago
Is this a vulnerability that would be impossible kn6, let's say, Rust?
- roblabla 5y ago
  Yes. Heap Memory Corruption is a type of memory safety issue that's impossible in Safe Rust. (As usual, this depends on any unsafe code and the compiler being bug-free, but that's supposed to be much easier to prove since the "scope" of things to check for correctness is much reduced).
- steveklabnik 5y ago
  This seems to be the patch: https://github.com/github/cmark-gfm/commit/cf7577d2f74289cb83de0a652afc1a8b08a37036 https://github.com/github/cmark-gfm/commit/cf7577d2f74289cb8... Integer overflow can happen in Rust, but it's well-defined, not undefined. This helps. Bounds checking is part of indexing, and so even if an index overflows, the check should happen, and panic. "impossible" is a strong word, but it would be significantly less likely in Rust. If you did the same thing as you did in C, with unsafe, then it could happen. But there's not a lot of reason to 99.9999% of the time, as it's the more difficult and less ergonomic option.
  - phillmv 5y ago
    the actual commit fix has some comments that may be useful for understanding: https://github.com/github/cmark-gfm/commit/ac80f7b56522ffa158e1f0c14a611ffccacd4027 https://github.com/github/cmark-gfm/commit/ac80f7b56522ffa15...
    - steveklabnik 5y ago
      I’m in my phone now but they cut two different patches to two different releases, I suspect I linked to one and you the other. Harder to double check that when I’m not at a computer, though that does have far better comments and I should have linked to it, thank you. I basically picked one at random.
      - phillmv 5y ago
        no worries, it's the same patch to two different releases; you linked to the merge commit & I linked to the fix commit. source: i cut the releases ;)
        steveklabnik 5y ago
        Ah ha! I should have realized. Thank you for your hard work, this kind of thing is never easy.
  - schemescape 5y ago
    Is this unsigned integer overflow? Isn’t that well defined in C++ as well? Edit: I didn’t research where the corruption comes from in this bug. Edit again: it looks like the source file is actually C and not C++.
    - tines 5y ago
      Yep, well-defined in C++, but the resulting out-of-bounds accesses and all that are not well-defined.
- pornel 5y ago
  Rust programs don't call `malloc` directly, so the problem of overflow in malloc size calculation is mitigated by never needing to write such code (Rust programs use something like Vec, which is a safe abstraction that reliably (re)allocates as much as required.) Rust's lack of implicit numeric conversions pushes authors towards using usize (size_t) for everything. So in Rust you'd be more likely to have a denial of service due to supporting 2^64 columns. If you tried to carelessly use u16 for the number of columns, you'd more likely have an application level bug like incorrect page rendering, or in the worst case a panic (equivalent of an uncaught C++ exception, which may be a program-stopping bug, but not a vulnerability).
- olliej 5y ago
  Unexpected overflow faults in most modern safe languages (rust, swift, presumably go?) by default - they generally use different operators or functions for when overflow is ok.
bob1029 5y ago
Does there actually exist any practical way to ensure user input does not cause mischief when authoring C/C++ programs at scale? Are memory-safe languages the only answer?
- pjmlp 5y ago
  Yes, the security standards like MISRA and AUTOSAR basically castrate C and C++ into subsets similar to those languages.
  - tialaramex 5y ago
    Somebody call the Lockpicking Lawyer to shove a paperclip in these "security standards". They're flimsy attempts to excuse still doing something that's a bad idea (programming safety critical software in respectively C and C++) by promising to try harder to achieve the impossible standards needed by humans programming these languages. And I do mean flimsy. Here's a fun example from a random copy of the AUTOSAR guidelines I found online labelled 17-03. AUTOSAR says if I have two 8-bit signed integers and I add them, that might overflow which is bad. So, what if I simply check that they're both less than 100, no more overflow? "Correct" says the AUTOSAR guide this is apparently OK. Huh. Signed 8-bit integer. 99 + 99 = -58. This is probably not what the person who purchased your car thought the answer was, I hope whatever accident you just caused isn't fatal.
    - pjmlp 5y ago
      I agree, but our opinion has zero value for whom calls the shots on such industries.
- tialaramex 5y ago
  Much worse than that, even memory-safe languages like (safe) Rust, and the inevitable suggestion of AUTOSAR and so on aren't the answer. To properly answer your demand for a "practical way to ensure user input does not cause mischief" you want a drastically less capable language which cannot even in principle express the programs that should not exist, that's exactly what WUFFS is for. https://github.com/google/wuffs https://github.com/google/wuffs This sort of bug can't happen in WUFFS because you can't express the idea "corrupt the heap memory" even if you desperately wanted to. The tell-tale sign of such languages is that they are not general purpose languages, because those are able to express a wide variety of stupid things you don't want to do.

esprehn 5y ago

This seems like a good opportunity to use wasm on the server to sandbox the processing of user provided content. Of course they could also try rewriting in a safer language, but given that this already exists and handles all their content, wasm might be a simple defense in depth protection.

pjmlp 5y ago
WASM doesn't protect against heap corruption, because bounds checking doesn't apply inside a linear memory segment.
- olliej 5y ago
  I think the point was that you can’t corrupt the containing process, and wasm separates code from data (Harvard arch?) so you don’t get arbitrary code exec. Of course if you process output of the wasm in a trusted environment the compromised wasm could generate something that compromises the host, but the same applies to using separate processes and IPC
  - pjmlp 5y ago
    You don't need to compromise the host, or trigger RCE, that is the fallacy of WebAssembly security sales pitch. It suffices to find a way to corrupt it's internal state and via this attack vector influence its behaviour. Which yes, boils down to common attacks to separate processes and IPC.
    - esprehn 5y ago
      Do you have a POC of such an attack? If true that would mean web browsers would be vulnerable executing wasm because you can intentionally feed it a program with out of bounds access.
      - pjmlp 5y ago
        Compile heartbleed into WebAssembly. Many that talk about how great the security sandbox is never looked into the security section of the standard. https://webassembly.org/docs/security/ https://webassembly.org/docs/security/ See memory safety and mitigations.

HNHatesUsers 5y ago