Mailing List Archive

[PATCH 8/12] openssh-4.3p2 return code check bugs
The get_handle function can return a negative value. The variable that
value is assigned to is eventually passed to handle_close which uses the
value as an array index thus not being able to handle negative values.
This patch adds the return code check and provides an appropriate error
exit in the event of a negative return code. This entire set of patches
passed the regression tests on my system. Bugs found by Coverity.

Signed-off-by: Kylene Hall <kjhall@us.ibm.com>
---
sftp-server.c | 27 +++++++++++++++++++++++++--
1 files changed, 25 insertions(+), 2 deletions(-)
diff -uprN openssh-4.3p2/sftp-server.c openssh-4.3p2-kylie/sftp-server.c
--- openssh-4.3p2/sftp-server.c 2006-01-02 06:40:51.000000000 -0600
+++ openssh-4.3p2-kylie/sftp-server.c 2006-05-08 15:42:04.795120304 -0500
@@ -408,9 +408,12 @@ process_close(void)

id = get_int();
handle = get_handle();
+ if (handle < 0)
+ goto out;
TRACE("close id %u handle %d", id, handle);
ret = handle_close(handle);
status = (ret == -1) ? errno_to_portable(errno) : SSH2_FX_OK;
+out:
send_status(id, status);
}

@@ -424,6 +427,8 @@ process_read(void)

id = get_int();
handle = get_handle();
+ if (handle < 0)
+ goto out;
off = get_int64();
len = get_int();

@@ -450,6 +455,7 @@ process_read(void)
}
}
}
+out:
if (status != SSH2_FX_OK)
send_status(id, status);
}
@@ -461,10 +467,12 @@ process_write(void)
u_int64_t off;
u_int len;
int handle, fd, ret, status = SSH2_FX_FAILURE;
- char *data;
+ char *data = NULL;

id = get_int();
handle = get_handle();
+ if (handle < 0)
+ goto out;
off = get_int64();
data = get_string(&len);

@@ -488,8 +496,10 @@ process_write(void)
}
}
}
+out:
send_status(id, status);
- xfree(data);
+ if (data)
+ xfree(data);
}

static void
@@ -539,6 +549,8 @@ process_fstat(void)

id = get_int();
handle = get_handle();
+ if (handle < 0)
+ goto out;
TRACE("fstat id %u handle %d", id, handle);
fd = handle_to_fd(handle);
if (fd >= 0) {
@@ -551,6 +563,7 @@ process_fstat(void)
status = SSH2_FX_OK;
}
}
+out:
if (status != SSH2_FX_OK)
send_status(id, status);
}
@@ -614,6 +627,10 @@ process_fsetstat(void)

id = get_int();
handle = get_handle();
+ if (handle < 0) {
+ status = SSH2_FX_FAILURE;
+ goto out;
+ }
a = get_attrib();
TRACE("fsetstat id %u handle %d", id, handle);
fd = handle_to_fd(handle);
@@ -654,6 +671,7 @@ process_fsetstat(void)
status = errno_to_portable(errno);
}
}
+out:
send_status(id, status);
}

@@ -697,6 +715,11 @@ process_readdir(void)

id = get_int();
handle = get_handle();
+ if (handle < 0) {
+ send_status(id, SSH2_FX_FAILURE);
+ return;
+ }
+
TRACE("readdir id %u handle %d", id, handle);
dirp = handle_to_dir(handle);
path = handle_to_name(handle);


_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
http://www.mindrot.org/mailman/listinfo/openssh-unix-dev
Re: [PATCH 8/12] openssh-4.3p2 return code check bugs [ In reply to ]
On Mon, May 15, 2006 at 03:19:17PM -0500, Kylene Jo Hall wrote:
> The get_handle function can return a negative value. The variable that
> value is assigned to is eventually passed to handle_close which uses the
> value as an array index thus not being able to handle negative values.
> This patch adds the return code check and provides an appropriate error
> exit in the event of a negative return code. This entire set of patches
> passed the regression tests on my system. Bugs found by Coverity.

i'm not sure about this one. we always call handle_is_ok()
after get_handle(), so an error is sent back in any case.

_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
http://www.mindrot.org/mailman/listinfo/openssh-unix-dev
Re: [PATCH 8/12] openssh-4.3p2 return code check bugs [ In reply to ]
On Tue, 2006-05-16 at 16:46 +0200, Markus Friedl wrote:
> On Mon, May 15, 2006 at 03:19:17PM -0500, Kylene Jo Hall wrote:
> > The get_handle function can return a negative value. The variable that
> > value is assigned to is eventually passed to handle_close which uses the
> > value as an array index thus not being able to handle negative values.
> > This patch adds the return code check and provides an appropriate error
> > exit in the event of a negative return code. This entire set of patches
> > passed the regression tests on my system. Bugs found by Coverity.
>
> i'm not sure about this one. we always call handle_is_ok()
> after get_handle(), so an error is sent back in any case.

In the instances in this patch I don't see handle_is_ok being called.
Is it called by some intermediate function that I didn't think to look
in. Maybe adding handle_is_ok is the appropriate fix here instead of
the way I dealt with it.

_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
http://www.mindrot.org/mailman/listinfo/openssh-unix-dev