Talk:Buffer overflow/GA1

GA Review

Article (edit | visual edit | history) · Article talk (edit | history) · Watch

Reviewer: Maury Markowitz (talk · contribs) 21:34, 20 October 2016 (UTC)[reply]

Ok, here we go...

"Buffer overflows can be triggered by inputs that are designed to execute code" - this appears to be mixing two separate things. One is that buffer overflows can trigger code execution, and the other is that you may design a program to do that deliberately. Putting them both together in a single statement seems confusing.
"Bounds checking can prevent buffer overflows" - indeed, but it can also significantly effect performance. AFAIK this is the reason C++ did not add it, and I think that is important to mention even in the lede.
I really like the example section! My only concern here is the end, which seems WAY too inside baseball for this article?
"by overwriting a local variable that is near the buffer in memory on the stack to change the behavior of the program" - this does not parse... near...in...on...to change... Perhaps this can be rewritten?
"With a method called "trampolining"" - I would recommend re-writing this statement, it is very confusing to me in its current form. Particularity, "if the address of the user-supplied data is unknown, but the location is stored in a register" I can't seem to understand. Perhaps this can be (greatly) simplified?
"Stack-based buffer overflows are not to be confused with stack overflows." - why is this statement here?
"Also note that these vulnerabilities" - no! A fuzzer is worth mentioning, but that should definitely be in the Protective countermeasures, not just sort of floating about here.
"Barriers to exploitation" - this seems out of place too. It would seem this is a protective countermeasure too.
"NOP-sled" - is it NOP-sled or NOP slide? Suggest picking the later unless there's a good reason not to.

More later, gotta put the kid to bed. Maury Markowitz (talk) 23:56, 20 October 2016 (UTC)[reply]

Thanks for reviewing this! I nominated it because I couldn't find any significant flaws, so it's really great to get another pair of eyes on it. To your comments:

I've updated the lede to address your first two concerns. The topic of buffer overflow exploitability prevention is complex (possibly one of the most complex things we have), but I think of all the things to mention in the lede that is it. Of course there are other reasons C++ doesn't add intrinsic bounds checking, but x86 now has extensions for it... anyway:
I like that the example is concrete. I think if I didn't know what a buffer overflow was at all or why it was a problem, that example would do a really good job of helping me understand. I think we can probably assume someone interested in this topic has enough C to understand it? I'm not sure what else could be used to mediate the explanation - certainly it could be written in Pascal, but I think people are less likely to understand it then.
I've made some minor changes for clarity to the stack-based examples.
Reference the trampoline, yes, when I think about it, a lot of stuff is going on there. Usually exploitation now involves an improbable number of steps and many interacting components, but I've simplified it as best I can.
Yes, you're right about fuzzing! I moved it to a new section in the mitigations part.

The remainder needs more work than I can do right this moment; more to follow. FalconK (talk) 11:16, 22 October 2016 (UTC)[reply]

I hope you don't mind, but I made edits to the lede to further separate the concept of a buffer overflow from the concept that those can be exploited. It also defines a buffer, which seems useful - ultimately it would be great if one can read just the lede and get a feel for the entire topic, and I think this expansion helps in that regard. Please check for any technical errors or potentially confusing use of terms!

I have also mentioned ASLR in the lede, because that strikes me as one of the most common modern attempts to attack the problem, and it seems like that should be up front.

More to follow, off to work! Maury Markowitz (talk) 13:37, 24 October 2016 (UTC)[reply]

Apologies - I've been a bit busy this week with work things, but I'll be able to take this back up again once it passes. FalconK (talk) 09:41, 27 October 2016 (UTC)[reply]

Status query

FalconK, Maury Markowitz, what is the status of this review? There doesn't appear to have been any work done on the article since late October. Thanks. BlueMoonset (talk) 18:51, 19 December 2016 (UTC)[reply]

I was waiting to hear back from FK, but now I have the sinking feeling he's waiting on me? Maury Markowitz (talk) 15:41, 20 December 2016 (UTC)[reply]

Nope, waiting for some work things to be over, actually. I might have a little time to allocate for the article this week. The comments above are valid and we should address them. FalconK (talk) 23:14, 20 December 2016 (UTC)[reply]

FalconK, Maury Markowitz, it's been another four weeks, and FalconK's last edit to the article was back in October. The review will have been open for three months as of this Friday, January 20. Perhaps, if nothing has been done by then, that would be an appropriate time to close this. FalconK can always renominate at another, more convenient time. BlueMoonset (talk) 22:47, 16 January 2017 (UTC)[reply]

I'm up to my ears in CFP responses and consulting work, so it might come to that unless I magically have spare time in the next few days. That said, the feedback here remains valid. FalconK (talk) 09:49, 17 January 2017 (UTC)[reply]

Maury Markowitz, this review is now 100 days old. I think it's time to close this. Thanks. BlueMoonset (talk) 03:33, 28 January 2017 (UTC)[reply]

Yes it looks that way, oh well. Maury Markowitz (talk) 18:50, 28 January 2017 (UTC)[reply]