git/bundle-uri.c, branch v2.45.4

bundle-uri: fix arbitrary file writes via parameter injection

2025-05-23T21:09:48Z

We fetch bundle URIs via `download_https_uri_to_file()`. The logic to fetch those bundles is not handled in-process, but we instead use a separate git-remote-https(1) process that performs the fetch for us. The information about which file should be downloaded and where that file should be put gets communicated via stdin of that process via a "get" request. This "get" request has the form "get $uri $file\n\n". As may be obvious to the reader, this will cause git-remote-https(1) to download the URI "$uri" and put it into "$file". The fact that we are using plain spaces and newlines as separators for the request arguments means that we have to be extra careful with the respective vaules of these arguments: - If "$uri" contained a space we would interpret this as both URI and target location. - If either "$uri" or "$file" contained a newline we would interpret this as a new command. But we neither quote the arguments such that any characters with special meaning would be escaped, nor do we verify that none of these special characters are contained. If either the URI or file contains a newline character, we are open to protocol injection attacks. Likewise, if the URI itself contains a space, then an attacker-controlled URI can lead to partially-controlled file writes. Note that the attacker-controlled URIs do not permit completely arbitrary file writes, but instead allows an attacker to control the path in which we will write a temporary (e.g., "tmp_uri_XXXXXX") file. The result is twofold: - By adding a space in "$uri" we can control where exactly a file will be written to, including out-of-repository writes. The final location is not completely arbitrary, as the injected string will be concatenated with the original "$file" path. Furthermore, the name of the bundle will be "tmp_uri_XXXXXX", further restricting what an adversary would be able to write. Also note that is not possible for the URI to contain a newline because we end up in `credential_from_url_1()` before we try to issue any requests using that URI. As such, it is not possible to inject arbitrary commands via the URI. - By adding a newline to "$file" we can inject arbitrary commands. This gives us full control over where a specific file will be written to. Potential attack vectors would be to overwrite hooks, but if an adversary were to guess where the user's home directory is located they might also easily write e.g. a "~/.profile" file and thus cause arbitrary code execution. This injection can only become possible when the adversary has full control over the target path where a bundle will be downloaded to. While this feels unlikely, it is possible to control this path when users perform a recursive clone with a ".gitmodules" file that is controlled by the adversary. Luckily though, the use of bundle URIs is not enabled by default in Git clients (yet): they have to be enabled by setting the `bundle.heuristic` config key explicitly. As such, the blast radius of this parameter injection should overall be quite contained. Fix the issue by rejecting spaces in the URI and newlines in both the URI and the file. As explained, it shouldn't be required to also restrict the use of newlines in the URI, as we would eventually die anyway in `credential_from_url_1()`. But given that we're only one small step away from arbitrary code execution, let's rather be safe and restrict newlines in URIs, as well. Eventually we should probably refactor the way that Git talks with the git-remote-https(1) subprocess so that it is less fragile. Until then, these two restrictions should plug the issue. Reported-by: David Leadbeater Based-on-patch-by: David Leadbeater Signed-off-by: Patrick Steinhardt Signed-off-by: Taylor Blau

treewide: remove unnecessary includes in source files

2023-12-26T20:04:31Z

Each of these were checked with gcc -E -I. ${SOURCE_FILE} | grep ${HEADER_FILE} to ensure that removing the direct inclusion of the header actually resulted in that header no longer being included at all (i.e. that no other header pulled it in transitively). ...except for a few cases where we verified that although the header was brought in transitively, nothing from it was directly used in that source file. These cases were: * builtin/credential-cache.c * builtin/pull.c * builtin/send-pack.c Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano

bundle-uri: mark unused parameters in callbacks

2023-08-30T00:56:26Z

The first hunk is similar to 02c3c59e62 (hashmap: mark unused callback parameters, 2022-08-19), but was added after that commit. The other two are used with for_all_bundles_in_list(), but don't use their void data pointer. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

Merge branch 'gc/config-context'

2023-07-06T18:54:48Z

Reduce reliance on a global state in the config reading API. * gc/config-context: config: pass source to config_parser_event_fn_t config: add kvi.path, use it to evaluate includes config.c: remove config_reader from configsets config: pass kvi to die_bad_number() trace2: plumb config kvi config.c: pass ctx with CLI config config: pass ctx with config files config.c: pass ctx in configsets config: add ctx arg to config_fn_t urlmatch.h: use config_fn_t type config: inline git_color_default_config

config: pass ctx with config files

2023-06-28T21:06:39Z

