9 ms·

Show HN: Hiding code from Git diff

167 points by dimastopel 9y ago

pkamb 9y ago
The way I've noticed people (accidentally) "hiding code" is by making changes during the conflict resolution stage of a git merge. This often happens after code review, and there's no oversight of the conflict resolutions. If the merge commit does get looked at, there will be potentially hundreds of lines and files of automated diffs within which the unrelated new code is hidden.
- WorldMaker 9y ago
  Too many experiences like that I make merge commits a required part of a code review. If a branch needs conflict resolution in order to merge, it needs that merge in that branch, and as part of the code review of that branch. (Some of the best conflict resolution is sometimes new code in light of the changes in the other branch, and so of course should be reviewed.) If a merge commit diff is showing too much information, that's more often than not a tooling problem. Most tools provide ways to filter out the "automated stuff" and focus only on the conflict resolutions. If they don't, they are deficient (and if there are too many conflict resolutions to easily review, that may be a sign of a larger process problem). (GitHub has gotten much better at merge commit views in the last couple of years, as one anecdotal bullet point.)
  - seanwilson 9y ago
    > Too many experiences like that I make merge commits a required part of a code review. How can you do this practically though? In the turnaround time to do the code review and make the changes, a new conflict might have been introduced into the branch being targeted by the merge. I think merging e.g. the master branch into your branch before code review is a good idea though as when the review is complete there's going to be less or no conflicts merging into master.
    - WorldMaker 9y ago
      That's also a tooling issue I believe. GitHub, again, as easy example, has gotten pretty good at "these are the commits since your last review" incremental reviews, so if you complete a code review and need conflicts to be fixed before it can merge, you expect a merge commit to be made in that branch and should just need to review that merge commit. I've not recently seen an issue with a PR cycling through a lot of merge commits to keep up with the branch it is targeting, but in almost every case where I've seen that there was some other process decision in the way (two people working on the exact same feature simultaneously) and at that point it was an architectural, "political", or "social" problem to fix first, outside of the code review process. Your mileage may vary, of course.
- stinos 9y ago
  Isn't this sort of solved by rebasing first instead of just merging? (well, and some rules in place which tells people 'we only accept properly rebased branches for review). Then the conflicts are resolved in the branch and there's a clear view of what actually changed in the diff, before anything is effectively merged into the main branch. Or maybe I'm missing something here.
  - Mithaldu 9y ago
    It's absolutely resolved that way. Rebase should be the weapon of choice in all cases, except those where it would be prohibitively expensive. People are too enamored with the SVN ways though, or have even made up rationalizations about fictional concepts such as "historical purity". So you have a ton of people merging instead, producing code repositories that look worse than the worst perl code i've touched.
    - lemoncucumber 9y ago
      Conversely, people get too enamored of rebasing as a way to solve the problem of merging in lots of junky commits. There are lots of ways to rewrite history and clean it up. Rebasing is just one of them, and it's one that can force you to spend more time resolving conflicts than you otherwise might have to (since you have to replay all of your commits and resolve any conflicts caused by each one individually). There's no reason why rebasing is a better way to clean up your history than squash merging (or even -- carefully -- using git reset).
      - Mithaldu 9y ago
        I already said that it can be too expensive, but thanks for playing. <3 Also, a squash merge is not a merge. It's just another form of rebase. And if you mention reset as an alternative to cherry pick and squash you should also mention --soft.
        lemoncucumber 9y ago
        My point was that many git users don't understand git all that well, and I think that encouraging everyone to rebase is a bad idea. If you're going to promote a particular workflow as the the one that "should be the weapon of choice," I think the workflow of merge-master-into-feature-branch, squash-merge-feature-branch-into-master is a more user friendly alternative, since then the cases where rebasing is expensive never even come up. And yes, you're right that I was remiss not to mention the --soft part. The git reset workflow is one that I would definitely not encourage people to do if they don't understand git well.
