sftp: avoid leaking path arg in calls to make_absolute_pwd_glob

[emaste]

Summary

As Coverity reports:
    Overwriting tmp in tmp = make_absolute_pwd_glob(tmp, remote_path)
    leaks the storage that tmp points to.

Consume the first arg in make_absolute_pwd_glob, and add xstrdup() to
the one case which did not assign to the same variable that was passed
in.

Reported by:    Coverity Scan
CID:            1500409
Sponsored by:   The FreeBSD Foundation
Differential Revision:

[markj]

crypto/openssh/sftp.c

624

The const qualifier on *p has to be dropped too.

[emaste]

update from @markj

[emaste]

rebase after 9.3p1 update, which included a fix for this same issue reported by upstream

commit 36c6c3eff5e4a669ff414b9daf85f919666e8e03
Author: [e-mail] <[e-mail]>
Date:   Wed Mar 8 06:21:32 2023 +0000

    upstream: Plug mem leak. Coverity CID 405196, ok djm@
    
    OpenBSD-Commit-ID: 175f09349387c292f626da68f65f334faaa085f2

diff --git a/sftp.c b/sftp.c
index 3a2525462..4a3774421 100644
--- a/sftp.c
+++ b/sftp.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: sftp.c,v 1.227 2023/03/08 04:43:12 guenther Exp $ */
+/* $OpenBSD: sftp.c,v 1.228 2023/03/08 06:21:32 dtucker Exp $ */
 /*
  * Copyright (c) 2001-2004 Damien Miller <[e-mail]>
  *
@@ -1997,7 +1997,9 @@ complete_match(EditLine *el, struct sftp_conn *conn, char *remote_path,
 
        memset(&g, 0, sizeof(g));
        if (remote != LOCAL) {
-               tmp = make_absolute_pwd_glob(tmp, remote_path);
+               tmp2 = make_absolute_pwd_glob(tmp, remote_path);
+               free(tmp);
+               tmp = tmp2;
                remote_glob(conn, tmp, GLOB_DOOFFS|GLOB_MARK, NULL, &g);
        } else
                glob(tmp, GLOB_DOOFFS|GLOB_MARK, NULL, &g);

[emaste]

Note that this patch was proposed upstream at https://lists.mindrot.org/pipermail/openssh-unix-dev/2022-November/040497.html but received no response.
I followed up with a link to the updated patch here, in https://lists.mindrot.org/pipermail/openssh-unix-dev/2023-March/040655.html

[markj]

So that upstream commit fixed only one of the leaks, right?

This patch looks good. It gives make_absolute() and make_absolute_pwd_glob() have the same semantics wrt freeing the input string.

[emaste]

So that upstream commit fixed only one of the leaks, right?

Yes - in a FreeBSD run Coverity reported only a single instance of the leak, but I fixed the general issue. I suspect Coverity gave the same report to OpenBSD and they fixed only the single instance.