diff mbox series

[v4,1/8] finalize_object_file(): check for name collision before renaming

Message ID 6f1ee91fff315678fef39a54220eae91632d2df9.1727199118.git.me@ttaylorr.com (mailing list archive)
State Superseded
Headers show
Series hash.h: support choosing a separate SHA-1 for non-cryptographic uses | expand

Commit Message

Taylor Blau Sept. 24, 2024, 5:32 p.m. UTC 
We prefer link()/unlink() to rename() for object files, with the idea
that we should prefer the data that is already on disk to what is
incoming. But we may fall back to rename() if the user has configured us
to do so, or if the filesystem seems not to support cross-directory
links. This loses the "prefer what is on disk" property.

We can mitigate this somewhat by trying to stat() the destination
filename before doing the rename. This is racy, since the object could
be created between the stat() and rename() calls. But in practice it is
expanding the definition of "what is already on disk" to be the point
that the function is called. That is enough to deal with any potential
attacks where an attacker is trying to collide hashes with what's
already in the repository.

Co-authored-by: Jeff King <peff@peff.net>
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 object-file.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Comments

Junio C Hamano Sept. 25, 2024, 5:02 p.m. UTC | #1 
Taylor Blau <me@ttaylorr.com> writes:

> ... But in practice it is
> expanding the definition of "what is already on disk" to be the point
> that the function is called.

Yeah, it is a reasonable argument for this additional protection.
It does not make things worse.  All it takes is for the attacker to
come a bit earlier to defeat the link/unlink dance, so doing it "the
right way" does not make it fundamentally safer.

I hope all TOCTOU races can be explained away this way ;-).

> Co-authored-by: Jeff King <peff@peff.net>
> Signed-off-by: Jeff King <peff@peff.net>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
>  object-file.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/object-file.c b/object-file.c
> index 968da27cd41..38407f468a9 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -1937,6 +1937,7 @@ static void write_object_file_prepare_literally(const struct git_hash_algo *algo
>   */
>  int finalize_object_file(const char *tmpfile, const char *filename)
>  {
> +	struct stat st;
>  	int ret = 0;
>  
>  	if (object_creation_mode == OBJECT_CREATION_USES_RENAMES)
> @@ -1957,9 +1958,12 @@ int finalize_object_file(const char *tmpfile, const char *filename)
>  	 */
>  	if (ret && ret != EEXIST) {
>  	try_rename:
> -		if (!rename(tmpfile, filename))
> +		if (!stat(filename, &st))
> +			ret = EEXIST;
> +		else if (!rename(tmpfile, filename))
>  			goto out;
> -		ret = errno;
> +		else
> +			ret = errno;
>  	}
>  	unlink_or_warn(tmpfile);
>  	if (ret) {
diff mbox series

Patch

diff --git a/object-file.c b/object-file.c
index 968da27cd41..38407f468a9 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1937,6 +1937,7 @@  static void write_object_file_prepare_literally(const struct git_hash_algo *algo
  */
 int finalize_object_file(const char *tmpfile, const char *filename)
 {
+	struct stat st;
 	int ret = 0;
 
 	if (object_creation_mode == OBJECT_CREATION_USES_RENAMES)
@@ -1957,9 +1958,12 @@  int finalize_object_file(const char *tmpfile, const char *filename)
 	 */
 	if (ret && ret != EEXIST) {
 	try_rename:
-		if (!rename(tmpfile, filename))
+		if (!stat(filename, &st))
+			ret = EEXIST;
+		else if (!rename(tmpfile, filename))
 			goto out;
-		ret = errno;
+		else
+			ret = errno;
 	}
 	unlink_or_warn(tmpfile);
 	if (ret) {