- deckar01 9y ago
  GitLab makes it really easy to see what changed between commits while reviewing a merge request. Parts of the diff that are identical to the upstream branch are automatically ignored, and you can view a diff between of last two pushes. https://docs.gitlab.com/ce/user/project/merge_requests/versions.html https://docs.gitlab.com/ce/user/project/merge_requests/versi... Edit: GitHub has similar functionality, but it doesn't know how to diff force pushes, so you would end up having to review the full diff again to ensure integrity.
  - WorldMaker 9y ago
    > Edit: GitHub has similar functionality, but it doesn't know how to diff force pushes, so you would end up having to review the full diff again to ensure integrity. I realize this is a religious war I'm stepping into, but I fall on the side of "if you are doing a force push of a branch already in code review you are doing something wrong". Once commits are out in the open and in review between multiple developers, that is shared code history that encodes important information about the code review process itself. (I make obvious exceptions for things like accidental PII disclosure or similar bone-headed mistakes, but the longer a branch has been in code review, the less willing I am to allow it to be force pushed.) Of course, to each their own and your mileage may vary, but that comment about diffing force pushes in a merge/pull request sent a shiver down my spine, regardless of whether or not the tool supports it.
    - closeparen 9y ago
      Phabricator is really nice here. The history of the code review process is maintained by Phab itself, you can amend all you want against an inflight code review, and upon landing it all becomes a single commit in master with a link to the code review in the commit message.
    - __david__ 9y ago
      You’d hate our team—we force push to master (not just feature branches) all the time. It seems like anarchy at first but it’s not really that hard to deal with. Our situation is a bit unique though, and I doubt it would scale well beyond a small team.
- simmons 9y ago
  That is scary. I think it would be a nifty feature for a code review tool to show only the conflict resolution for merge commits. (Although I suppose it would need knowledge of exactly which automated merge method/options were used to create the pre-CR merge, for any given commit with multiple parents.)
