Search: Home Bugtraq Vulnerabilities Mailing Lists Jobs Tools Beta Programs
Digg this story   Add to del.icio.us  
Halvar Flake vs. Michael Howard on memcpy, and Matasano's Solution
Thomas Ptacek, Matasano 2007-05-06

Halvar’s reaction to Microsoft’s Michael Howard hinting that memcpy may soon be verboten in Redmond code:

This is an excellent idea - and along with memcpy, malloc() should be banned. While we are at it, the addition and multiplication operators have caused so much grief over the last years, I think it would make total sense to ban them. Oh, and if we ban the memory dereference, I am quite sure we’d be safe.

Get it? He thinks banning memcpy is a bad idea!

Here’s why Michael Howard thinks memcpy is a bad idea in secure code: it copies memory from one location to another, with an unsigned (“can’t be negative”) count parameter. If you screw the count up, or use a bad offset to find the copy target, you (or your attacker) have corrupted memory.

Here’s why Halvar thinks banning memcpy is a bad idea: if you’re writing C code, memcpy is about as good as you can do; at least it has a count parameter! By banning memcmpy, you’re just making memory corruption bugs more obscure: any time you have to copy data, you need to find the target and the count of bytes and provide them to something.

I present a Third Way. Halvar’s right. If you’re working in C, without some (performance-killing) abstraction like growable ring buffers or string libraries, you need to accept that your code has hotspots that need to be carefully audited. But Howard is also right: at the very least, you could acknowledge that 99.999% of copies won’t need even 31 bytes of length and lock out a whole class of integer overflows.

I propose that Microsoft bans memcpy(), replacing each call with one of:

  • memcpy-untrusted: the default choice when data to be copied comes from a tainted source, like a file header or network packet. Asserts attempts to copy lengths with the sign bit set.

  • memcpy-untrusted32: basically “memcpy”, including the unsigned length, but at least it communicates developer intent.

  • memcpy-untrusted-fixed: for the ANI-like case where the count of bytes is known at compile time. Perhaps another variant like memcmpy-untrusted-ranged could provide a second “maxlen” argument.

  • memcpy-trusted: basically “memcpy”, but again communicates developer intend and acknowledgement that the copy is risky.

  • memcpy-trusted-fixed: as with memcpyuntrustedfixed, for when the count is known at compile time.

The theme of my proposal is, ban memcpy like Michael Howard suggests (and, in particular, remove the code ambiguity of whether an untrusted length parameter of a full 32 bits was intended), but make the call sites communicate to auditors what the intent of the code was, to facilitate audits.

Yes, _trusted and _untrusted32 pretty much differ only in name. The point is, callsites for each can be audited differently, and code standards for them (for instance, the type of checking required) can differ.

Really, memcpy just needs to take 4 arguments: the size of the target as well as the source. But even that doesn’t solve the problem; the LP64 overflow in qmail came from a looped copy. The best you can do is at least flag the spots in your code that need careful auditing.


Comments


The information, views, and opinions contained on this page are those of the author and do not necessarily reflect the views and opinions of SecurityFocus.






 

Privacy Statement
Copyright 2009, SecurityFocus