Re: git log -p unexpected behaviour - security risk?

Simon Ruderich [off-list ref] writes:

It probably is a better change to drop "Use `-c`..." and refer to
the "Diff formatting" section.

And then add '-p' and the fact that by default it will not show
pairwise diff for merge commits to the "Diff Formatting" section.
That is where -c/--cc/-m are already described.

On 21 April 2013 08:26, Junio C Hamano [off-list ref] wrote:

Simon Ruderich [off-list ref] writes:

quoted

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 104579d..cd35ec7 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt

@@ -24,6 +24,10 @@ ifndef::git-format-patch[]
 --patch::
      Generate patch (see section on generating patches).
      {git-diff? This is the default.}
+ifdef::git-log[]
+     Changes introduced in merge commits are not displayed. Use `-c`,
+     `--cc` or `-m` to include them.
+endif::git-log[]

It probably is a better change to drop "Use `-c`..." and refer to
the "Diff formatting" section.

And then add '-p' and the fact that by default it will not show
pairwise diff for merge commits to the "Diff Formatting" section.
That is where -c/--cc/-m are already described.

Why not have it in both places?  This is really important.

I'm concerned that noone is taking this security risk seriously.  Just
because it doesn't show up in certain workflows doesn't make the risk
go away.

What about all the people who use git internally?  They aren't using
github and almost certainly aren't using a mail based system.

It's bad that we can't even set the right behaviour as a default.

John

John Tapsell wrote:

I'm concerned that noone is taking this security risk seriously.

If anyone relies on "git log -p" or "git log -p --cc" output to make
sure that the untrusted code they use doesn't introduce unwanted
behavior, they are making a serious mistake.  A merge can completely
undo important changes made in a side branch and "-c" and "--cc" will
not show it.  The lack of "-c" cannot be a security issue here,
because in normal life adding "-c" isn't a secure deployment strategy.

That's why if you want to review the code you are pulling in as a
whole, it is worthwhile to do

	git diff HEAD...FETCH_HEAD

That is how you ask "What code changes does FETCH_HEAD introduce?"
before putting your stamp of approval on them by merging and pushing
out the result.  Unfortunately that doesn't protect you from
maliciously written commits that will be encountered when bisecting.
At some point you have to be able to trust people.

Hope that helps,
Jonathan

On 21 April 2013 11:21, Jonathan Nieder [off-list ref] wrote:

John Tapsell wrote:

quoted

I'm concerned that noone is taking this security risk seriously.

If anyone relies on "git log -p" or "git log -p --cc" output to make
sure that the untrusted code they use doesn't introduce unwanted
behavior, they are making a serious mistake.

Which is exactly my problem.

Go and ask the average person using git this very question, and I bet
you the vast majority will not know about -cc etc.

You can't just push all the blame on the user for bad defaults.
Hiding code changes is a bad default.

A merge can completely
undo important changes made in a side branch and "-c" and "--cc" will
not show it.

Wait, what?  This is getting even worse then!  Can you expand on this please?

And then how do I show all of these important changes with a git log -p ?
Or is it impossible to get a sane output?

 The lack of "-c" cannot be a security issue here,
because in normal life adding "-c" isn't a secure deployment strategy.

So, is it impossible to make git log -p a "secure deployment strategy" ?

That's why if you want to review the code you are pulling in as a
whole, it is worthwhile to do

        git diff HEAD...FETCH_HEAD

Which basically means that you're asking the review the same code
twice.  Once that way, and once using git log -p (to check for the
exact reason that you said).

 Unfortunately that doesn't protect you from
maliciously written commits that will be encountered when bisecting.
At some point you have to be able to trust people.

Seriously?  Your reasoning for awful defaults is that you should just
trust people?

This is getting worse and worse!

John

John Tapsell wrote:

Jonathan Nieder wrote:

quoted

If anyone relies on "git log -p" or "git log -p --cc" output to make
sure that the untrusted code they use doesn't introduce unwanted
behavior, they are making a serious mistake.

[...]

You can't just push all the blame on the user for bad defaults.

The thing is, I'm not convinced this is a bad default.  "Shows no diff
at all for merges" is easy for a person to understand.  It is much
easier to understand its limitations than -c and --cc.  For that
reason, it is a much *better* default for security than --cc or -c
(even though I believe one of the latter would be a better default for
convenience).

I agree that this is an important documentation bug, since
introductory documentation does not explain clearly enough how
"git log -p" will act for merges.

quoted

A merge can completely
undo important changes made in a side branch and "-c" and "--cc" will
not show it.

Wait, what?  This is getting even worse then!  Can you expand on this please?

If a given file matches one of its parents, there is nothing to show
in the combined diff format.  Otherwise every merge would have a very
long diff.

If what you really want is the diff against the first parent, you
can use -m --first-parent with -p.  If you want the diffs against each
parent, you can use -m -p.

[...]

quoted

 Unfortunately that doesn't protect you from
maliciously written commits that will be encountered when bisecting.
At some point you have to be able to trust people.

Seriously?  Your reasoning for awful defaults is that you should just
trust people?

I didn't set the defaults.  I'm explaining how the tool currently
behaves in response to your question.  A person can do many
unfortunate things if you blindly trust them and merge from them.

