{"thread":{"id":"45206","subject":"[BUG] allowtipsha1inwant serves unreachable blobs if you know its hash","startedAt":"2017-02-23T23:04:09Z","lastAt":"2017-02-24T00:44:09Z","messageCount":5,"participants":["Jonathan Tan","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"312448","messageId":"20170223230358.30050-1-jonathantanmy@google.com","threadId":"45206","inReplyTo":null,"subject":"[BUG] allowtipsha1inwant serves unreachable blobs if you know its hash","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-02-23T23:03:58Z","receivedAt":"2017-02-23T23:04:09Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"If a server sets allowtipsha1inwant (or allowreachablesha1inwant), a\nclient can call \"git fetch <server> <SHA-1>\" where SHA-1 is the hash of\na blob (reachable or unreachable) to obtain it. The test below (which\npasses) demonstrates that.\n\nI have bisected this, and this bug occurs at least as early as the\nintroduction of allowreachablesha1inwant in commit 68ee628\n(\"upload-pack: optionally allow fetching reachable sha1\", 2015-05-21).\nIt may have occurred earlier, though - initially, I thought that this\nwas due to allowreachablesha1inwant, so I did not investigate\nallowtipsha1inwant until later.\n\nI found this out while investigating the feasibility of using the\nexisting fetch-pack/upload-pack protocol to support fetching of\narbitrary blobs.\n\nThis happens most likely because the \"rev-list\" call in\n\"do_reachable_revlist\" (called through \"has_unreachable\") is invoked\nwithout the \"--objects\" argument. \"has_unreachable\" determines that an\nobject is unreachable if nothing is printed to stdout, which normally\nworks, except that \"rev-list\" prints nothing when asked which commits\nare reachable from a blob (which makes sense).\n\nAdding \"--objects\" works, and all existing tests pass, except for the\npotential performance issue and the side effect that even fetching a\nreachable blob no longer works. This is due to a possible bug where a\ncall like \"git rev-list --objects $tree ^master\" (where $tree is the\ntree object corresponding to master) prints out objects even though all\nobjects reachable from the tree are also reachable from master. (There\nshould be no issue with \"get_reachable_list\", the other invoker of\n\"do_reachable_revlist\", because non-commits in the command's stdout are\nskipped.)\n\nThe other option, of course, is to whitelist object types (that is, only\nallow commits and tags - I haven't checked if this is sufficient,\nthough). This might be a sufficient temporary fix, although I expect\nthat we would still have to revisit this later (especially if we decide\nthat we want to allow downloading blobs through this interface).\n\nI don't mind taking a look at either solution, but thought of putting\nthis out first in case people have any opinion or insight into this\nproblem and its possible solutions.\n\n(CC-ing some people who might have an interest in downloading arbitrary\nblobs.)\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n t/t9999-mytests.sh | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 61 insertions(+)\n create mode 100644 t/t9999-mytests.sh\n\ndiff --git a/t/t9999-mytests.sh b/t/t9999-mytests.sh\nnew file mode 100644\nindex 000000000..e71cfbf48\n--- /dev/null\n+++ b/t/t9999-mytests.sh\n@@ -0,0 +1,61 @@\n+#!/bin/sh\n+\n+test_description='my tests'\n+. ./test-lib.sh\n+. \"$TEST_DIRECTORY\"/lib-httpd.sh\n+start_httpd\n+\n+test_expect_success 'setup' '\n+\tserver=\"$HTTPD_DOCUMENT_ROOT_PATH/blobs\" &&\n+\tgit init \"$server\" &&\n+\t(\n+\t\tcd \"$server\" &&\n+\t\ttest_commit myfile\n+\t) &&\n+\n+\t# A reachable blob.\n+\treachable=$(\n+\t\tgit -C \"$server\" cat-file -p \\\n+\t\t$(git -C \"$server\" cat-file -p HEAD | grep \"^tree \" | cut -c6-) | \\\n+\t\tgrep myfile | cut -c13-52) &&\n+\ttest -n \"$reachable\" &&\n+\n+\t# 2 unreachable blobs. Only one will be fetched. (2 are included here\n+\t# to demonstrate that there is no whole-repo copying or anything like\n+\t# that.)\n+\tunreachable=$(echo abc123 | git -C \"$server\" hash-object -w --stdin) &&\n+\ttest -n \"$unreachable\" &&\n+\tanother_unreachable=$(echo def456 | git -C \"$server\" hash-object -w --stdin) &&\n+\ttest -n \"$another_unreachable\" &&\n+\n+\tgit init --bare client.git &&\n+\tgit -C client.git remote add origin \"$HTTPD_URL/smart/blobs\" &&\n+\n+\t# These fetches are supposed to fail (and do)\n+\ttest_must_fail git -C client.git fetch origin $reachable &&\n+\ttest_must_fail git -C client.git fetch origin $unreachable\n+'\n+\n+test_expect_success 'allowtipsha1inwant suddenly allows blobs' '\n+\ttest -n \"$reachable\" &&\n+\n+\tgit -C \"$server\" config uploadpack.allowtipsha1inwant 1 &&\n+\n+\t# This fetch passes\n+\tgit -C client.git fetch origin $reachable &&\n+\ttest \"myfile\" = $(git -C client.git cat-file -p $reachable)\n+'\n+\n+test_expect_success 'even unreachable ones' '\n+\ttest -n \"$unreachable\" &&\n+\n+\t# This fetch is supposed to fail (for multiple reasons), but passes.\n+\t# Only the wanted blob is fetched.\n+\tgit -C client.git fetch origin $unreachable &&\n+\tgit -C client.git cat-file -e $unreachable &&\n+\ttest_must_fail git -C client.git cat-file -e $another_unreachable &&\n+\ttest \"abc123\" = $(git -C client.git cat-file -p $unreachable)\n+'\n+\n+stop_httpd\n+test_done\n-- \n2.11.0.483.g087da7b7c-goog\n\n"},{"id":"312483","messageId":"xmqqtw7k5uxj.fsf@gitster.mtv.corp.google.com","threadId":"45206","inReplyTo":"20170223230358.30050-1-jonathantanmy@google.com","subject":"Re: [BUG] allowtipsha1inwant serves unreachable blobs if you know its hash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-23T23:50:00Z","receivedAt":"2017-02-23T23:50:38Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> Adding \"--objects\" works, and all existing tests pass, except for the\n> potential performance issue and the side effect that even fetching a\n> reachable blob no longer works. This is due to a possible bug where a\n> call like \"git rev-list --objects $tree ^master\" (where $tree is the\n> tree object corresponding to master) prints out objects ...\n\nThe \"reachable from this, excluding what is reachable from that\"\nnotation was originally designed to work only on commits, and I\nwouldn't be surprised if \"$tree ^master\" did not work as you expect\nin the current implementation.\n\nI agree that ideally it shouldn't show anything, but I suspect that\nit would make it very expensive if done naively---we'd end up having\nto call mark_tree_uninteresting() for all uninteresting commits, not\njust for the commits at the edge of the DAG as we do right now.\n"},{"id":"312486","messageId":"20170224000522.6ctrglphidmbsei7@sigill.intra.peff.net","threadId":"45206","inReplyTo":"xmqqtw7k5uxj.fsf@gitster.mtv.corp.google.com","subject":"Re: [BUG] allowtipsha1inwant serves unreachable blobs if you know its hash","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-24T00:05:23Z","receivedAt":"2017-02-24T00:12:18Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 23, 2017 at 03:50:00PM -0800, Junio C Hamano wrote:\n\n> Jonathan Tan <jonathantanmy@google.com> writes:\n> \n> > Adding \"--objects\" works, and all existing tests pass, except for the\n> > potential performance issue and the side effect that even fetching a\n> > reachable blob no longer works. This is due to a possible bug where a\n> > call like \"git rev-list --objects $tree ^master\" (where $tree is the\n> > tree object corresponding to master) prints out objects ...\n> \n> The \"reachable from this, excluding what is reachable from that\"\n> notation was originally designed to work only on commits, and I\n> wouldn't be surprised if \"$tree ^master\" did not work as you expect\n> in the current implementation.\n> \n> I agree that ideally it shouldn't show anything, but I suspect that\n> it would make it very expensive if done naively---we'd end up having\n> to call mark_tree_uninteresting() for all uninteresting commits, not\n> just for the commits at the edge of the DAG as we do right now.\n\nYes, it's super-expensive to do naively (like 40+ seconds of CPU on\ntorvalds/linux). Bitmaps should generally make it tolerable, though\nthere are corner cases (e.g., if a ref tip has to walk a bit to get to a\nbitmap; we try to put bitmaps near the ref tips, but when you have\n50,000 tags it's hard to do).\n\nWe've explored similar things at GitHub for doing reachability checks on\nall 40-hex lookups (because right now I can point you to\n.../git/git/commit/1234abcd and you see that object, even if it's only\nin _my_ fork and not reachable from git/git).\n\n-Peff\n"},{"id":"312490","messageId":"xmqqpoi85ui3.fsf@gitster.mtv.corp.google.com","threadId":"45206","inReplyTo":"xmqqtw7k5uxj.fsf@gitster.mtv.corp.google.com","subject":"Re: [BUG] allowtipsha1inwant serves unreachable blobs if you know its hash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-23T23:59:16Z","receivedAt":"2017-02-24T00:31:00Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Jonathan Tan <jonathantanmy@google.com> writes:\n>\n>> Adding \"--objects\" works, and all existing tests pass, except for the\n>> potential performance issue and the side effect that even fetching a\n>> reachable blob no longer works. This is due to a possible bug where a\n>> call like \"git rev-list --objects $tree ^master\" (where $tree is the\n>> tree object corresponding to master) prints out objects ...\n>\n> The \"reachable from this, excluding what is reachable from that\"\n> notation was originally designed to work only on commits, and I\n> wouldn't be surprised if \"$tree ^master\" did not work as you expect\n> in the current implementation.\n>\n> I agree that ideally it shouldn't show anything, but I suspect that\n> it would make it very expensive if done naively---we'd end up having\n> to call mark_tree_uninteresting() for all uninteresting commits, not\n> just for the commits at the edge of the DAG as we do right now.\n\nBTW, by \"it would be expensive\" I didn't mean \"hence it shouldn't be\ndone.\"  Even though I do not know by how much expensive it would\nbecome, I think this approach is a lot better way going forward than\npunting and say \"no, you cannot specify anything lower than commit.\"\n\nThanks for looking into this.\n\n"},{"id":"312491","messageId":"20170224004401.iil6kcjfkrifrij2@sigill.intra.peff.net","threadId":"45206","inReplyTo":"20170223230358.30050-1-jonathantanmy@google.com","subject":"Re: [BUG] allowtipsha1inwant serves unreachable blobs if you know its hash","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-24T00:44:01Z","receivedAt":"2017-02-24T00:44:09Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 23, 2017 at 03:03:58PM -0800, Jonathan Tan wrote:\n\n> If a server sets allowtipsha1inwant (or allowreachablesha1inwant), a\n> client can call \"git fetch <server> <SHA-1>\" where SHA-1 is the hash of\n> a blob (reachable or unreachable) to obtain it. The test below (which\n> passes) demonstrates that.\n\nThanks for a nice demonstration.\n\nTBH, I think the whole \"we will not give you unreachable things\" is\noverblown as a security measure. There are a lot of ways to leak\ninformation out of the repo. E.g., claiming you _do_ have the\nunreachable thing, at which point pack-objects may create a delta\nagainst it. Done repeatedly, it works as an oracle by which you can\nguess the contents of the unreachable thing.\n\n> This happens most likely because the \"rev-list\" call in\n> \"do_reachable_revlist\" (called through \"has_unreachable\") is invoked\n> without the \"--objects\" argument. \"has_unreachable\" determines that an\n> object is unreachable if nothing is printed to stdout, which normally\n> works, except that \"rev-list\" prints nothing when asked which commits\n> are reachable from a blob (which makes sense).\n> \n> Adding \"--objects\" works, and all existing tests pass, except for the\n> potential performance issue and the side effect that even fetching a\n> reachable blob no longer works. This is due to a possible bug where a\n> call like \"git rev-list --objects $tree ^master\" (where $tree is the\n> tree object corresponding to master) prints out objects even though all\n> objects reachable from the tree are also reachable from master. (There\n> should be no issue with \"get_reachable_list\", the other invoker of\n> \"do_reachable_revlist\", because non-commits in the command's stdout are\n> skipped.)\n\nI think \"--objects --use-bitmap-index\" would be faster. If you have\nbitmaps.\n\n-Peff\n"}]}