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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Stefan Beller <sbeller@xxxxxxxxxx> 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 ./" @~@).

> +	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.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]