, Matasano 2007-05-06
Halvars reaction to Microsofts 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 wed be safe.
Get it? He thinks banning memcpy is a bad idea!
Heres why Michael Howard thinks memcpy is a bad idea in secure code: it copies memory from one location to another, with an unsigned (cant 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.
Heres why Halvar thinks banning memcpy is a bad idea: if youre writing C code, memcpy is about as good as you can do; at least it has a count parameter! By banning memcmpy, youre 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. Halvars right. If youre 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 wont 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 doesnt 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.