From patchwork Sat Nov 30 01:09:29 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Elijah Newren X-Patchwork-Id: 13889041 Received: from mail-wm1-f54.google.com (mail-wm1-f54.google.com [209.85.128.54]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 9B5D1AD31 for ; Sat, 30 Nov 2024 01:09:34 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.54 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1732928976; cv=none; b=lGhNo7wAFSfNdGEiJUopzApnm93dELj5fxco3SeNa5FNeV2tAGliAROeYyXmmKsWx8/qkfMqC46a1vp/xLa6HzQtpy5V3phvo79oB6Kl45U2/5C5Mu062I63aDebSNThqK4FkXm5Z5ZF6t+WfrmuSc7VVxWOLKvc2uaJ9qmxb18= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1732928976; c=relaxed/simple; bh=Vpsr+9loWkFPvCSEJTQ9EzJGHu88ExuTSGQx9rLmY54=; h=Message-Id:In-Reply-To:References:From:Date:Subject:Content-Type: MIME-Version:To:Cc; b=bwA1f1OC6liazN7dbQBI8nW1Yc8uA/g7lPYRwnuaUIk1+1XisSqbCnElLefV4B8VLK0owAfba3RpnJxnlNbdIF1xAp1EXpP3w4jPzsclzni3IwNWFtrAN1ROJZz7WAwH+gJID9G1xyTF1QS86OwqhggXIqbJ8/m2MKCd+GRtHQ8= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=UWB9443V; arc=none smtp.client-ip=209.85.128.54 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="UWB9443V" Received: by mail-wm1-f54.google.com with SMTP id 5b1f17b1804b1-434a742481aso21514195e9.3 for ; Fri, 29 Nov 2024 17:09:34 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1732928972; x=1733533772; darn=vger.kernel.org; h=cc:to:mime-version:content-transfer-encoding:fcc:subject:date:from :references:in-reply-to:message-id:from:to:cc:subject:date :message-id:reply-to; bh=4yMAJgGxZGPXADE+VpgJ3FnqhSuyL7NwwSKyGFYNnUE=; b=UWB9443VSwEp5hghxkAPY0apdFSCJVSa3G61vKxIwBn78nKwPTIIzE23qsSMJt3x5t 5u2y9nL7e6DNM52QoUgdh+8oIQCj7FJraKfKByZ7TM137hap7/sXjefNb57gfvqlzMkp s17Oa+mW+lYrHdt4ADL6wVI+5/csMtoMPhOvmEPVxrAk1CNEMYjTMP7BPC8Guve5gXQB vZh1Ifc39eVoOgJQixblzefSfNkW6+uCJVf44hbm5rLjgdyVhmA3OfK3NIBHPncFzspv 1g9x3UKgSI7+YXAU9cFiTwOzJTL32K/JPi/+brhoUmHsQ6OPuNrl9Ai9a3/aQI7s/5RY wQig== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1732928972; x=1733533772; h=cc:to:mime-version:content-transfer-encoding:fcc:subject:date:from :references:in-reply-to:message-id:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=4yMAJgGxZGPXADE+VpgJ3FnqhSuyL7NwwSKyGFYNnUE=; b=pO6xPEZfjE51k8jnrII1i7/NXhNZ7v35M3EUgIGMhpy3tLJFXEUZ5XPO3aROEs5xt9 Hxib8R0vq+QA6099rCZuOoMulW/8KVlX9mA4UI6lXUCFwqD/6rIGomidRNKhpmHIdgqE GUk5yt7Pm9tLg0k0D/eHogYfx/AZZ+AeL3dOi1LFpz0fITMhiH/OF6XUKjtce8Gw/4wA w+ZT/Vm1skpm20ll2MvmxEf6/Q7tfpTM8lHZUQClryxPeIu9iLh/74Ksblx76IVJNF9a YelzPe923ERCToQKyILeQJMsn0MSqZphTqjxNyqqPlyBM00EMPfMtUvZ1g9V1ZTSxNPi Harg== X-Gm-Message-State: AOJu0YwDClkgn/qqP+j8j8PICoaN+ykcb6Xb8fm4IBqFPOE0V3XbZw5O q8yTtJe+gYEY+ysuHfiXfcvAtvgFVGOquDxN7p4JWZVUF67Xn3pfyT0flA== X-Gm-Gg: ASbGncslsa4BBmHAJ88qbbDMc+nEfK52r0biWHc+wNNi8ycQD8E/nv/zifBZ3GqaXxy +z9rF3eH415HNQqhLPZucGqnVGn6g/tJEvL5jJ7uOUrG1zaH5JlkBdvWgBRmxyy/QYTX6093Toh j7zFUnUI/E+3q+/ieKbmFLDhJQoviXC+pAhUyObPk+xNrSIFP6/kw/ucFjjn5XT/MPAbilnlEDE yHHOqiatHrT8cWVTHsMboH342+0kt4B3oSOPUaGjDFaWdSHJkM= X-Google-Smtp-Source: AGHT+IEHiKEWpBVdIQVUYw0FeiHEsAO18ykzUzk0NSIIn6KANR/VdehuxE06lJgjY57oluZf/a9xLQ== X-Received: by 2002:a05:600c:348a:b0:431:4e25:fe42 with SMTP id 5b1f17b1804b1-434a9e0fdbdmr119930355e9.32.1732928971861; Fri, 29 Nov 2024 17:09:31 -0800 (PST) Received: from [127.0.0.1] ([13.74.141.28]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-434aa6e929csm103189135e9.0.2024.11.29.17.09.30 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 29 Nov 2024 17:09:31 -0800 (PST) Message-Id: In-Reply-To: References: Date: Sat, 30 Nov 2024 01:09:29 +0000 Subject: [PATCH v2] fast-import: disallow more path components Fcc: Sent Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 To: git@vger.kernel.org Cc: Eric Sunshine , Patrick Steinhardt , Kristoffer Haugsbakk , Jeff King , Elijah Newren , Elijah Newren From: Elijah Newren From: Elijah Newren Instead of just disallowing '.' and '..', make use of verify_path() to ensure that fast-import will disallow anything we wouldn't allow into the index, such as anything under .git/, .gitmodules as a symlink, or a dos drive prefix on Windows. Since a few fast-export and fast-import tests that tried to stress-test the correct handling of quoting relied on filenames that fail is_valid_win32_path(), such as spaces or periods at the end of filenames or backslashes within the filename, turn off core.protectNTFS for those tests to ensure they keep passing. Helped-by: Jeff King Signed-off-by: Elijah Newren --- Disallow verify_path() failures from fast-import Since en/fast-import-path-sanitize has already made it to next, this commit is based on that. (See https://lore.kernel.org/git/pull.1831.v2.git.1732561248717.gitgitgadget@gmail.com/ for discussion of that series.) Changes relative to that commit: this fixes up the error message as suggested by Kristoffer, and makes the checks more encompassing as suggested by Patrick and Peff -- in particular, using verify_path() as suggested by Peff. Changes since v1: * Moved the check to a higher level, as suggested by Peff. Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1832%2Fnewren%2Fdisallow-verify-path-fast-import-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1832/newren/disallow-verify-path-fast-import-v2 Pull-Request: https://github.com/gitgitgadget/git/pull/1832 Range-diff vs v1: 1: 2c23d601112 ! 1: 787e9d71ae7 fast-import: disallow more path components @@ Commit message or backslashes within the filename, turn off core.protectNTFS for those tests to ensure they keep passing. + Helped-by: Jeff King Signed-off-by: Elijah Newren ## builtin/fast-import.c ## @@ builtin/fast-import.c #include "refs.h" #include "csum-file.h" #include "quote.h" -@@ builtin/fast-import.c: static int tree_content_set( - die("Empty path component found in input"); - if (!*slash1 && !S_ISDIR(mode) && subtree) - die("Non-directories cannot have subtrees"); -+ if (!verify_path(p, mode)) -+ die("invalid path '%s'", p); - - if (!root->tree) - load_tree(root); @@ builtin/fast-import.c: static int tree_content_set( root->tree = t = grow_tree_content(t, t->entry_count); e = new_tree_entry(); @@ builtin/fast-import.c: static int tree_content_set( e->versions[0].mode = 0; oidclr(&e->versions[0].oid, the_repository->hash_algo); t->entries[t->entry_count++] = e; +@@ builtin/fast-import.c: static void file_change_m(const char *p, struct branch *b) + tree_content_replace(&b->branch_tree, &oid, mode, NULL); + return; + } ++ ++ if (!verify_path(path.buf, mode)) ++ die("invalid path '%s'", path.buf); + tree_content_set(&b->branch_tree, path.buf, &oid, mode, NULL); + } + +@@ builtin/fast-import.c: static void file_change_cr(const char *p, struct branch *b, int rename) + leaf.tree); + return; + } ++ if (!verify_path(dest.buf, leaf.versions[1].mode)) ++ die("invalid path '%s'", dest.buf); + tree_content_set(&b->branch_tree, dest.buf, + &leaf.versions[1].oid, + leaf.versions[1].mode, ## t/t9300-fast-import.sh ## @@ t/t9300-fast-import.sh: test_expect_success 'B: fail on invalid committer (5)' ' builtin/fast-import.c | 8 +++- t/t9300-fast-import.sh | 88 ++++++++++++++++++++++++++++++++++++++++-- t/t9350-fast-export.sh | 2 +- 3 files changed, 91 insertions(+), 7 deletions(-) base-commit: 4a2790a257b314ab59f6f2e25f3d7ca120219922 diff --git a/builtin/fast-import.c b/builtin/fast-import.c index 3e7ec1f1198..265d1b7d52b 100644 --- a/builtin/fast-import.c +++ b/builtin/fast-import.c @@ -13,6 +13,7 @@ #include "delta.h" #include "pack.h" #include "path.h" +#include "read-cache-ll.h" #include "refs.h" #include "csum-file.h" #include "quote.h" @@ -1468,8 +1469,6 @@ static int tree_content_set( root->tree = t = grow_tree_content(t, t->entry_count); e = new_tree_entry(); e->name = to_atom(p, n); - if (is_dot_or_dotdot(e->name->str_dat)) - die("path %s contains invalid component", p); e->versions[0].mode = 0; oidclr(&e->versions[0].oid, the_repository->hash_algo); t->entries[t->entry_count++] = e; @@ -2416,6 +2415,9 @@ static void file_change_m(const char *p, struct branch *b) tree_content_replace(&b->branch_tree, &oid, mode, NULL); return; } + + if (!verify_path(path.buf, mode)) + die("invalid path '%s'", path.buf); tree_content_set(&b->branch_tree, path.buf, &oid, mode, NULL); } @@ -2453,6 +2455,8 @@ static void file_change_cr(const char *p, struct branch *b, int rename) leaf.tree); return; } + if (!verify_path(dest.buf, leaf.versions[1].mode)) + die("invalid path '%s'", dest.buf); tree_content_set(&b->branch_tree, dest.buf, &leaf.versions[1].oid, leaf.versions[1].mode, diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh index 5a5127fffa7..e2b1db6bc2f 100755 --- a/t/t9300-fast-import.sh +++ b/t/t9300-fast-import.sh @@ -522,7 +522,7 @@ test_expect_success 'B: fail on invalid committer (5)' ' test_must_fail git fast-import input <<-INPUT_END && blob mark :1 @@ -542,6 +542,86 @@ test_expect_success 'B: fail on invalid file path' ' test_must_fail git fast-import input <<-INPUT_END && + blob + mark :1 + data < $GIT_COMMITTER_DATE + data <input <<-INPUT_END && + blob + mark :1 + data < $GIT_COMMITTER_DATE + data <input <<-INPUT_END && + blob + mark :1 + data < $GIT_COMMITTER_DATE + data <input <<-INPUT_END && + blob + mark :1 + data < $GIT_COMMITTER_DATE + data <output && cut -d" " -f1,2,5 output >actual && test_cmp expect actual @@ -3117,7 +3197,7 @@ test_path_eol_success () { test_expect_success "S: paths at EOL with $test must work" ' test_when_finished "git branch -D S-path-eol" && - git fast-import --export-marks=marks.out <<-EOF >out 2>err && + git -c core.protectNTFS=false fast-import --export-marks=marks.out <<-EOF >out 2>err && blob mark :401 data <err && + git -c core.protectNTFS=false fast-import --export-marks=marks.out <<-EOF 2>err && blob mark :401 data <expect && git init result && cd result && - git fast-import <../export.out && + git -c core.protectNTFS=false fast-import <../export.out && git rev-list HEAD >actual && test_cmp ../expect actual ) From patchwork Tue Dec 3 21:06:52 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13892974 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id BF157204F78 for ; Tue, 3 Dec 2024 21:07:00 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=104.130.231.41 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1733260023; cv=none; b=QPMHhDl2QNtg/FbhrqIlu4WgNwv4Y32L+NB5o+b1e1MHSkI/5odGAV6lwvs9aCR5UWgdmWOYLLUWP6UJ4SKHaN/X++JaMK5JHwotVIPi186A7BI8XrFMmQG9XvPy/qxwUH6PqiX0l1SLAwaxyVVX6Xs/ZWOuP0t/K2y4xeWv+1c= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1733260023; c=relaxed/simple; bh=g5QEsqpc00Z7J9UfzNUaeX/naI97YTEXMjXhZVO6hq8=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=CrSfm4VJWD9yuy65m8xmhNxykOFad0fmzJ9ge4ob+/iQx176Jpu3tRod88aiRWVkJeKXz9od2qYqMiRyZsZ+WR6viQvZ2kjc0q6Hxu5ggjSLYdeOnb7Fw1vHrU0ZbdbA8+MPl7LWpHGcSjEZYUy3SgNavPNHMkMU2p8+h+/Qd3A= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=peff.net; spf=pass smtp.mailfrom=peff.net; dkim=pass (2048-bit key) header.d=peff.net header.i=@peff.net header.b=aReUZrra; arc=none smtp.client-ip=104.130.231.41 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=peff.net Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=peff.net Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=peff.net header.i=@peff.net header.b="aReUZrra" Received: (qmail 30458 invoked by uid 109); 3 Dec 2024 21:06:53 -0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed; d=peff.net; h=date:from:to:cc:subject:message-id:references:mime-version:content-type:in-reply-to; s=20240930; bh=g5QEsqpc00Z7J9UfzNUaeX/naI97YTEXMjXhZVO6hq8=; b=aReUZrra0M3+xIb+0ehzcdbmtMmhmQnULRfGh+d7nW5ooDdYBAUJYSid4hGkBDT8wTwUZmOocok80VWlWUVUi+AsMUBaWp1Q0uaI37rx9nms1p3Gc7silmMHKtmM4rgYqlPPUvKMnnYXltEssq/UTFvvkMzw1SqhH8EfeF12/MhSMeHCkR05k/Wi67FCRH6f9vmshKlKsCOp6B1wVcidP6GNlDhJtGSvpxHfBUhYPF6zJpG4vPd+PvKSrtOEDh6iGqmjHnfIZueFKnhvjLPMbSzNraGfpPjBJ8uKwKWwNlOXpx3bmlGWE0ShbDQqT74OJai3l+4OfuJ1tNPQW35M0Q== Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Tue, 03 Dec 2024 21:06:53 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 10197 invoked by uid 111); 3 Dec 2024 21:06:52 -0000 Received: from coredump.intra.peff.net (HELO coredump.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Tue, 03 Dec 2024 16:06:52 -0500 Authentication-Results: peff.net; auth=none Date: Tue, 3 Dec 2024 16:06:52 -0500 From: Jeff King To: Elijah Newren Cc: Junio C Hamano , Elijah Newren via GitGitGadget , git@vger.kernel.org, Eric Sunshine , Patrick Steinhardt , Kristoffer Haugsbakk Subject: [PATCH 2/1] t9300: test verification of renamed paths Message-ID: <20241203210652.GA1413195@coredump.intra.peff.net> References: <20241201214014.GC145938@coredump.intra.peff.net> Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: On Tue, Dec 03, 2024 at 12:01:51AM -0800, Elijah Newren wrote: > > On Sat, Nov 30, 2024 at 01:09:29AM +0000, Elijah Newren via GitGitGadget wrote: > > > > > Changes since v1: > > > > > > * Moved the check to a higher level, as suggested by Peff. > > > > Thanks, the code change looks good. Is it worth tweaking one of the > > tests to do "R innocent-path .git/evil"? Otherwise I don't think there's > > any coverage of the file_change_cr() call at all. > > I would say yes, but since this patch too has made it to next and is > marked for master, I'm kinda tempted to just leave it as-is... Is is tempting. :) I wrote this up, though, which can just go on top (of en/fast-import-verify-path). -Peff -- >8 -- Subject: [PATCH] t9300: test verification of renamed paths Commit da91a90c2f (fast-import: disallow more path components, 2024-11-30) added two separate verify_path() calls (one for added/modified files, and one for renames/copies). But our tests only exercise the first one. Let's protect ourselves against regressions by tweaking one of the tests to rename into the bad path. There are adjacent tests that will stay as additions, so now both calls are covered. Signed-off-by: Jeff King --- t/t9300-fast-import.sh | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh index e2b1db6bc2..fd01a2353c 100755 --- a/t/t9300-fast-import.sh +++ b/t/t9300-fast-import.sh @@ -553,9 +553,16 @@ test_expect_success 'B: fail on invalid file path of .' ' commit refs/heads/badpath committer Name $GIT_COMMITTER_DATE data < $GIT_COMMITTER_DATE + data <