- pkamb 9y ago
  What I personally do is commit the merge as is, with all the <<< === >>> conflict markers. Then, in the next couple commits, fix each conflict via a normal commit that mixes and matches "mine" and "theirs" as needed + deletes the markers. Each resolution commit is independent of the giant merge commit, and can be separately diff'ed and code reviewed via the github PR or any other tool. Also easier IMO to make the complicated resolution you want, or reset back to a previous resolution attempt without messing up the entire merge. The drawback is that some commits won't compile, but compilable mid-branch commits are not something that I view as a goal.
  - jsteemann 9y ago
    Non-compiling commits make bisecting harder, when the goal is to find the commit that broke the build or a specific test. IMHO having to skip over non-compiling merge commits only makes bisecting take more time, and it may also "pollute" the test pipelines if all commits are going to be built. Additionally, merging and fixing the merge in separate commits makes it much hard to revert a merge later, in case it turned out that the merge introduced something unwanted). So I will always recommend doing the merge and fixing the merge in the same commit. Ideally this produces a squash commit that can easily be reverted later when needed. This makes the merge process more time-consuming, but it can help to keep the builds more stable, and to more easily track which commit (merge) introduced a particular problem.
    - pkamb 9y ago
      > makes it much hard to revert a merge later I don't think this would apply, as I'd be merging from `dev` > `feature`, resolving conflicts, and then doing a single clean no-conflict merge back from `feature` > `dev`. That's the only commit you'd need to revert to undo the merge. (or on a new, third, `merge` branch, if you don't want to pollute the `feature` branch with commits from `dev`) I admit the non-compiling commits might be a problem in some workflows, but `git bisect skip` is the solution to the only problem I've ever had with it.
      - jsteemann 9y ago
- herpderperator 9y ago
  That's why I say "you have conflicts" and have them resolve the conflict before I do the review. GitHub, at least, correctly diffs against master after the conflicts are resolved so you don't see any of the merged code (if it's the same as on the master branch), only the actual changes. Doesn't that solve the problem?
amigoingtodie 9y ago
What about code being 'deleted' that git diff does not catch? Is there a recommended external diff tool that ameliorates these issues?
- hawski 9y ago
  Maybe cat -v? $ git diff | cat -v However it's considered harmful: http://harmful.cat-v.org/cat-v/ http://harmful.cat-v.org/cat-v/ ;)
  - UncleEntity 9y ago
    or git diff > tmp.diff and open with a text editor?
- jwilk 9y ago
  git diff | less -U But the you lose all the colors.
mikedilger 9y ago
If the output of a git command was raw text sent to stdout, like cat, then I would argue that escape sequences should be ignored and passed through unmolested. But the output of git tools is colored (if enabled) using terminal escape sequences. Given that, the output should fully comply with the appropriate terminal escape sequences and should filter the ones the author has discovered.
ComputerGuru 9y ago
I actually reported this same issue but under the context of a bug rather than as a security issue, exactly 2 months ago: https://public-inbox.org/git/CACcTrKctqAWeWWrc9Q+Y7ewXGc_o+uJoeHS83LDw5O_s1-3Nug@mail.gmail.com/ https://public-inbox.org/git/CACcTrKctqAWeWWrc9Q+Y7ewXGc_o+u... Same response, “it’s the pager’s job,” in that case.
- jwilk 9y ago
  I mentioned the problem on oss-security in 2016: http://openwall.com/lists/oss-security/2016/04/21/9 http://openwall.com/lists/oss-security/2016/04/21/9 (I think my claim that git escapes some control characters is incorrect; it's less that does it.) > Same response, “it’s the pager’s job,” in that case. It can't be the pagers job, because pager has no way to distingiush from escapes generated by git (to make things colorful) from malicious escapes.
CGamesPlay 9y ago
This is tenuous at best. iTerm2, at least, doesn't support the "conceal" color (^[[8m) and any other color would rely on the "victim's" color scheme to work properly. Git is safely removing sequences that could actually cause problems like OSC. This technique also wouldn't stand up to review on Github or any other online code review platform.
deathanatos 9y ago
Unix pipes lack any ability to say "this is the kind of data that I'm pushing you." The push raw, unidentified bytes. If the downstream program had some idea what format the data was in, it would know how to output it. Imagine an alternate universe where a pipe was a stream of bytes and a MIME type. If a terminal gets data written to it from a program in application/terminal.vt100, then it knows it should process those escape sequences. If it get text/plain; charset=utf-8, it knows it should not, and can take a different action. I think such a universe would make for more pleasant piping, too. Imagine that every interactive command on a terminal ended with an imaginary |show command; it's job is to read the mimetype and display, on the terminal, that data. A command that emitted CSV data could then automatically render in the terminal as an actual table. A program that emitted a PNG could render an ASCII art version of that image (or, since we're in an imaginary universe, imagine our terminal has escape sequences for images – which actually exists in some terminals today – it could emit the actual image!). Essentially, the program emits the data in whatever form it natively speaks, and that implicit |show parses that into something appropriate for the terminal. (That way, the program can still be used in pipelines, such as make_a_png | resize_it.) If you want your raw bytes, then you can imagine make_a_png | force_type 'application/octet-stream' and then the final, implicit |show would perhaps output a nice hexdump for you. (Since emitting raw bytes to a terminal never makes sense.) Now, I'm a bit fuzzy on exactly how the pipeline being executed, the shell, and the terminal all interact exactly, and I'm nearly certain such a change would require low-level, POSIX-breaking changes. But it was a dream I had, and I think it might be a better model than what we work w/ presently. (In the article's case, though, git log/diff both emit terminal sequences, so even in my imaginary world, they'd need to know that they should escape them. It mostly works for simpler types, but git rm could conceptually emit just text, since I don't think it ever colors.)
- chickenfries 9y ago
  > Imagine an alternate universe where a pipe was a stream of bytes and a MIME type. Isn't this powershell? > I'm nearly certain such a change would require low-level, POSIX-breaking changes. That seems to be the trade off, breaking with posix compatibility for nicer abstractions.
- CGamesPlay 9y ago
jsteemann 9y ago
One mitigation for this is to use `col -bp` as a git pager, but it seems clumsy. However, for completeness I post it here with an example for how this makes the backdoor visible. Reproduce instruction: # clone repo and cd into it git clone https://github.com/twistlock/gitPocDiff https://github.com/twistlock/gitPocDiff cd gitPocDiff # show diff (this includes the backdoor but it won't be visible) git show ebc3f506a7ec8278e1a3ad4108612b66d10b41ca # expose the backdoor, using `col -bp` as pager git show ebc3f506a7ec8278e1a3ad4108612b66d10b41ca | col -bp
- jwilk 9y ago
  No, "col -bp" is not bulletproof. The attacker could still use backspace characters to hide malicious content.
carapace 9y ago
(Tiny grey sans-serif body text means you hate your readers' eyes.)
ChuckMcM 9y ago
Interesting to see how the standardization of escape sequences by ANSI leads to a vulnerability in code obfuscation.
- shabble 9y ago
  we need a 'secure escape' sequence that disables all subsequent escapes until rescinded! And it can change the background colour of the affected lines to a shade unrepresentable by unprivileged sequences. Mostly tongue-in-cheek, but... one of these years I'm going to track down what the heck the 'PRIVACY MESSAGE' sequence is actually for. ECMA48 is infuriatingly vague, and nothing seems to actually use or support it.
tomcam 9y ago
Unfortunately had to stop reading due to their text-hiding scheme--miniscule light-gray text on white background
- mrob 9y ago
  Firefox's Reader View reduces the problem, and with some custom CSS, solves it completely (see https://news.ycombinator.com/item?id=15080363 https://news.ycombinator.com/item?id=15080363 ). I imagine other browsers include a similar solution.
- dimastopel 9y ago
  This will be fixed. Thanks for the feedback.
kbutler 9y ago
The issue with with terminal handling of escape sequences, rather than with git. If you are concerned, you can use a gui diff tool, or you can pipe it through a filter that removes escape codes: seal:~/work/gitPocDiff$ cat > uncolor #!/usr/bin/env perl ## uncolor — remove terminal escape sequences such as color changes while (<>) { s/ \e[ #%()*+\-.\/]. | (?:\e\[|\x9b) [ -?]* [@-~] | # CSI ... Cmd (?:\e\]|\x9d) .*? (?:\e\\|[\a\x9c]) | # OSC ... (ST|BEL) (?:\e[P^_]|[\x90\x9e\x9f]) .*? (?:\e\\|\x9c) | # (DCS|PM|APC) ... ST \e.|[\x80-\x9f] //xg; print; } seal:~/work/gitPocDiff$ chmod a+x uncolor seal:~/work/gitPocDiff$ git diff eeeb ebc3 | ./uncolor diff --git a/main.c b/main.c index 6daa9b2..0321ff2 100644 --- a/main.c +++ b/main.c @@ -3,4 +3,9 @@ int main() { printf("I'm just a stub!\n"); + /* + * Must always return a value Hidden > */printf("Insert bad backdoor here...!\n");/* + * TODO: Return result + */ + return 0; } Credit to: gilles https://unix.stackexchange.com/a/14707 https://unix.stackexchange.com/a/14707
- realusername 9y ago
  You can also just pipe it to cat -e, it works the same. git diff | cat -e make all escape sequences visible.
  - kbutler 9y ago
    There's always a utility... Thanks!
always_good 9y ago
Some pitiful Vimming right there. From holding down `h` for 30 seconds to go to the beginning of line to using :wq instead of :x.
unhammer 9y ago
magit ftw: https://i.imgur.com/a8Mvbkf.png https://i.imgur.com/a8Mvbkf.png Though I'm sure Emacs has loads of other vulnerabilities :)

167 points by dimastopel 9y ago

The way I've noticed people (accidentally) "hiding code" is by making changes during the conflict resolution stage of a git merge. This often happens after code review, and there's no oversight of the conflict resolutions. If the merge commit does get looked at, there will be potentially hundreds of lines and files of automated diffs within which the unrelated new code is hidden.

WorldMaker 9y ago
Too many experiences like that I make merge commits a required part of a code review. If a branch needs conflict resolution in order to merge, it needs that merge in that branch, and as part of the code review of that branch. (Some of the best conflict resolution is sometimes new code in light of the changes in the other branch, and so of course should be reviewed.) If a merge commit diff is showing too much information, that's more often than not a tooling problem. Most tools provide ways to filter out the "automated stuff" and focus only on the conflict resolutions. If they don't, they are deficient (and if there are too many conflict resolutions to easily review, that may be a sign of a larger process problem). (GitHub has gotten much better at merge commit views in the last couple of years, as one anecdotal bullet point.)
- seanwilson 9y ago
  > Too many experiences like that I make merge commits a required part of a code review. How can you do this practically though? In the turnaround time to do the code review and make the changes, a new conflict might have been introduced into the branch being targeted by the merge. I think merging e.g. the master branch into your branch before code review is a good idea though as when the review is complete there's going to be less or no conflicts merging into master.
  - WorldMaker 9y ago
    That's also a tooling issue I believe. GitHub, again, as easy example, has gotten pretty good at "these are the commits since your last review" incremental reviews, so if you complete a code review and need conflicts to be fixed before it can merge, you expect a merge commit to be made in that branch and should just need to review that merge commit. I've not recently seen an issue with a PR cycling through a lot of merge commits to keep up with the branch it is targeting, but in almost every case where I've seen that there was some other process decision in the way (two people working on the exact same feature simultaneously) and at that point it was an architectural, "political", or "social" problem to fix first, outside of the code review process. Your mileage may vary, of course.
stinos 9y ago
Isn't this sort of solved by rebasing first instead of just merging? (well, and some rules in place which tells people 'we only accept properly rebased branches for review). Then the conflicts are resolved in the branch and there's a clear view of what actually changed in the diff, before anything is effectively merged into the main branch. Or maybe I'm missing something here.
- Mithaldu 9y ago
  It's absolutely resolved that way. Rebase should be the weapon of choice in all cases, except those where it would be prohibitively expensive. People are too enamored with the SVN ways though, or have even made up rationalizations about fictional concepts such as "historical purity". So you have a ton of people merging instead, producing code repositories that look worse than the worst perl code i've touched.
  - lemoncucumber 9y ago
    Conversely, people get too enamored of rebasing as a way to solve the problem of merging in lots of junky commits. There are lots of ways to rewrite history and clean it up. Rebasing is just one of them, and it's one that can force you to spend more time resolving conflicts than you otherwise might have to (since you have to replay all of your commits and resolve any conflicts caused by each one individually). There's no reason why rebasing is a better way to clean up your history than squash merging (or even -- carefully -- using git reset).
    - Mithaldu 9y ago
      I already said that it can be too expensive, but thanks for playing. <3 Also, a squash merge is not a merge. It's just another form of rebase. And if you mention reset as an alternative to cherry pick and squash you should also mention --soft.
      - lemoncucumber 9y ago
        My point was that many git users don't understand git all that well, and I think that encouraging everyone to rebase is a bad idea. If you're going to promote a particular workflow as the the one that "should be the weapon of choice," I think the workflow of merge-master-into-feature-branch, squash-merge-feature-branch-into-master is a more user friendly alternative, since then the cases where rebasing is expensive never even come up. And yes, you're right that I was remiss not to mention the --soft part. The git reset workflow is one that I would definitely not encourage people to do if they don't understand git well.
deckar01 9y ago
GitLab makes it really easy to see what changed between commits while reviewing a merge request. Parts of the diff that are identical to the upstream branch are automatically ignored, and you can view a diff between of last two pushes. https://docs.gitlab.com/ce/user/project/merge_requests/versions.html https://docs.gitlab.com/ce/user/project/merge_requests/versi... Edit: GitHub has similar functionality, but it doesn't know how to diff force pushes, so you would end up having to review the full diff again to ensure integrity.
- WorldMaker 9y ago
  > Edit: GitHub has similar functionality, but it doesn't know how to diff force pushes, so you would end up having to review the full diff again to ensure integrity. I realize this is a religious war I'm stepping into, but I fall on the side of "if you are doing a force push of a branch already in code review you are doing something wrong". Once commits are out in the open and in review between multiple developers, that is shared code history that encodes important information about the code review process itself. (I make obvious exceptions for things like accidental PII disclosure or similar bone-headed mistakes, but the longer a branch has been in code review, the less willing I am to allow it to be force pushed.) Of course, to each their own and your mileage may vary, but that comment about diffing force pushes in a merge/pull request sent a shiver down my spine, regardless of whether or not the tool supports it.
  - closeparen 9y ago
    Phabricator is really nice here. The history of the code review process is maintained by Phab itself, you can amend all you want against an inflight code review, and upon landing it all becomes a single commit in master with a link to the code review in the commit message.
  - __david__ 9y ago
    You’d hate our team—we force push to master (not just feature branches) all the time. It seems like anarchy at first but it’s not really that hard to deal with. Our situation is a bit unique though, and I doubt it would scale well beyond a small team.
simmons 9y ago
That is scary. I think it would be a nifty feature for a code review tool to show only the conflict resolution for merge commits. (Although I suppose it would need knowledge of exactly which automated merge method/options were used to create the pre-CR merge, for any given commit with multiple parents.)
pkamb 9y ago
What I personally do is commit the merge as is, with all the <<< === >>> conflict markers. Then, in the next couple commits, fix each conflict via a normal commit that mixes and matches "mine" and "theirs" as needed + deletes the markers. Each resolution commit is independent of the giant merge commit, and can be separately diff'ed and code reviewed via the github PR or any other tool. Also easier IMO to make the complicated resolution you want, or reset back to a previous resolution attempt without messing up the entire merge. The drawback is that some commits won't compile, but compilable mid-branch commits are not something that I view as a goal.
- jsteemann 9y ago
  Non-compiling commits make bisecting harder, when the goal is to find the commit that broke the build or a specific test. IMHO having to skip over non-compiling merge commits only makes bisecting take more time, and it may also "pollute" the test pipelines if all commits are going to be built. Additionally, merging and fixing the merge in separate commits makes it much hard to revert a merge later, in case it turned out that the merge introduced something unwanted). So I will always recommend doing the merge and fixing the merge in the same commit. Ideally this produces a squash commit that can easily be reverted later when needed. This makes the merge process more time-consuming, but it can help to keep the builds more stable, and to more easily track which commit (merge) introduced a particular problem.
  - pkamb 9y ago
    > makes it much hard to revert a merge later I don't think this would apply, as I'd be merging from `dev` > `feature`, resolving conflicts, and then doing a single clean no-conflict merge back from `feature` > `dev`. That's the only commit you'd need to revert to undo the merge. (or on a new, third, `merge` branch, if you don't want to pollute the `feature` branch with commits from `dev`) I admit the non-compiling commits might be a problem in some workflows, but `git bisect skip` is the solution to the only problem I've ever had with it.
    - jsteemann 9y ago
herpderperator 9y ago
That's why I say "you have conflicts" and have them resolve the conflict before I do the review. GitHub, at least, correctly diffs against master after the conflicts are resolved so you don't see any of the merged code (if it's the same as on the master branch), only the actual changes. Doesn't that solve the problem?

amigoingtodie 9y ago

What about code being 'deleted' that git diff does not catch? Is there a recommended external diff tool that ameliorates these issues?

hawski 9y ago
Maybe cat -v? $ git diff | cat -v However it's considered harmful: http://harmful.cat-v.org/cat-v/ http://harmful.cat-v.org/cat-v/ ;)
- UncleEntity 9y ago
  or git diff > tmp.diff and open with a text editor?
jwilk 9y ago
git diff | less -U But the you lose all the colors.

mikedilger 9y ago

If the output of a git command was raw text sent to stdout, like cat, then I would argue that escape sequences should be ignored and passed through unmolested. But the output of git tools is colored (if enabled) using terminal escape sequences. Given that, the output should fully comply with the appropriate terminal escape sequences and should filter the ones the author has discovered.

ComputerGuru 9y ago

I actually reported this same issue but under the context of a bug rather than as a security issue, exactly 2 months ago: https://public-inbox.org/git/CACcTrKctqAWeWWrc9Q+Y7ewXGc_o+uJoeHS83LDw5O_s1-3Nug@mail.gmail.com/ https://public-inbox.org/git/CACcTrKctqAWeWWrc9Q+Y7ewXGc_o+u... Same response, “it’s the pager’s job,” in that case.

jwilk 9y ago
I mentioned the problem on oss-security in 2016: http://openwall.com/lists/oss-security/2016/04/21/9 http://openwall.com/lists/oss-security/2016/04/21/9 (I think my claim that git escapes some control characters is incorrect; it's less that does it.) > Same response, “it’s the pager’s job,” in that case. It can't be the pagers job, because pager has no way to distingiush from escapes generated by git (to make things colorful) from malicious escapes.

CGamesPlay 9y ago

This is tenuous at best. iTerm2, at least, doesn't support the "conceal" color (^[[8m) and any other color would rely on the "victim's" color scheme to work properly. Git is safely removing sequences that could actually cause problems like OSC. This technique also wouldn't stand up to review on Github or any other online code review platform.

deathanatos 9y ago

Unix pipes lack any ability to say "this is the kind of data that I'm pushing you." The push raw, unidentified bytes. If the downstream program had some idea what format the data was in, it would know how to output it. Imagine an alternate universe where a pipe was a stream of bytes and a MIME type. If a terminal gets data written to it from a program in application/terminal.vt100, then it knows it should process those escape sequences. If it get text/plain; charset=utf-8, it knows it should not, and can take a different action. I think such a universe would make for more pleasant piping, too. Imagine that every interactive command on a terminal ended with an imaginary |show command; it's job is to read the mimetype and display, on the terminal, that data. A command that emitted CSV data could then automatically render in the terminal as an actual table. A program that emitted a PNG could render an ASCII art version of that image (or, since we're in an imaginary universe, imagine our terminal has escape sequences for images – which actually exists in some terminals today – it could emit the actual image!). Essentially, the program emits the data in whatever form it natively speaks, and that implicit |show parses that into something appropriate for the terminal. (That way, the program can still be used in pipelines, such as make_a_png | resize_it.) If you want your raw bytes, then you can imagine make_a_png | force_type 'application/octet-stream' and then the final, implicit |show would perhaps output a nice hexdump for you. (Since emitting raw bytes to a terminal never makes sense.) Now, I'm a bit fuzzy on exactly how the pipeline being executed, the shell, and the terminal all interact exactly, and I'm nearly certain such a change would require low-level, POSIX-breaking changes. But it was a dream I had, and I think it might be a better model than what we work w/ presently. (In the article's case, though, git log/diff both emit terminal sequences, so even in my imaginary world, they'd need to know that they should escape them. It mostly works for simpler types, but git rm could conceptually emit just text, since I don't think it ever colors.)

chickenfries 9y ago
> Imagine an alternate universe where a pipe was a stream of bytes and a MIME type. Isn't this powershell? > I'm nearly certain such a change would require low-level, POSIX-breaking changes. That seems to be the trade off, breaking with posix compatibility for nicer abstractions.
CGamesPlay 9y ago

jsteemann 9y ago

One mitigation for this is to use `col -bp` as a git pager, but it seems clumsy. However, for completeness I post it here with an example for how this makes the backdoor visible. Reproduce instruction: # clone repo and cd into it git clone https://github.com/twistlock/gitPocDiff https://github.com/twistlock/gitPocDiff cd gitPocDiff # show diff (this includes the backdoor but it won't be visible) git show ebc3f506a7ec8278e1a3ad4108612b66d10b41ca # expose the backdoor, using `col -bp` as pager git show ebc3f506a7ec8278e1a3ad4108612b66d10b41ca | col -bp

jwilk 9y ago
No, "col -bp" is not bulletproof. The attacker could still use backspace characters to hide malicious content.

carapace 9y ago

(Tiny grey sans-serif body text means you hate your readers' eyes.)

ChuckMcM 9y ago

Interesting to see how the standardization of escape sequences by ANSI leads to a vulnerability in code obfuscation.

shabble 9y ago
we need a 'secure escape' sequence that disables all subsequent escapes until rescinded! And it can change the background colour of the affected lines to a shade unrepresentable by unprivileged sequences. Mostly tongue-in-cheek, but... one of these years I'm going to track down what the heck the 'PRIVACY MESSAGE' sequence is actually for. ECMA48 is infuriatingly vague, and nothing seems to actually use or support it.

tomcam 9y ago

Unfortunately had to stop reading due to their text-hiding scheme--miniscule light-gray text on white background

mrob 9y ago
Firefox's Reader View reduces the problem, and with some custom CSS, solves it completely (see https://news.ycombinator.com/item?id=15080363 https://news.ycombinator.com/item?id=15080363 ). I imagine other browsers include a similar solution.
dimastopel 9y ago
This will be fixed. Thanks for the feedback.

kbutler 9y ago

The issue with with terminal handling of escape sequences, rather than with git. If you are concerned, you can use a gui diff tool, or you can pipe it through a filter that removes escape codes: seal:~/work/gitPocDiff$ cat > uncolor #!/usr/bin/env perl ## uncolor — remove terminal escape sequences such as color changes while (<>) { s/ \e[ #%()*+\-.\/]. | (?:\e\[|\x9b) [ -?]* [@-~] | # CSI ... Cmd (?:\e\]|\x9d) .*? (?:\e\\|[\a\x9c]) | # OSC ... (ST|BEL) (?:\e[P^_]|[\x90\x9e\x9f]) .*? (?:\e\\|\x9c) | # (DCS|PM|APC) ... ST \e.|[\x80-\x9f] //xg; print; } seal:~/work/gitPocDiff$ chmod a+x uncolor seal:~/work/gitPocDiff$ git diff eeeb ebc3 | ./uncolor diff --git a/main.c b/main.c index 6daa9b2..0321ff2 100644 --- a/main.c +++ b/main.c @@ -3,4 +3,9 @@ int main() { printf("I'm just a stub!\n"); + /* + * Must always return a value Hidden > */printf("Insert bad backdoor here...!\n");/* + * TODO: Return result + */ + return 0; } Credit to: gilles https://unix.stackexchange.com/a/14707 https://unix.stackexchange.com/a/14707

realusername 9y ago
You can also just pipe it to cat -e, it works the same. git diff | cat -e make all escape sequences visible.
- kbutler 9y ago
  There's always a utility... Thanks!

always_good 9y ago

Some pitiful Vimming right there. From holding down `h` for 30 seconds to go to the beginning of line to using :wq instead of :x.

unhammer 9y ago

magit ftw: https://i.imgur.com/a8Mvbkf.png https://i.imgur.com/a8Mvbkf.png Though I'm sure Emacs has loads of other vulnerabilities :)