Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Heap memory corruption in GitHub's Markdown table parsing extension

github.com

Post reply on HN
Print view View original

11–20 of 44 posts

Re: Heap memory corruption in GitHub's Markdown table parsing extension

#11

Post by schemescape »

steveklabnik wrote: post #6
endorphine wrote: post #4

Earlier quoted context omitted.

Is this a vulnerability that would be impossible kn6, let's say, Rust?

This seems to be the patch: 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 uns…

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

Re: Heap memory corruption in GitHub's Markdown table parsing extension

#12

Post by tines »

schemescape wrote: post #11
steveklabnik wrote: post #6

Earlier quoted context omitted.

This seems to be the patch: 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 uns…

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

Yep, well-defined in C++, but the resulting out-of-bounds accesses and all that are not well-defined.

Re: Heap memory corruption in GitHub's Markdown table parsing extension

#13

Post by schemescape »

tines wrote: post #12
schemescape wrote: post #11

Earlier quoted context omitted.

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

Yep, well-defined in C++, but the resulting out-of-bounds accesses and all that are not well-defined.

Thanks. I should look at the code. I thought unsigned int overflow would wrap to zero, which would still be in bounds for a nontrivial array. Maybe they’re freeing the item at that index the first time through the array or something.

Re: Heap memory corruption in GitHub's Markdown table parsing extension

#14

Post by pjmlp »

esprehn wrote: post #5

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.

WASM doesn't protect against heap corruption, because bounds checking doesn't apply inside a linear memory segment.

Re: Heap memory corruption in GitHub's Markdown table parsing extension

#15

Post by staticassertion »

esprehn wrote: post #5

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.

What Dropbox did for this sort of thing is ideal. You spawn a child process that has two file handles piped to/from the parent - stdin, stdout.

That child process does the scary stuff - parsing. Parsing requires zero system calls. Reading to/from the parent requires only read and write, but not open, so they can only read and write to those file descriptors.

And exit.

That's it. Seccomp v1 is trivial to apply, gives 4 system calls, and makes the process virtually useless to an attacker. If you want to get fancy and allow for multithreading you can use seccomp v2 and create your threadpool before you drop privs, and probably add futex and memmap.

You pay a latency cost but the security win is huge.

Re: Heap memory corruption in GitHub's Markdown table parsing extension

#16

Post by staticassertion »

steveklabnik wrote: post #6
endorphine wrote: post #4

Earlier quoted context omitted.

Is this a vulnerability that would be impossible kn6, let's say, Rust?

This seems to be the patch: 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 uns…

Well defined and it also panics in debug mode. Unit tests tend not to catch these sorts of bugs tbh, but still, nice to have :)

Re: Heap memory corruption in GitHub's Markdown table parsing extension

#17

Post by steveklabnik »

phillmv wrote: post #9
steveklabnik wrote: post #6

Earlier quoted context omitted.

This seems to be the patch: 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 uns…

the actual commit fix has some comments that may be useful for understanding: https://github.com/github/cmark-gfm/commit/ac80f7b56522ffa15...

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.

Re: Heap memory corruption in GitHub's Markdown table parsing extension

#18

Post by phillmv »

steveklabnik wrote: post #17
phillmv wrote: post #9

Earlier quoted context omitted.

the actual commit fix has some comments that may be useful for understanding: https://github.com/github/cmark-gfm/commit/ac80f7b56522ffa15...

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.

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 ;)

Re: Heap memory corruption in GitHub's Markdown table parsing extension

#19

Post by steveklabnik »

phillmv wrote: post #18
steveklabnik wrote: post #17

Earlier quoted context omitted.

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.

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 ;)

Ah ha! I should have realized. Thank you for your hard work, this kind of thing is never easy.

Re: Heap memory corruption in GitHub's Markdown table parsing extension

#20

Post by tialaramex »

bob1029 wrote: post #7
tines wrote: post #3

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.

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?

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

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.

Post reply on HN