On 20/08/2015 11:27, Rainer Weikusat wrote:
> Roger Leigh <rleigh@???> writes:
>> On 19/08/2015 17:39, Rainer Weikusat wrote:
>
> [...]
>

>>> static void saveFile(char* essid, char* pw) //argv[1], argv[2]
>>> {
>>>     char *path;
>>>     FILE *fp;
>>>     unsigned p_len, e_len;

>>>

>>>     p_len = strlen(IFACES_PATH);
>>>     e_len = strlen(essid);
>>>     path = alloca(p_len + e_len + 2);

>>>     

>>>     strcpy(path, IFACES_PATH);
>>>     path[p_len] = '/';
>>>     strcpy(path + p_len + 1, essid);

>>>     

>>>     fp = fopen(path, "ab+");
>>>     fprintf(fp, IFACE_TMPL, essid, pw);
>>>     fclose(fp);
>>> }

>>>

>>> int main(int argc, char **argv)
>>> {
>>>     saveFile(argv[1], argv[2]);
>>>     return 0;
>>> }

>>
>> I'm not picking on this post in particular out of the rest of today's
>> thread, but I did think this was a good example. While I don't want
>> to act like a rabid C++ zealot, stuff like this really makes me
>> shudder due to the fragility and unnecessary complexity for something
>> which is really trivial.
>>
>> While the relative safety and security of C string handling can be
>> debated, I do think the question needs asking: Why not use a language
>> with proper safe string handling and avoid the issue entirely?
>> It's only "safe" until it's refactored to break the existing
>> assumptions and make it accidentally unsafe. The constants such as 2,
>> 1 plus the strlen() calls are prime candidates for future bugs. It's
>> not like this /needs/ to be done in C.
>
> The 'constant 2' follows from the fact that the length of the result
> string is the length of the path plus the length of the essid string
> plus two more bytes/ characters, namely, the '/' and the terminating
> zero.
>
> The 'constant 1', in turn, follows from the fact that the essid has to
> be appended after the string made up of the path and the added '/'.
>
> That's how string processing in C happens to look like because of how
> the language (library, actually) defines a string and because of the
> operation supposed to be performed here (combine three different parts,
> one with constant length).
>
> Could you perhaps elaborate on what you were writing about? Minus trying
> to start a language war, that is. The original author chose C. Because
> of this, I wrote and posted a simple example how to do string processing
> in C without relying on 'large enough' fixed size buffers.

The rationale for the use of the constants is fine. But consider that
the code does not document where those numbers come from, and fact that
code to calculate the buffer size and the code to copy data into the
buffer are separate steps. This is where problems can occur. Maybe not
right now, after all you got it right when you wrote it, one would hope.
But when it comes to future modifications, you must update all the
size calculations and constants in line with any changes to how the
buffer is filled, and this is a prime candidate for mistakes and
consequent crashes and/or buffer overflows. When it comes to making
modifications, you or whoever is making the change, needs to work out
exactly what the intent of the orginal code was--i.e. re-derive all the
constants and re-compute them correctly to match the new behaviour.
This is often non-trivial depending on the nature of the string
manipulation.

The fact that C code is a continual source of such quality and security
defects is a clear indication that in general people *don't* get this
right even when they think they are geniuses who know their own code.
This example is short, but people continue to routinely screw up even
stuff this simple, and it only becomes more likely with more complexity.

IMO, stuff like this just doesn't belong in any program that claims to
be secure. Especially in one that's setuid root doing privileged
operations.

When I wrote the schroot(1) tool, which is setuid-root out of necessity
since chroot(2) is privileged, I originally wrote it in C, but converted
it to C++ primarily due to the security considerations, of which
avoiding insecure and buggy string handling was the prime consideration.
It removes this entire class of bugs and security exploits at a
stroke. And if I .reserve() space in a string for efficiency, it's as
efficient as all the manual C manipulations, but safe from overflow, and
if I get it wrong it'll merely perform another allocation rather than
being exploitable. i.e. it's as efficient as string manipulation in C,
only vastly less complex and with safety factored in.

All too often the attitude of C programmers is "it'll be perfect and
efficient since I won't screw up". But we all screw up at some point.
And the question then is, "what are the consequences of screwing up".
And here, the consequences are severe. But with std::string, are
entirely avoidable. So we both reduce the chance of a screwup by making
the API match our intent exactly without lots of extra make-work to
allocate buffers and unsafely mangle strings, and we also mitigate for
any mistakes in calculations by the failure case being a slightly less
efficient allocation strategy rather than a root exploit.


Regards,
Roger