From: Junio C Hamano <hidden> Date: 2016-06-15 23:03:00
Junio C Hamano [off-list ref] writes:
Stefan Beller [off-list ref] writes:
quoted
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.
...
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.
Don't get me wrong. I am not saying that using snprintf() here is
bad per-se. There should be no difference.
But I do not want to see people getting in the habit of thinking "I
now use snprintf/strlcpy instead of sprintf/strcpy, and made my code
more secure." Often they are not doing that.
The only case snprintf/strlcpy is useful is when your data does not
matter in its detail. E.g. when you are preparing human-readble
data whose first part is sufficient to convey the information you
want to convey, you would be perfectly happy if the result is
truncated. In such a case, counting to allocate enough to hold
everything and running sprintf() only to chop the result later is
not necessary --- it still is not wrong, though --- and allocating
enough to satisify the eventual chop length and using snprintf()
is easier way to achieve the same result.
But I do not think this codepath falls into such an "I am willing to
lose data" case.
From: Stefan Beller <hidden> Date: 2016-06-15 23:03:00
Break out the code to create the string and writing it to the file
descriptor from log_ref_write and add it into a dedicated function
log_ref_write_fd. For now this is only used from log_ref_write,
but later on we will call this function from reflog transactions too,
which means that we will end up with only a single place,
where we write a reflog entry to a file instead of the current two
places (log_ref_write and builtin/reflog.c).
Signed-off-by: Ronnie Sahlberg <redacted>
Signed-off-by: Stefan Beller <redacted>
---
refs.c | 48 ++++++++++++++++++++++++++++++------------------
1 file changed, 30 insertions(+), 18 deletions(-)
Sorry for the small change w.r.t. sprintf/snprintf
Here comes a resend without changing code, but just making it a new function,
so we come forwards with the patch.
Let's discuss the change and decide if I shouldsend a follow up patch to change
it into snprintf.
On Wed, Nov 19, 2014 at 5:22 PM, Junio C Hamano [off-list ref] wrote:
quoted
+ 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.
Hypothetically speaking:
There should be no difference between sprintf() and snprintf(). As soon as you
would have a difference, you'd risk stomping on other peoples data, which could lead
to code insertion, iff the format of the inserted data is long enough overwriting
parts of your stack? Or you could change data to alter the program flow to direct
the program to another easier abusable bug.
Does this make sense? This is about protecting a user from a malicious attacker.
imagine you intended to copy "rm -fr ./tmpdir" and by mistake
you stopped at "rm -fr ./" @~@).
Yes, that's when you should check the return code of snprintf and compare to
strlen("rm -fr ./tmpdir"). That kind of bug also has severe consequences, but
it's not yet involving a malicious attacker, so I'd file this as a safety instead
of a security flaw.
Safety related bugs are easier to find usually as you don't need to have to
think about what weird things an attacker might do. This kind of bug can be found
directly from reading the code and its surroundings
("there it should print 'rm -fr ./tmpdir'... Gah! I did not check the return value")
From: Jonathan Nieder <hidden> Date: 2016-06-15 23:03:00
Stefan Beller wrote:
[Subject: [PATCH] refs.c: add a function to append a reflog entry to a fd]
Does this supersede the other patch with the same subject?
Please keep adding v<num> in the subject --- when it's there, it makes
reading much easier.
Missing 'From:' line naming the original patch author.
Break out the code to create the string and writing it to the file
descriptor from log_ref_write and add it into a dedicated function
log_ref_write_fd.
(grammar) I'm having trouble parsing the above.
Is the idea something like "Split out a function that [explanation
of when a caller would use this function goes here]?"
For now this is only used from log_ref_write,
but later on we will call this function from reflog transactions too,
Useful to know. I'd end the sentence here, since it seems to run
on with a different thought.
which means that we will end up with only a single place,
where we write a reflog entry to a file instead of the current two
places (log_ref_write and builtin/reflog.c).
Ah, so builtin/reflog.c is doing something similar and will later
be changed to use this code, too?
Overall it sounds like a very good change.
[...]
Here comes a resend without changing code, but just making it a new function,
so we come forwards with the patch.
\o/
Let's discuss the change and decide if I shouldsend a follow up patch to change
it into snprintf.
Both sprintf and snprintf are error-prone functions. It would be
lovely in a followup to use strbuf_addf or xstrfmt in this code path.
strbufs are how git deals with bookkeeping for string sizes --- they
are very pleasant.
[...]
+ result = log_ref_write_fd(logfd, old_sha1, new_sha1,
+ git_committer_info(0), msg);
+ if (result) {
int save_errno = errno;
close(logfd);
error("Unable to append to %s", log_file);
Since 'result' isn't used here, this could be simplified to
if (log_ref_write_fd(...)) {
...
}
Thanks and hope that helps,
Jonathan
From: Stefan Beller <hidden> Date: 2016-06-15 23:03:00
From: Ronnie Sahlberg <redacted>
Move code to create the string for a ref and write it to a file descriptor
from log_ref_write and add it into a new dedicated function
log_ref_write_fd.
For now the new function is only used from log_ref_write, but later
on we will call this function from reflog transactions too. That means
that we will end up with only a single place, where we write a
reflog entry to a file instead of the current two places
(log_ref_write and builtin/reflog.c).
Signed-off-by: Ronnie Sahlberg <redacted>
Signed-off-by: Stefan Beller <redacted>
---
Changes in version 3:
* reword the commit message to make it more understandable.
* no changes in code
* wait for the follow up to address any changes in the code.
refs.c | 48 ++++++++++++++++++++++++++++++------------------
1 file changed, 30 insertions(+), 18 deletions(-)
From: Jonathan Nieder <hidden> Date: 2016-06-15 23:03:00
Stefan Beller wrote:
From: Ronnie Sahlberg <redacted>
Move code to create the string for a ref and write it to a file descriptor
from log_ref_write and add it into a new dedicated function
log_ref_write_fd.
For now the new function is only used from log_ref_write, but later
on we will call this function from reflog transactions too. That means
that we will end up with only a single place, where we write a
reflog entry to a file instead of the current two places
(log_ref_write and builtin/reflog.c).
Line-wrapping width is still inconsistent. I don't think it's worth
resending just for that, but something to look out for in the future.
Signed-off-by: Ronnie Sahlberg <redacted>
Signed-off-by: Stefan Beller <redacted>
+ result = log_ref_write_fd(logfd, old_sha1, new_sha1,
+ git_committer_info(0), msg);
+ if (result) {
int save_errno = errno;
I don't understand why the above writes to a temporary variable and
checks it, never to read that temporary again.
I don't think that alone is a reason to block the patch, but it
worries me in that the review comment seems to have been just lost.
From: Stefan Beller <hidden> Date: 2016-06-15 23:03:00
On Thu, Nov 20, 2014 at 1:20 PM, Jonathan Nieder [off-list ref] wrote:
quoted
For now the new function is only used from log_ref_write, but later
on we will call this function from reflog transactions too. That means
that we will end up with only a single place, where we write a
reflog entry to a file instead of the current two places
(log_ref_write and builtin/reflog.c).
Line-wrapping width is still inconsistent. I don't think it's worth
resending just for that, but something to look out for in the future.
ok, I'll care about that more in the future.
I don't understand why the above writes to a temporary variable and
checks it, never to read that temporary again.
I don't think that alone is a reason to block the patch, but it
worries me in that the review comment seems to have been just lost.
It wasn't lost as I think it should go in a follow up patch. Sorry for
not stating that clearly.
(This patch is about moving code around, not changing code)
I got interrupted preparing the follow up patch, which gets rid of the temporary
variable.
From: Jonathan Nieder <hidden> Date: 2016-06-15 23:03:00
Stefan Beller wrote:
On Thu, Nov 20, 2014 at 1:20 PM, Jonathan Nieder [off-list ref] wrote:
quoted
I don't understand why the above writes to a temporary variable and
checks it, never to read that temporary again.
I don't think that alone is a reason to block the patch, but it
worries me in that the review comment seems to have been just lost.
It wasn't lost as I think it should go in a follow up patch. Sorry for
not stating that clearly.
(This patch is about moving code around, not changing code)
Ah, sorry for the lack of clarity.
I agree completely with the above principle. But the code that writes
to result and checks result is new code, not part of the code that
moved.