* git log -p unexpected behaviour - security risk? @ 2013-04-11 10:36 John Tapsell 2013-04-11 15:19 ` Tay Ray Chuan 2013-04-20 14:00 ` Simon Ruderich 0 siblings, 2 replies; 22+ messages in thread From: John Tapsell @ 2013-04-11 10:36 UTC (permalink / raw) To: Git List 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: git log -p unexpected behaviour - security risk? 2013-04-11 10:36 git log -p unexpected behaviour - security risk? John Tapsell @ 2013-04-11 15:19 ` Tay Ray Chuan 2013-04-20 14:00 ` Simon Ruderich 1 sibling, 0 replies; 22+ messages in thread From: Tay Ray Chuan @ 2013-04-11 15:19 UTC (permalink / raw) To: John Tapsell; +Cc: Git List On Thu, Apr 11, 2013 at 6:36 PM, John Tapsell <johnflux@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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: git log -p unexpected behaviour - security risk? 2013-04-11 10:36 git log -p unexpected behaviour - security risk? John Tapsell 2013-04-11 15:19 ` Tay Ray Chuan @ 2013-04-20 14:00 ` Simon Ruderich 2013-04-21 7:26 ` Junio C Hamano 1 sibling, 1 reply; 22+ messages in thread From: Simon Ruderich @ 2013-04-20 14:00 UTC (permalink / raw) To: John Tapsell; +Cc: Git List, Tay Ray Chuan, Junio C Hamano 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 <simon@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[] -U<n>:: -- 1.8.2.1.513.gdedbb69.dirty -- 8< -- Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: git log -p unexpected behaviour - security risk? 2013-04-20 14:00 ` Simon Ruderich @ 2013-04-21 7:26 ` Junio C Hamano 2013-04-21 8:56 ` John Tapsell 0 siblings, 1 reply; 22+ messages in thread From: Junio C Hamano @ 2013-04-21 7:26 UTC (permalink / raw) To: Simon Ruderich; +Cc: John Tapsell, Git List, Tay Ray Chuan Simon Ruderich <simon@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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: git log -p unexpected behaviour - security risk? 2013-04-21 7:26 ` Junio C Hamano @ 2013-04-21 8:56 ` John Tapsell 2013-04-21 10:21 ` Jonathan Nieder 0 siblings, 1 reply; 22+ messages in thread From: John Tapsell @ 2013-04-21 8:56 UTC (permalink / raw) To: Junio C Hamano; +Cc: Simon Ruderich, Git List, Tay Ray Chuan On 21 April 2013 08:26, Junio C Hamano <gitster@pobox.com> wrote: > Simon Ruderich <simon@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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: git log -p unexpected behaviour - security risk? 2013-04-21 8:56 ` John Tapsell @ 2013-04-21 10:21 ` Jonathan Nieder 2013-04-21 13:46 ` John Tapsell 2013-04-21 18:25 ` Junio C Hamano 0 siblings, 2 replies; 22+ messages in thread From: Jonathan Nieder @ 2013-04-21 10:21 UTC (permalink / raw) To: John Tapsell; +Cc: Junio C Hamano, Simon Ruderich, Git List, Tay Ray Chuan 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: git log -p unexpected behaviour - security risk? 2013-04-21 10:21 ` Jonathan Nieder @ 2013-04-21 13:46 ` John Tapsell 2013-04-21 15:56 ` Thomas Rast 2013-04-21 16:09 ` Jonathan Nieder 2013-04-21 18:25 ` Junio C Hamano 1 sibling, 2 replies; 22+ messages in thread From: John Tapsell @ 2013-04-21 13:46 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Junio C Hamano, Simon Ruderich, Git List, Tay Ray Chuan On 21 April 2013 11:21, Jonathan Nieder <jrnieder@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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: git log -p unexpected behaviour - security risk? 2013-04-21 13:46 ` John Tapsell @ 2013-04-21 15:56 ` Thomas Rast 2013-04-21 16:09 ` Jonathan Nieder 1 sibling, 0 replies; 22+ messages in thread From: Thomas Rast @ 2013-04-21 15:56 UTC (permalink / raw) To: John Tapsell Cc: Jonathan Nieder, Junio C Hamano, Simon Ruderich, Git List, Tay Ray Chuan John Tapsell <johnflux@gmail.com> writes: > On 21 April 2013 11:21, Jonathan Nieder <jrnieder@gmail.com> wrote: > >> 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? It pretty much by definition does not show changes if the merge picks one side unchanged: -c [...] lists only files which were modified from all parents. --cc This flag implies the -c option and further compresses the patch output [...] On top of that, the default history simplification when you specify a pathspec will only walk the (or any one) unchanged side of such a merge, so if you filter for a file you wouldn't even find the offending commit further back in history. I don't think this can be improved easily with the current one-pass[1] history/diff generation. To know what the merge *should* have done, you'd need to somehow get an idea what parts of the resulting files should be affected, which AFAICS boils down to redoing the merge. And to do that, you need to scan history so far that you can compute the merge-bases. Not to mention that redoing all merges while walking history is somewhat expensive. You could hack up a script that does the verification manually, by actually running a merge and comparing the result with what the merge gave you. But it's not something that you would want to run by default. [1] some things like --simplify-merges are actually another pass, but the default is to generate everything -- including diffs -- as we go. -- Thomas Rast trast@{inf,student}.ethz.ch ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: git log -p unexpected behaviour - security risk? 2013-04-21 13:46 ` John Tapsell 2013-04-21 15:56 ` Thomas Rast @ 2013-04-21 16:09 ` Jonathan Nieder 2013-04-21 18:42 ` Junio C Hamano 2013-04-30 11:48 ` git log -p unexpected behaviour - security risk? shawn wilson 1 sibling, 2 replies; 22+ messages in thread From: Jonathan Nieder @ 2013-04-21 16:09 UTC (permalink / raw) To: John Tapsell; +Cc: Junio C Hamano, Simon Ruderich, Git List, Tay Ray Chuan 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: git log -p unexpected behaviour - security risk? 2013-04-21 16:09 ` Jonathan Nieder @ 2013-04-21 18:42 ` Junio C Hamano 2013-04-30 10:09 ` John Szakmeister 2013-04-30 11:48 ` git log -p unexpected behaviour - security risk? shawn wilson 1 sibling, 1 reply; 22+ messages in thread From: Junio C Hamano @ 2013-04-21 18:42 UTC (permalink / raw) To: Jonathan Nieder; +Cc: John Tapsell, Simon Ruderich, Git List, Tay Ray Chuan Jonathan Nieder <jrnieder@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". ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: git log -p unexpected behaviour - security risk? 2013-04-21 18:42 ` Junio C Hamano @ 2013-04-30 10:09 ` John Szakmeister 2013-04-30 16:37 ` Junio C Hamano 0 siblings, 1 reply; 22+ messages in thread From: John Szakmeister @ 2013-04-30 10:09 UTC (permalink / raw) To: Junio C Hamano Cc: Jonathan Nieder, John Tapsell, Simon Ruderich, Git List, Tay Ray Chuan On Sun, Apr 21, 2013 at 2:42 PM, Junio C Hamano <gitster@pobox.com> wrote: > Jonathan Nieder <jrnieder@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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: git log -p unexpected behaviour - security risk? 2013-04-30 10:09 ` John Szakmeister @ 2013-04-30 16:37 ` Junio C Hamano 2013-04-30 16:47 ` John Szakmeister 2013-04-30 17:05 ` Matthieu Moy 0 siblings, 2 replies; 22+ messages in thread From: Junio C Hamano @ 2013-04-30 16:37 UTC (permalink / raw) To: John Szakmeister Cc: Jonathan Nieder, John Tapsell, Simon Ruderich, Git List, Tay Ray Chuan John Szakmeister <john@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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: git log -p unexpected behaviour - security risk? 2013-04-30 16:37 ` Junio C Hamano @ 2013-04-30 16:47 ` John Szakmeister 2013-04-30 17:05 ` Matthieu Moy 1 sibling, 0 replies; 22+ messages in thread From: John Szakmeister @ 2013-04-30 16:47 UTC (permalink / raw) To: Junio C Hamano Cc: Jonathan Nieder, John Tapsell, Simon Ruderich, Git List, Tay Ray Chuan On Tue, Apr 30, 2013 at 12:37 PM, Junio C Hamano <gitster@pobox.com> wrote: > John Szakmeister <john@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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: git log -p unexpected behaviour - security risk? 2013-04-30 16:37 ` Junio C Hamano 2013-04-30 16:47 ` John Szakmeister @ 2013-04-30 17:05 ` Matthieu Moy 2013-04-30 17:58 ` John Szakmeister 1 sibling, 1 reply; 22+ messages in thread From: Matthieu Moy @ 2013-04-30 17:05 UTC (permalink / raw) To: Junio C Hamano Cc: John Szakmeister, Jonathan Nieder, John Tapsell, Simon Ruderich, Git List, Tay Ray Chuan Junio C Hamano <gitster@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/ ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: git log -p unexpected behaviour - security risk? 2013-04-30 17:05 ` Matthieu Moy @ 2013-04-30 17:58 ` John Szakmeister 2013-04-30 19:31 ` John Tapsell 0 siblings, 1 reply; 22+ messages in thread From: John Szakmeister @ 2013-04-30 17:58 UTC (permalink / raw) To: Matthieu Moy Cc: Junio C Hamano, Jonathan Nieder, John Tapsell, Simon Ruderich, Git List, Tay Ray Chuan On Tue, Apr 30, 2013 at 1:05 PM, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote: > Junio C Hamano <gitster@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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: git log -p unexpected behaviour - security risk? 2013-04-30 17:58 ` John Szakmeister @ 2013-04-30 19:31 ` John Tapsell 2013-04-30 19:44 ` git log -p unexpected behaviour Junio C Hamano 0 siblings, 1 reply; 22+ messages in thread From: John Tapsell @ 2013-04-30 19:31 UTC (permalink / raw) To: Git List On 30 April 2013 18:58, John Szakmeister <john@szakmeister.net> wrote: > On Tue, Apr 30, 2013 at 1:05 PM, Matthieu Moy > <Matthieu.Moy@grenoble-inp.fr> wrote: >> Junio C Hamano <gitster@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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: git log -p unexpected behaviour 2013-04-30 19:31 ` John Tapsell @ 2013-04-30 19:44 ` Junio C Hamano 2013-04-30 20:12 ` John Tapsell 0 siblings, 1 reply; 22+ messages in thread From: Junio C Hamano @ 2013-04-30 19:44 UTC (permalink / raw) To: John Tapsell; +Cc: Git List John Tapsell <johnflux@gmail.com> writes: > Is there no way to fix --cc to work even in the edge cases? Can you clarify what you mean by "fix" and "edge cases"? ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: git log -p unexpected behaviour 2013-04-30 19:44 ` git log -p unexpected behaviour Junio C Hamano @ 2013-04-30 20:12 ` John Tapsell 2013-04-30 20:38 ` Junio C Hamano 0 siblings, 1 reply; 22+ messages in thread From: John Tapsell @ 2013-04-30 20:12 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git List On 30 April 2013 20:44, Junio C Hamano <gitster@pobox.com> wrote: > John Tapsell <johnflux@gmail.com> writes: > >> Is there no way to fix --cc to work even in the edge cases? > > Can you clarify what you mean by "fix" and "edge cases"? My understanding is that even with -cc there will be changes that won't be seen - and hence why --cc could be even more of a "security risk", no? John ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: git log -p unexpected behaviour 2013-04-30 20:12 ` John Tapsell @ 2013-04-30 20:38 ` Junio C Hamano 2013-05-01 7:23 ` John Tapsell 0 siblings, 1 reply; 22+ messages in thread From: Junio C Hamano @ 2013-04-30 20:38 UTC (permalink / raw) To: John Tapsell; +Cc: Git List John Tapsell <johnflux@gmail.com> writes: > On 30 April 2013 20:44, Junio C Hamano <gitster@pobox.com> wrote: >> John Tapsell <johnflux@gmail.com> writes: >> >>> Is there no way to fix --cc to work even in the edge cases? >> >> Can you clarify what you mean by "fix" and "edge cases"? > > My understanding is that even with -cc there will be changes that > won't be seen - and hence why --cc could be even more of a "security > risk", no? Combined diff is a way to show a tricky conflict resolved in a tricky way, so that the tricky-ness of the resolution can be examined. A trivial resolution that takes one side is not shown because it is not usually interesting. This design choice of course have to trust people involved in the project do not pull from untrustworthy sources. You would need "log -p -m" (without any pathspec) for the kind of "security" you are talking about. Note that "-p -m --first-parent" is not necessarily enough. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: git log -p unexpected behaviour 2013-04-30 20:38 ` Junio C Hamano @ 2013-05-01 7:23 ` John Tapsell 0 siblings, 0 replies; 22+ messages in thread From: John Tapsell @ 2013-05-01 7:23 UTC (permalink / raw) To: Git List On 30 April 2013 21:38, Junio C Hamano <gitster@pobox.com> wrote: > John Tapsell <johnflux@gmail.com> writes: > >> On 30 April 2013 20:44, Junio C Hamano <gitster@pobox.com> wrote: >>> John Tapsell <johnflux@gmail.com> writes: >>> >>>> Is there no way to fix --cc to work even in the edge cases? >>> >>> Can you clarify what you mean by "fix" and "edge cases"? >> >> My understanding is that even with -cc there will be changes that >> won't be seen - and hence why --cc could be even more of a "security >> risk", no? > > Combined diff is a way to show a tricky conflict resolved in a > tricky way, so that the tricky-ness of the resolution can be > examined. A trivial resolution that takes one side is not shown > because it is not usually interesting. I don't really understand your point sorry. In this trivial resolution case, you would still just see the commit that added the code in a later commit. No? There couldn't be a case where you add or change a line of code, but not see it with --cc ? > This design choice of course > have to trust people involved in the project do not pull from > untrustworthy sources. > > You would need "log -p -m" (without any pathspec) for the kind of > "security" you are talking about. Note that "-p -m --first-parent" > is not necessarily enough. This results in seeing the same change more than once though, right? ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: git log -p unexpected behaviour - security risk? 2013-04-21 16:09 ` Jonathan Nieder 2013-04-21 18:42 ` Junio C Hamano @ 2013-04-30 11:48 ` shawn wilson 1 sibling, 0 replies; 22+ messages in thread From: shawn wilson @ 2013-04-30 11:48 UTC (permalink / raw) To: Jonathan Nieder Cc: John Tapsell, Junio C Hamano, Simon Ruderich, Git List, Tay Ray Chuan Sorta OT, but I'm curious, On Sun, Apr 21, 2013 at 12:09 PM, Jonathan Nieder <jrnieder@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? ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: git log -p unexpected behaviour - security risk? 2013-04-21 10:21 ` Jonathan Nieder 2013-04-21 13:46 ` John Tapsell @ 2013-04-21 18:25 ` Junio C Hamano 1 sibling, 0 replies; 22+ messages in thread From: Junio C Hamano @ 2013-04-21 18:25 UTC (permalink / raw) To: Jonathan Nieder; +Cc: John Tapsell, Simon Ruderich, Git List, Tay Ray Chuan Jonathan Nieder <jrnieder@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. ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2013-05-01 7:23 UTC | newest] Thread overview: 22+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-04-11 10:36 git log -p unexpected behaviour - security risk? John Tapsell 2013-04-11 15:19 ` Tay Ray Chuan 2013-04-20 14:00 ` Simon Ruderich 2013-04-21 7:26 ` Junio C Hamano 2013-04-21 8:56 ` John Tapsell 2013-04-21 10:21 ` Jonathan Nieder 2013-04-21 13:46 ` John Tapsell 2013-04-21 15:56 ` Thomas Rast 2013-04-21 16:09 ` Jonathan Nieder 2013-04-21 18:42 ` Junio C Hamano 2013-04-30 10:09 ` John Szakmeister 2013-04-30 16:37 ` Junio C Hamano 2013-04-30 16:47 ` John Szakmeister 2013-04-30 17:05 ` Matthieu Moy 2013-04-30 17:58 ` John Szakmeister 2013-04-30 19:31 ` John Tapsell 2013-04-30 19:44 ` git log -p unexpected behaviour Junio C Hamano 2013-04-30 20:12 ` John Tapsell 2013-04-30 20:38 ` Junio C Hamano 2013-05-01 7:23 ` John Tapsell 2013-04-30 11:48 ` git log -p unexpected behaviour - security risk? shawn wilson 2013-04-21 18:25 ` Junio C Hamano
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).