cmd: add sc_string_append

[zyga]

This patch adds a security-paranoid version of strcat that is more
sensitive to buffer overflows and other misuse of common string
utilities.

Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com

[chipaca]

LGTM!

[jdstrand]

I think this function is misnamed since the arg list doesn't match stpcpy() (note that sc_must_snprintf() matches snprintf()). With something named 'sc_must_stpcpy' I would expect it to be 'sc_must_stpcpy(char *dest, const char *src)`. Of course, it cannot be that cause you need to know the buf_size to make sure everything is ok.

[zyga]

Yeah, I wanted to keep the spirit of stpcpy which is fast and easy way to append while adding the safety.

[jdstrand]

What is this meant to protect against? The point of the function is to make sure that 'src + dst + \0' fits in buf_size. Why do we care how big buf_size is?

[zyga]

Because we don't need big buffers and our protections break down (note that overflow detection is not implemented to guard against integer overflow). Limiting the maximum buffer size to something small (and entirely reasonable for where we want to use this) adds some security.

[jdstrand]

I provided an example for sc_must_strncat() (#2658 (comment)) that both has the same arguments as strncat() and is implemented more simply and you don't have to worry about the stpcpy semantics. Why did you opt for this more complicated implementation? If you insist on stpcpy semantics, you could start with the sc_must_strncat() (renaming it to something appropriate) and just 'return &dst[strlen(dst)];', but strncat() seems the more obviously useful choice for consumers of this function.

[zyga]

I'm not sure if you meant this but strncat is not protecting anything in the output buffer. strncat just limits the amount of data read from the source buffer.

The stpcpy semantics is nice because it allows for fast and simple appending. I wanted to keep that while making it safe.

Edit: to clarify my point. With strncat you need to keep track of used space and limit n all the time. This makes it error prone and, I think, defeats the purpose for security-sensitive code. It is good for appending a substring but not for appending while not overflowing.

[jdstrand]

Meh, in the original comment in the other PR I had called this sc_append_string(char *dst, char *src, size_t dst_size) since it safely considers the dst size when appending src to dst, then at the last minute changed it without thinking critically about that (sorry for the confusion). I'll now refer to it as sc_string_append(char *dst, char *src, size_t dst_size).

I still maintain that sc_append_string(char *dst, const char *src, size_t dst_size) is a simpler implementation and easy to use (just keep appending src and dst always points to the whole string) whereas stpcpy is more specialized and focuses on the implementation detail of how the string is built up.

Re strncat() safety> strncat() can be used safely if you are checking the the sizes correctly but a must_strncat() can't be written without changing the function's signature for the reasons outlined.

[zyga]

I agree about how stpcpy is an optimization detail for how append should work. If you want I can rename this to sc_append_string and just use that everywhere.

[zyga]

OR: sc_must_append_string() or similar.

[jdstrand]

If you change to use exactly what I recommended, feel free to commit after making the changes, otherwise please have Tyler or Seth review in my absence.

[jdstrand]

Can you rename these as:

size_t sc_string_append(char *dst, size_t dst_size, const char *str)

This will make things more readable IMHO and lets the caller know the new size of dst (previously buf).

[zyga]

Done

[jdstrand]

Above here is fine.

[zyga]

I feel so silly not being able to write a correct strcat after so many tries. Thank you for guiding me.

[jdstrand]

Please rewrite the portion of the function after my comment 'Above here is fine' (I started to do inline comments but it will be easier this way) as:

	size_t dst_len = strnlen(dst, dst_size);
	if (dst_len == dst_size) {
		die("cannot append string: dst is unterminated");
	}

	size_t max_str_len = dst_size - dst_len;
	size_t str_len = strnlen(str, max_str_len);
	if (str_len == max_str_len) {
		die("cannot append string: str is too long or unterminated");
	}

	// Append the string
	memcpy(dst + dst_len, str, str_len);

	// Ensure we are terminated
	dst[dst_len + str_len] = '\0';

	// return the new size
	return strlen(dst);

[zyga]

Curious, what use is the new size? If this wasn't a function that dies on error I'd understand but honestly I cannot see us using the return value.

[jdstrand]

It isn't for safety, it is for convenience so the caller doesn't have to calculate it (this is a library function after all).

[zyga]

Done. I never thought about using strnlen and in retrospective it is much better and safer to do it this way.

[jdstrand]

Please use: size_t sc_string_append(char *dst, size_t dst_size, const char *str);

[zyga]

Done