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

[Jonathan Nieder <jrnieder@gmail.com>]

John Tapsell wrote:
> Jonathan Nieder wrote:

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

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

[...]
>>  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