For example, whenever git adds (or plans) support for a new header
line in commit objects, before you've upgraded, a prankster can
provide a bad value for that header line in objects they hand-craft.
"git fsck" in your older version of git will accept the resulting
objects on the assumption that they came from a newer version of git,
so you won't notice.  Later you upgrade Git and "git fsck" considers
the objects malformed.  Clients with "[transfer] fsckobjects" enabled
start to reject your history.  That is, this person has made your
repository corrupt in the eyes of "git fsck".

The usual excellent integrity checking will let you pinpoint the
problem to the merge from that untrusted person so you can avoid
trusting them again, and all the data will be there to recover without
them.  So it is auditable later.  But this does mean that with the
current design, there is some level of trust required to let someone
commit into your history unless you inspect their work with a
fine-toothed comb.

All that said, if someone has ideas for improving git's support for
such inspection, that would be great.  "-c" just isn't it.  "-c" can
be a good tool for finding honest mistakes, but it doesn't protect
well against an adversary.

In the meantime, if you didn't intend to trust those people this much,
this might mean your procedures (and git's documentation, for the sake
of others in the same boat) need some changes.  Sorry to be the bearer
of bad news.

Hope that helps,
Jonathan

Jonathan Nieder [off-list ref] writes:

The thing is, I'm not convinced this is a bad default.  "Shows no diff
at all for merges" is easy for a person to understand.  It is much
easier to understand its limitations than -c and --cc.

Making "log -p -m" a default before -c/--cc was introduced would
have been the stupidest thing to do, as it would make the command
mostly useless.  Nobody would want to see repetitious output from a
merge that he would eventually get when the traversal drills down to
individual commits on the merged side branch.

When I added -c/--cc, I contemplated making -p imply --cc, but
decided against it primarily because it is a change in traditional
behaviour, and it is easy for users to say --cc instead of -p from
the command line.

On the other hand, "show" was a newer command and it was easy to
turn its default to --cc without having to worry too much about
existing users.

For that
reason, it is a much *better* default for security than --cc or -c
(even though I believe one of the latter would be a better default for
convenience).

Yes.  I do not fundamentally oppose to the idea of "log -p" to imply
"log --cc" when "-m" is not given ("log -p -m" is specifically
declining the combined diff simplification).  It may be a usability
improvement.

But "--cc/-c" does not have anything to do with Tapsell's "security
worries".  The only real audit he can do is with "log -m -p",
possibly with --first-parent (only if he trusts his first-parent
history).

The "recreate mechanical merge and compare recorded merge against
it" mode may highlight a malicious merger, but it will not show a
cleanly merged hunk of malicious code in the merge, so it cannot be
used with --first-parent when used as a "security audit tool".
Tapsell still needs to drill down to the merged side branch that
introduced the malicious change that merged cleanly with "-p".

On Sun, Apr 21, 2013 at 2:42 PM, Junio C Hamano [off-list ref] wrote:

Jonathan Nieder [off-list ref] writes:

quoted

The thing is, I'm not convinced this is a bad default.  "Shows no diff
at all for merges" is easy for a person to understand.  It is much
easier to understand its limitations than -c and --cc.

Making "log -p -m" a default before -c/--cc was introduced would
have been the stupidest thing to do, as it would make the command
mostly useless.  Nobody would want to see repetitious output from a
merge that he would eventually get when the traversal drills down to
individual commits on the merged side branch.

When I added -c/--cc, I contemplated making -p imply --cc, but
decided against it primarily because it is a change in traditional
behaviour, and it is easy for users to say --cc instead of -p from
the command line.

FWIW, security aside, I would've like to have seen that.  I find it
confusing that merge commits that introduce code don't have a diff
shown when using -p.  And I find it hard to remember --cc.  BTW,
what's the mnemonic for it?  -p => patch, --cc => ?

On the other hand, "show" was a newer command and it was easy to
turn its default to --cc without having to worry too much about
existing users.

quoted

For that
reason, it is a much *better* default for security than --cc or -c
(even though I believe one of the latter would be a better default for
convenience).

Yes.  I do not fundamentally oppose to the idea of "log -p" to imply
"log --cc" when "-m" is not given ("log -p -m" is specifically
declining the combined diff simplification).  It may be a usability
improvement.

Would you consider such a patch?

-John

Sorta OT, but I'm curious,

On Sun, Apr 21, 2013 at 12:09 PM, Jonathan Nieder [off-list ref] wrote:

For example, whenever git adds (or plans) support for a new header
line in commit objects, before you've upgraded, a prankster can
provide a bad value for that header line in objects they hand-craft.
"git fsck" in your older version of git will accept the resulting
objects on the assumption that they came from a newer version of git,
so you won't notice.  Later you upgrade Git and "git fsck" considers
the objects malformed.  Clients with "[transfer] fsckobjects" enabled
start to reject your history.  That is, this person has made your
repository corrupt in the eyes of "git fsck".

The usual excellent integrity checking will let you pinpoint the
problem to the merge from that untrusted person so you can avoid
trusting them again, and all the data will be there to recover without
them.  So it is auditable later.  But this does mean that with the
current design, there is some level of trust required to let someone
commit into your history unless you inspect their work with a
fine-toothed comb.

Has anyone written a test case for this?