From 46cfeac8e5e6263f64f03028b98e27c0b09575f9 Mon Sep 17 00:00:00 2001 From: simon Date: Sun, 20 Feb 2005 10:30:05 +0000 Subject: [PATCH] Additional robustness to SFTP packet parsing and memory allocation. git-svn-id: svn://svn.tartarus.org/sgt/putty@5358 cda61777-01e9-0310-a592-d414129be87e --- misc.c | 35 +++++++--- puttymem.h | 22 ++++--- sftp.c | 213 ++++++++++++++++++++++++++++++++++++++++--------------------- 3 files changed, 180 insertions(+), 90 deletions(-) diff --git a/misc.c b/misc.c index 59e91f2f..fea96a97 100644 --- a/misc.c +++ b/misc.c @@ -5,6 +5,7 @@ #include #include #include +#include #include #include #include "putty.h" @@ -388,14 +389,21 @@ void mlog(char *file, int line) } #endif -void *safemalloc(size_t size) +void *safemalloc(size_t n, size_t size) { void *p; + + if (n > INT_MAX / size) { + p = NULL; + } else { + size *= n; #ifdef MINEFIELD - p = minefield_c_malloc(size); + p = minefield_c_malloc(size); #else - p = malloc(size); + p = malloc(size); #endif + } + if (!p) { char str[200]; #ifdef MALLOC_LOG @@ -415,22 +423,29 @@ void *safemalloc(size_t size) return p; } -void *saferealloc(void *ptr, size_t size) +void *saferealloc(void *ptr, size_t n, size_t size) { void *p; - if (!ptr) { + + if (n > INT_MAX / size) { + p = NULL; + } else { + size *= n; + if (!ptr) { #ifdef MINEFIELD - p = minefield_c_malloc(size); + p = minefield_c_malloc(size); #else - p = malloc(size); + p = malloc(size); #endif - } else { + } else { #ifdef MINEFIELD - p = minefield_c_realloc(ptr, size); + p = minefield_c_realloc(ptr, size); #else - p = realloc(ptr, size); + p = realloc(ptr, size); #endif + } } + if (!p) { char str[200]; #ifdef MALLOC_LOG diff --git a/puttymem.h b/puttymem.h index 9fe272fd..b529c985 100644 --- a/puttymem.h +++ b/puttymem.h @@ -11,18 +11,22 @@ /* #define MALLOC_LOG do this if you suspect putty of leaking memory */ #ifdef MALLOC_LOG -#define smalloc(z) (mlog(__FILE__,__LINE__), safemalloc(z)) -#define srealloc(y,z) (mlog(__FILE__,__LINE__), saferealloc(y,z)) +#define smalloc(z) (mlog(__FILE__,__LINE__), safemalloc(z,1)) +#define snmalloc(z,s) (mlog(__FILE__,__LINE__), safemalloc(z,s)) +#define srealloc(y,z) (mlog(__FILE__,__LINE__), saferealloc(y,z,1)) +#define snrealloc(y,z) (mlog(__FILE__,__LINE__), saferealloc(y,z,s)) #define sfree(z) (mlog(__FILE__,__LINE__), safefree(z)) void mlog(char *, int); #else -#define smalloc safemalloc -#define srealloc saferealloc +#define smalloc(z) safemalloc(z,1) +#define snmalloc safemalloc +#define srealloc(y,z) saferealloc(y,z,1) +#define snrealloc saferealloc #define sfree safefree #endif -void *safemalloc(size_t); -void *saferealloc(void *, size_t); +void *safemalloc(size_t, size_t); +void *saferealloc(void *, size_t, size_t); void safefree(void *); /* @@ -31,8 +35,8 @@ void safefree(void *); * you don't mistakenly allocate enough space for one sort of * structure and assign it to a different sort of pointer. */ -#define snew(type) ((type *)smalloc(sizeof(type))) -#define snewn(n, type) ((type *)smalloc((n)*sizeof(type))) -#define sresize(ptr, n, type) ((type *)srealloc(ptr, (n)*sizeof(type))) +#define snew(type) ((type *)snmalloc(1, sizeof(type))) +#define snewn(n, type) ((type *)snmalloc((n), sizeof(type))) +#define sresize(ptr, n, type) ((type *)snrealloc((ptr), (n), sizeof(type))) #endif diff --git a/sftp.c b/sftp.c index a55262a7..4d9810fc 100644 --- a/sftp.c +++ b/sftp.c @@ -27,8 +27,8 @@ struct sftp_packet { char *data; - int length, maxlen; - int savedpos; + unsigned length, maxlen; + unsigned savedpos; int type; }; @@ -139,61 +139,67 @@ static void sftp_pkt_addattrs(struct sftp_packet *pkt, struct fxp_attrs attrs) * SFTP packet decode functions. */ -static unsigned char sftp_pkt_getbyte(struct sftp_packet *pkt) +static int sftp_pkt_getbyte(struct sftp_packet *pkt, unsigned char *ret) { - unsigned char value; if (pkt->length - pkt->savedpos < 1) - return 0; /* arrgh, no way to decline (FIXME?) */ - value = (unsigned char) pkt->data[pkt->savedpos]; + return 0; + *ret = (unsigned char) pkt->data[pkt->savedpos]; pkt->savedpos++; - return value; + return 1; } -static unsigned long sftp_pkt_getuint32(struct sftp_packet *pkt) +static int sftp_pkt_getuint32(struct sftp_packet *pkt, unsigned long *ret) { - unsigned long value; if (pkt->length - pkt->savedpos < 4) - return 0; /* arrgh, no way to decline (FIXME?) */ - value = GET_32BIT(pkt->data + pkt->savedpos); + return 0; + *ret = GET_32BIT(pkt->data + pkt->savedpos); pkt->savedpos += 4; - return value; + return 1; } -static void sftp_pkt_getstring(struct sftp_packet *pkt, - char **p, int *length) +static int sftp_pkt_getstring(struct sftp_packet *pkt, + char **p, int *length) { *p = NULL; if (pkt->length - pkt->savedpos < 4) - return; + return 0; *length = GET_32BIT(pkt->data + pkt->savedpos); pkt->savedpos += 4; - if (pkt->length - pkt->savedpos < *length) - return; + if (pkt->length - pkt->savedpos < *length || *length < 0) { + *length = 0; + return 0; + } *p = pkt->data + pkt->savedpos; pkt->savedpos += *length; + return 1; } -static struct fxp_attrs sftp_pkt_getattrs(struct sftp_packet *pkt) +static int sftp_pkt_getattrs(struct sftp_packet *pkt, struct fxp_attrs *ret) { - struct fxp_attrs ret; - ret.flags = sftp_pkt_getuint32(pkt); - if (ret.flags & SSH_FILEXFER_ATTR_SIZE) { + if (!sftp_pkt_getuint32(pkt, &ret->flags)) + return 0; + if (ret->flags & SSH_FILEXFER_ATTR_SIZE) { unsigned long hi, lo; - hi = sftp_pkt_getuint32(pkt); - lo = sftp_pkt_getuint32(pkt); - ret.size = uint64_make(hi, lo); + if (!sftp_pkt_getuint32(pkt, &hi) || + !sftp_pkt_getuint32(pkt, &lo)) + return 0; + ret->size = uint64_make(hi, lo); } - if (ret.flags & SSH_FILEXFER_ATTR_UIDGID) { - ret.uid = sftp_pkt_getuint32(pkt); - ret.gid = sftp_pkt_getuint32(pkt); + if (ret->flags & SSH_FILEXFER_ATTR_UIDGID) { + if (!sftp_pkt_getuint32(pkt, &ret->uid) || + !sftp_pkt_getuint32(pkt, &ret->gid)) + return 0; } - if (ret.flags & SSH_FILEXFER_ATTR_PERMISSIONS) { - ret.permissions = sftp_pkt_getuint32(pkt); + if (ret->flags & SSH_FILEXFER_ATTR_PERMISSIONS) { + if (!sftp_pkt_getuint32(pkt, &ret->permissions)) + return 0; } - if (ret.flags & SSH_FILEXFER_ATTR_ACMODTIME) { - ret.atime = sftp_pkt_getuint32(pkt); - ret.mtime = sftp_pkt_getuint32(pkt); + if (ret->flags & SSH_FILEXFER_ATTR_ACMODTIME) { + if (!sftp_pkt_getuint32(pkt, &ret->atime) || + !sftp_pkt_getuint32(pkt, &ret->mtime)) + return 0; } - if (ret.flags & SSH_FILEXFER_ATTR_EXTENDED) { - int count; - count = sftp_pkt_getuint32(pkt); + if (ret->flags & SSH_FILEXFER_ATTR_EXTENDED) { + unsigned long count; + if (!sftp_pkt_getuint32(pkt, &count)) + return 0; while (count--) { char *str; int len; @@ -201,11 +207,12 @@ static struct fxp_attrs sftp_pkt_getattrs(struct sftp_packet *pkt) * We should try to analyse these, if we ever find one * we recognise. */ - sftp_pkt_getstring(pkt, &str, &len); - sftp_pkt_getstring(pkt, &str, &len); + if (!sftp_pkt_getstring(pkt, &str, &len) || + !sftp_pkt_getstring(pkt, &str, &len)) + return 0; } } - return ret; + return 1; } static void sftp_pkt_free(struct sftp_packet *pkt) { @@ -230,6 +237,7 @@ struct sftp_packet *sftp_recv(void) { struct sftp_packet *pkt; char x[4]; + unsigned char uc; if (!sftp_recvdata(x, 4)) return NULL; @@ -244,7 +252,12 @@ struct sftp_packet *sftp_recv(void) return NULL; } - pkt->type = sftp_pkt_getbyte(pkt); + if (!sftp_pkt_getbyte(pkt, &uc)) { + sftp_pkt_free(pkt); + return NULL; + } else { + pkt->type = uc; + } return pkt; } @@ -357,7 +370,10 @@ struct sftp_request *sftp_find_request(struct sftp_packet *pktin) return NULL; } - id = sftp_pkt_getuint32(pktin); + if (!sftp_pkt_getuint32(pktin, &id)) { + fxp_internal_error("did not receive a valid SFTP packet\n"); + return NULL; + } req = find234(sftp_requests, &id, sftp_reqfind); if (!req || !req->registered) { @@ -413,12 +429,18 @@ static int fxp_got_status(struct sftp_packet *pktin) fxp_error_message = "expected FXP_STATUS packet"; fxp_errtype = -1; } else { - fxp_errtype = sftp_pkt_getuint32(pktin); - if (fxp_errtype < 0 || - fxp_errtype >= sizeof(messages) / sizeof(*messages)) + unsigned long ul; + if (!sftp_pkt_getuint32(pktin, &ul)) { + fxp_error_message = "malformed FXP_STATUS packet"; + fxp_errtype = -1; + } else { + fxp_errtype = ul; + if (fxp_errtype < 0 || + fxp_errtype >= sizeof(messages) / sizeof(*messages)) fxp_error_message = "unknown error code"; - else - fxp_error_message = messages[fxp_errtype]; + else + fxp_error_message = messages[fxp_errtype]; + } } if (fxp_errtype == SSH_FX_OK) @@ -451,7 +473,7 @@ int fxp_error_type(void) int fxp_init(void) { struct sftp_packet *pktout, *pktin; - int remotever; + unsigned long remotever; pktout = sftp_pkt_init(SSH_FXP_INIT); sftp_pkt_adduint32(pktout, SFTP_PROTO_VERSION); @@ -467,7 +489,11 @@ int fxp_init(void) sftp_pkt_free(pktin); return 0; } - remotever = sftp_pkt_getuint32(pktin); + if (!sftp_pkt_getuint32(pktin, &remotever)) { + fxp_internal_error("malformed FXP_VERSION packet"); + sftp_pkt_free(pktin); + return 0; + } if (remotever > SFTP_PROTO_VERSION) { fxp_internal_error ("remote protocol is more advanced than we support"); @@ -507,18 +533,16 @@ char *fxp_realpath_recv(struct sftp_packet *pktin, struct sftp_request *req) sfree(req); if (pktin->type == SSH_FXP_NAME) { - int count; + unsigned long count; char *path; int len; - count = sftp_pkt_getuint32(pktin); - if (count != 1) { - fxp_internal_error("REALPATH returned name count != 1\n"); + if (!sftp_pkt_getuint32(pktin, &count) || count != 1) { + fxp_internal_error("REALPATH did not return name count of 1\n"); sftp_pkt_free(pktin); return NULL; } - sftp_pkt_getstring(pktin, &path, &len); - if (!path) { + if (!sftp_pkt_getstring(pktin, &path, &len)) { fxp_internal_error("REALPATH returned malformed FXP_NAME\n"); sftp_pkt_free(pktin); return NULL; @@ -561,8 +585,7 @@ struct fxp_handle *fxp_open_recv(struct sftp_packet *pktin, struct fxp_handle *handle; int len; - sftp_pkt_getstring(pktin, &hstring, &len); - if (!hstring) { + if (!sftp_pkt_getstring(pktin, &hstring, &len)) { fxp_internal_error("OPEN returned malformed FXP_HANDLE\n"); sftp_pkt_free(pktin); return NULL; @@ -604,8 +627,7 @@ struct fxp_handle *fxp_opendir_recv(struct sftp_packet *pktin, struct fxp_handle *handle; int len; - sftp_pkt_getstring(pktin, &hstring, &len); - if (!hstring) { + if (!sftp_pkt_getstring(pktin, &hstring, &len)) { fxp_internal_error("OPENDIR returned malformed FXP_HANDLE\n"); sftp_pkt_free(pktin); return NULL; @@ -773,8 +795,12 @@ int fxp_stat_recv(struct sftp_packet *pktin, struct sftp_request *req, { sfree(req); if (pktin->type == SSH_FXP_ATTRS) { - *attrs = sftp_pkt_getattrs(pktin); - sftp_pkt_free(pktin); + if (!sftp_pkt_getattrs(pktin, attrs)) { + fxp_internal_error("malformed SSH_FXP_ATTRS packet"); + sftp_pkt_free(pktin); + return 0; + } + sftp_pkt_free(pktin); return 1; } else { fxp_got_status(pktin); @@ -802,8 +828,12 @@ int fxp_fstat_recv(struct sftp_packet *pktin, struct sftp_request *req, { sfree(req); if (pktin->type == SSH_FXP_ATTRS) { - *attrs = sftp_pkt_getattrs(pktin); - sftp_pkt_free(pktin); + if (!sftp_pkt_getattrs(pktin, attrs)) { + fxp_internal_error("malformed SSH_FXP_ATTRS packet"); + sftp_pkt_free(pktin); + return 0; + } + sftp_pkt_free(pktin); return 1; } else { fxp_got_status(pktin); @@ -900,7 +930,11 @@ int fxp_read_recv(struct sftp_packet *pktin, struct sftp_request *req, char *str; int rlen; - sftp_pkt_getstring(pktin, &str, &rlen); + if (!sftp_pkt_getstring(pktin, &str, &rlen)) { + fxp_internal_error("READ returned malformed SSH_FXP_DATA packet"); + sftp_pkt_free(pktin); + return -1; + } if (rlen > len || rlen < 0) { fxp_internal_error("READ returned more bytes than requested"); @@ -941,18 +975,55 @@ struct fxp_names *fxp_readdir_recv(struct sftp_packet *pktin, sfree(req); if (pktin->type == SSH_FXP_NAME) { struct fxp_names *ret; - int i; + unsigned long i; + + /* + * Sanity-check the number of names. Minimum is obviously + * zero. Maximum is the remaining space in the packet + * divided by the very minimum length of a name, which is + * 12 bytes (4 for an empty filename, 4 for an empty + * longname, 4 for a set of attribute flags indicating that + * no other attributes are supplied). + */ + if (!sftp_pkt_getuint32(pktin, &i) || + i > (pktin->length-pktin->savedpos)/12) { + fxp_internal_error("malformed FXP_NAME packet"); + sftp_pkt_free(pktin); + return NULL; + } + + /* + * Ensure the implicit multiplication in the snewn() call + * doesn't suffer integer overflow and cause us to malloc + * too little space. + */ + if (i > INT_MAX / sizeof(struct fxp_name)) { + fxp_internal_error("unreasonably large FXP_NAME packet"); + sftp_pkt_free(pktin); + return NULL; + } + ret = snew(struct fxp_names); - ret->nnames = sftp_pkt_getuint32(pktin); + ret->nnames = i; ret->names = snewn(ret->nnames, struct fxp_name); for (i = 0; i < ret->nnames; i++) { - char *str; - int len; - sftp_pkt_getstring(pktin, &str, &len); - ret->names[i].filename = mkstr(str, len); - sftp_pkt_getstring(pktin, &str, &len); - ret->names[i].longname = mkstr(str, len); - ret->names[i].attrs = sftp_pkt_getattrs(pktin); + char *str1, *str2; + int len1, len2; + if (!sftp_pkt_getstring(pktin, &str1, &len1) || + !sftp_pkt_getstring(pktin, &str2, &len2) || + !sftp_pkt_getattrs(pktin, &ret->names[i].attrs)) { + fxp_internal_error("malformed FXP_NAME packet"); + while (i--) { + sfree(ret->names[i].filename); + sfree(ret->names[i].longname); + } + sfree(ret->names); + sfree(ret); + sfree(pktin); + return NULL; + } + ret->names[i].filename = mkstr(str1, len1); + ret->names[i].longname = mkstr(str2, len2); } sftp_pkt_free(pktin); return ret; -- 2.11.0