Pass config_context to config_callbacks when parsing config files. To provide the .kvi member, refactor out the configset logic that caches "struct config_source" and "enum config_scope" as a "struct key_value_info". Make the "enum config_scope" available to the config file machinery by plumbing an additional arg through git_config_from_file_with_options(). We do not exercise ctx yet because the remaining current_config_*() callers may be used with config_with_options(), which may read config from parameters, but parameters don't pass ctx yet. Signed-off-by: Glen Choo Signed-off-by: Junio C Hamano

config: add ctx arg to config_fn_t

2023-06-28T21:06:39Z

Add a new "const struct config_context *ctx" arg to config_fn_t to hold additional information about the config iteration operation. config_context has a "struct key_value_info kvi" member that holds metadata about the config source being read (e.g. what kind of config source it is, the filename, etc). In this series, we're only interested in .kvi, so we could have just used "struct key_value_info" as an arg, but config_context makes it possible to add/adjust members in the future without changing the config_fn_t signature. We could also consider other ways of organizing the args (e.g. moving the config name and value into config_context or key_value_info), but in my experiments, the incremental benefit doesn't justify the added complexity (e.g. a config_fn_t will sometimes invoke another config_fn_t but with a different config value). In subsequent commits, the .kvi member will replace the global "struct config_reader" in config.c, making config iteration a global-free operation. It requires much more work for the machinery to provide meaningful values of .kvi, so for now, merely change the signature and call sites, pass NULL as a placeholder value, and don't rely on the arg in any meaningful way. Most of the changes are performed by contrib/coccinelle/config_fn_ctx.pending.cocci, which, for every config_fn_t: - Modifies the signature to accept "const struct config_context *ctx" - Passes "ctx" to any inner config_fn_t, if needed - Adds UNUSED attributes to "ctx", if needed Most config_fn_t instances are easily identified by seeing if they are called by the various config functions. Most of the remaining ones are manually named in the .cocci patch. Manual cleanups are still needed, but the majority of it is trivial; it's either adjusting config_fn_t that the .cocci patch didn't catch, or adding forward declarations of "struct config_context ctx" to make the signatures make sense. The non-trivial changes are in cases where we are invoking a config_fn_t outside of config machinery, and we now need to decide what value of "ctx" to pass. These cases are: - trace2/tr2_cfg.c:tr2_cfg_set_fl() This is indirectly called by git_config_set() so that the trace2 machinery can notice the new config values and update its settings using the tr2 config parsing function, i.e. tr2_cfg_cb(). - builtin/checkout.c:checkout_main() This calls git_xmerge_config() as a shorthand for parsing a CLI arg. This might be worth refactoring away in the future, since git_xmerge_config() can call git_default_config(), which can do much more than just parsing. Handle them by creating a KVI_INIT macro that initializes "struct key_value_info" to a reasonable default, and use that to construct the "ctx" arg. Signed-off-by: Glen Choo Signed-off-by: Junio C Hamano

object-store-ll.h: split this header out of object-store.h

2023-06-21T20:39:54Z

The vast majority of files including object-store.h did not need dir.h nor khash.h. Split the header into two files, and let most just depend upon object-store-ll.h, while letting the two callers that need it depend on the full object-store.h. After this patch: $ git grep -h include..object-store | sort | uniq -c 2 #include "object-store.h" 129 #include "object-store-ll.h" Diff best viewed with `--color-moved`. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano

Merge branch 'en/header-split-cache-h-part-2'

2023-05-09T23:45:46Z

More header clean-up. * en/header-split-cache-h-part-2: (22 commits) reftable: ensure git-compat-util.h is the first (indirect) include diff.h: reduce unnecessary includes object-store.h: reduce unnecessary includes commit.h: reduce unnecessary includes fsmonitor: reduce includes of cache.h cache.h: remove unnecessary headers treewide: remove cache.h inclusion due to previous changes cache,tree: move basic name compare functions from read-cache to tree cache,tree: move cmp_cache_name_compare from tree.[ch] to read-cache.c hash-ll.h: split out of hash.h to remove dependency on repository.h tree-diff.c: move S_DIFFTREE_IFXMIN_NEQ define from cache.h dir.h: move DTYPE defines from cache.h versioncmp.h: move declarations for versioncmp.c functions from cache.h ws.h: move declarations for ws.c functions from cache.h match-trees.h: move declarations for match-trees.c functions from cache.h pkt-line.h: move declarations for pkt-line.c functions from cache.h base85.h: move declarations for base85.c functions from cache.h copy.h: move declarations for copy.c functions from cache.h server-info.h: move declarations for server-info.c functions from cache.h packfile.h: move pack_window and pack_entry from cache.h ...

treewide: remove cache.h inclusion due to previous changes

2023-04-24T19:47:33Z

Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano

copy.h: move declarations for copy.c functions from cache.h

2023-04-24T19:47:31Z

Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano