Re: git log -p unexpected behaviour - security risk?
On Sun, Apr 21, 2013 at 2:42 PM, Junio C Hamano gits...@pobox.com wrote: Jonathan Nieder jrnie...@gmail.com 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. 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. 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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git log -p unexpected behaviour - security risk?
Sorta OT, but I'm curious, On Sun, Apr 21, 2013 at 12:09 PM, Jonathan Nieder jrnie...@gmail.com 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? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git log -p unexpected behaviour - security risk?
John Szakmeister j...@szakmeister.net writes: 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 = ? Compact combined. By the way, these options are _not_ about showing merge commits that introduce code, and they do not help your kind of security. As I repeatedly said, you would need -p -m for that. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git log -p unexpected behaviour - security risk?
On Tue, Apr 30, 2013 at 12:37 PM, Junio C Hamano gits...@pobox.com wrote: John Szakmeister j...@szakmeister.net writes: 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 = ? Compact combined. Thank you. By the way, these options are _not_ about showing merge commits that introduce code, and they do not help your kind of security. As I repeatedly said, you would need -p -m for that. I'm sorry, I didn't mean to imply that it's useful for security, just that it better meets my expectations when -p is turned on. I realize there are some edges in the logic, but I'm fine with those edges. -John -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git log -p unexpected behaviour - security risk?
Junio C Hamano gits...@pobox.com writes: By the way, these options are _not_ about showing merge commits that introduce code, and they do not help your kind of security. As I repeatedly said, you would need -p -m for that. Actually, while defaulting to --cc may be convenient, it would indeed increase the security risk: currently, git log -p shows nothing for merges, so it's rather clear that _everything_ is omitted. With --cc, the user would see a diff, and could hardly guess that not everything is shown without reading the doc very carefully. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git log -p unexpected behaviour - security risk?
On Tue, Apr 30, 2013 at 1:05 PM, Matthieu Moy matthieu@grenoble-inp.fr wrote: Junio C Hamano gits...@pobox.com writes: By the way, these options are _not_ about showing merge commits that introduce code, and they do not help your kind of security. As I repeatedly said, you would need -p -m for that. Actually, while defaulting to --cc may be convenient, it would indeed increase the security risk: currently, git log -p shows nothing for merges, so it's rather clear that _everything_ is omitted. With --cc, the user would see a diff, and could hardly guess that not everything is shown without reading the doc very carefully. I don't believe it's that clear. I bet people assume there's nothing to show, and unless you dig in and discover that `-p` doesn't include merges. In git 1.8.2, `git help log` doesn't seem to make any mention of `-p` not showing a diff for merges. Just to see, I asked several people around here whether they knew `-p` didn't show diffs for merges, and they were all surprised that diffs were being omitted for merge commits. -John -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git log -p unexpected behaviour - security risk?
On 30 April 2013 18:58, John Szakmeister j...@szakmeister.net wrote: On Tue, Apr 30, 2013 at 1:05 PM, Matthieu Moy matthieu@grenoble-inp.fr wrote: Junio C Hamano gits...@pobox.com writes: By the way, these options are _not_ about showing merge commits that introduce code, and they do not help your kind of security. As I repeatedly said, you would need -p -m for that. Actually, while defaulting to --cc may be convenient, it would indeed increase the security risk: currently, git log -p shows nothing for merges, so it's rather clear that _everything_ is omitted. With --cc, the user would see a diff, and could hardly guess that not everything is shown without reading the doc very carefully. I don't believe it's that clear. I bet people assume there's nothing to show, and unless you dig in and discover that `-p` doesn't include merges. In git 1.8.2, `git help log` doesn't seem to make any mention of `-p` not showing a diff for merges. Just to see, I asked several people around here whether they knew `-p` didn't show diffs for merges, and they were all surprised that diffs were being omitted for merge commits. Is there no way to fix --cc to work even in the edge cases? John -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git log -p unexpected behaviour - security risk?
Simon Ruderich si...@ruderich.org writes:
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.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git log -p unexpected behaviour - security risk?
On 21 April 2013 08:26, Junio C Hamano gits...@pobox.com wrote:
Simon Ruderich si...@ruderich.org writes:
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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git log -p unexpected behaviour - security risk?
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git log -p unexpected behaviour - security risk?
On 21 April 2013 11:21, Jonathan Nieder jrnie...@gmail.com wrote: 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. 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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git log -p unexpected behaviour - security risk?
Jonathan Nieder jrnie...@gmail.com writes: 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. And the only way to retroactively review that a merge C did not do anything funly is to check git diff C^1 C, assuming that you already trust C^1, the state before you performed the merge. Incidentally, this works for non-merge commits just as well. git log -m -p is not the default because most of the time people are not interested in seeing what it shows over --cc or -c. It is a repetition of what you would get from individual patches on the side branch merged that you will later see in the traversal of that command. --cc/-c gives a representation for tricky merge cases where people could likely have made a mistake, or had correctly resolved semantic conflicts (e.g. one side renames a function, the other side adds a callsite, the merge result renames the function new caller calls). For the purpose of a merge audit John seems to want, the only way is to wade through log -m -p output. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git log -p unexpected behaviour - security risk?
Jonathan Nieder jrnie...@gmail.com 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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git log -p unexpected behaviour - security risk?
On Thu, Apr 11, 2013 at 11:36:26AM +0100, John Tapsell wrote:
Is there a way to make --cc default?
If you use aliases, something like this is easy:
git config --global --add alias.lp 'log --patch --cc'
I use aliases heavily, so that's my fix for now.
But I think the current behaviour is unexpected for most (new?)
users (including me). I thought -p would display all changes in
all commits, including merges.
I guess changing -p's default behaviour to imply --cc is
problematic, so I think we should document that -p doesn't
generate patches for merges. Maybe something like this:
-- 8 --
Subject: [PATCH] Documentation/diff-options.txt: -p doesn't display merge
changes
Signed-off-by: Simon Ruderich si...@ruderich.org
---
Documentation/diff-options.txt | 4
1 file changed, 4 insertions(+)
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[]
endif::git-format-patch[]
-Un::
--
1.8.2.1.513.gdedbb69.dirty
-- 8 --
Regards
Simon
--
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
git log -p unexpected behaviour - security risk?
Hi, I noticed that code that you put in merge will not be visible by default. This seems like a pretty horrible security problem, no? I made the following test tree, with just 3 commits: https://github.com/johnflux/ExampleEvilness.git Doing git log -p shows all very innocent commits. Completely hidden is the change to add EVIL CODE MUWHAHAHA. This seems really dangerous! The evil code only shows up with the non-default --cc or -m option. Is there a way to make --cc default? John -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git log -p unexpected behaviour - security risk?
On Thu, Apr 11, 2013 at 6:36 PM, John Tapsell johnf...@gmail.com wrote: I noticed that code that you put in merge will not be visible by default. This seems like a pretty horrible security problem, no? I made the following test tree, with just 3 commits: https://github.com/johnflux/ExampleEvilness.git Doing git log -p shows all very innocent commits. Completely hidden is the change to add EVIL CODE MUWHAHAHA. This seems really dangerous! The evil code only shows up with the non-default --cc or -m option. For email-based patch workflows (eg. git, linux kernel), then this is not a problem - the diff doesn't even show up, so nothing is applied when git-am is run. For github with pull-requests, a diff is shown between trees, so this will show up. -- Cheers, Ray Chuan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
