Re: [PATCH] refs.c: add a function to append a reflog entry to a fd

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 20, 2014, 01:22 UTC
Message-ID: <xmqqzjbm4qe5.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1416444142-28042-1-git-send-email-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

> Compared to the last send of this patch[1], there was one change in the print
> function. Replaced sprintf by snprintf for security reasons.

Careful. I despise people who blindly think strlcpy() and snprintf() are good solutions for for security. They are by themselves not.

Surely, the use of them lets you avoid stomping on pieces of memory that you did not intend to write to. I'd call that "protecting other people's data".

But what about your own data? If you are thinking that you are trying to write "this and that", but by mistake (either by your or by a careless future change) if you did not allocate enough, you would stop at "this and th". You may have protected other people's data, but your result is still broken. If you gave that to others without checking that you produced an incomplete piece of data, what guarantee are you getting that you are not creating a security hole there (imagine you intended to copy "rm -fr ./tmpdir" and by mistake you stopped at "rm -fr ./" @~@).

Show 9 quoted lines

> +	msglen = msg ? strlen(msg) : 0;
> +	maxlen = strlen(committer) + msglen + 100;
> +	logrec = xmalloc(maxlen);
> +	len = snprintf(logrec, maxlen, "%s %s %s\n",
> +		       sha1_to_hex(old_sha1),
> +		       sha1_to_hex(new_sha1),
> +		       committer);
> +	if (msglen)
> +		len += copy_msg(logrec + len - 1, msg) - 1;

In this codepath, you are allocating enough buffer to hold the whole message; there is no difference between sprintf() and snprintf(). If the difference mattered, you would have chopped the reflog entry too short, and produced a wrong result, but you then discard the whole record (the code that follows the above), losing data.

So use of snprintf() is not really buying you much here, not in the current code certainly, but not as a future-proofing measure, either.