nhyatt/putty-source
1
0
Fork 0
mirror of https://git.tartarus.org/simon/putty.git synced 2025-01-09 09:27:59 +00:00
Code Issues Projects Releases Wiki Activity

Tighten up pointer handling after ssh_pkt_getstring.

Browse Source 
ssh_pkt_getstring can return (NULL,0) if the input packet is too short
to contain a valid string.

In quite a few places we were passing the returned pointer,length pair
to a printf function with "%.*s" type format, which seems in practice
to have not been dereferencing the pointer but the C standard doesn't
actually guarantee that. In one place we were doing the same job by
hand with memcpy, and apparently that _can_ dereference the pointer in
practice (so a server could have caused a NULL-dereference crash by
sending an appropriately malformed "x11" type channel open request).
And also I spotted a logging call in the "forwarded-tcpip" channel
open handler which had forgotten the field width completely, so it was
erroneously relying on the string happening to be NUL-terminated in
the received packet.

I've tightened all of this up in general by normalising (NULL,0) to
("",0) before calling printf("%.*s"), and replacing the two even more
broken cases with the corrected version of that same idiom.
This commit is contained in:
Simon Tatham 2016-02-29 19:38:12 +00:00
parent bc6c15ab5f
commit b49a8db1b4
2 changed files with 19 additions and 13 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
misc.h
View File

@ -169,4 +169,9 @@ void debug_memdump(void *buf, int len, int L);
(cp)[0] = (unsigned char)((value) >> 8), \
(cp)[1] = (unsigned char)(value) )
/* Replace NULL with the empty string, permitting an idiom in which we
* get a string (pointer,length) pair that might be NULL,0 and can
* then safely say things like printf("%.*s", length, NULLTOEMPTY(ptr)) */
#define NULLTOEMPTY(s) ((s)?(s):"")
#endif

27
ssh.c
View File

@ -5419,7 +5419,7 @@ static void ssh1_msg_port_open(Ssh ssh, struct Packet *pktin)
ssh_pkt_getstring(pktin, &host, &hostsize);
port = ssh_pkt_getuint32(pktin);
pf.dhost = dupprintf("%.*s", hostsize, host);
pf.dhost = dupprintf("%.*s", hostsize, NULLTOEMPTY(host));
pf.dport = port;
pfp = find234(ssh->rportfwds, &pf, NULL);
@ -5902,7 +5902,7 @@ static void ssh1_msg_debug(Ssh ssh, struct Packet *pktin)
int msglen;
ssh_pkt_getstring(pktin, &msg, &msglen);
logeventf(ssh, "Remote debug message: %.*s", msglen, msg);
logeventf(ssh, "Remote debug message: %.*s", msglen, NULLTOEMPTY(msg));
}
static void ssh1_msg_disconnect(Ssh ssh, struct Packet *pktin)
@ -5912,7 +5912,8 @@ static void ssh1_msg_disconnect(Ssh ssh, struct Packet *pktin)
int msglen;
ssh_pkt_getstring(pktin, &msg, &msglen);
bombout(("Server sent disconnect message:\n\"%.*s\"", msglen, msg));
bombout(("Server sent disconnect message:\n\"%.*s\"",
msglen, NULLTOEMPTY(msg)));
}
static void ssh_msg_ignore(Ssh ssh, struct Packet *pktin)
@ -7960,7 +7961,8 @@ static void ssh2_msg_channel_open_failure(Ssh ssh, struct Packet *pktin)
reason_code = 0; /* ensure reasons[reason_code] in range */
ssh_pkt_getstring(pktin, &reason_string, &reason_length);
logeventf(ssh, "Forwarded connection refused by server: %s [%.*s]",
reasons[reason_code], reason_length, reason_string);
reasons[reason_code], reason_length,
NULLTOEMPTY(reason_string));
pfd_close(c->u.pfd.pf);
} else if (c->type == CHAN_ZOMBIE) {
@ -8255,9 +8257,7 @@ static void ssh2_msg_channel_open(Ssh ssh, struct Packet *pktin)
char *addrstr;
ssh_pkt_getstring(pktin, &peeraddr, &peeraddrlen);
addrstr = snewn(peeraddrlen+1, char);
memcpy(addrstr, peeraddr, peeraddrlen);
addrstr[peeraddrlen] = '\0';
addrstr = dupprintf("%.*s", peeraddrlen, NULLTOEMPTY(peeraddr));
peerport = ssh_pkt_getuint32(pktin);
logeventf(ssh, "Received X11 connect request from %s:%d",
@ -8292,13 +8292,14 @@ static void ssh2_msg_channel_open(Ssh ssh, struct Packet *pktin)
char *shost;
int shostlen;
ssh_pkt_getstring(pktin, &shost, &shostlen);/* skip address */
pf.shost = dupprintf("%.*s", shostlen, shost);
pf.shost = dupprintf("%.*s", shostlen, NULLTOEMPTY(shost));
pf.sport = ssh_pkt_getuint32(pktin);
ssh_pkt_getstring(pktin, &peeraddr, &peeraddrlen);
peerport = ssh_pkt_getuint32(pktin);
realpf = find234(ssh->rportfwds, &pf, NULL);
logeventf(ssh, "Received remote port %s:%d open request "
"from %s:%d", pf.shost, pf.sport, peeraddr, peerport);
"from %.*s:%d", pf.shost, pf.sport,
peeraddrlen, NULLTOEMPTY(peeraddr), peerport);
sfree(pf.shost);
if (realpf == NULL) {
@ -9957,7 +9958,7 @@ static void do_ssh2_authconn(Ssh ssh, unsigned char *in, int inlen,
s->cur_prompt->to_server = TRUE;
s->cur_prompt->name = dupstr("New SSH password");
s->cur_prompt->instruction =
dupprintf("%.*s", prompt_len, prompt);
dupprintf("%.*s", prompt_len, NULLTOEMPTY(prompt));
s->cur_prompt->instr_reqd = TRUE;
/*
* There's no explicit requirement in the protocol
@ -10394,13 +10395,13 @@ static void ssh2_msg_disconnect(Ssh ssh, struct Packet *pktin)
logevent(buf);
sfree(buf);
buf = dupprintf("Disconnection message text: %.*s",
msglen, msg);
msglen, NULLTOEMPTY(msg));
logevent(buf);
bombout(("Server sent disconnect message\ntype %d (%s):\n\"%.*s\"",
reason,
(reason > 0 && reason < lenof(ssh2_disconnect_reasons)) ?
ssh2_disconnect_reasons[reason] : "unknown",
msglen, msg));
msglen, NULLTOEMPTY(msg)));
sfree(buf);
}
@ -10414,7 +10415,7 @@ static void ssh2_msg_debug(Ssh ssh, struct Packet *pktin)
ssh2_pkt_getbool(pktin);
ssh_pkt_getstring(pktin, &msg, &msglen);
logeventf(ssh, "Remote debug message: %.*s", msglen, msg);
logeventf(ssh, "Remote debug message: %.*s", msglen, NULLTOEMPTY(msg));
}
static void ssh2_msg_transport(Ssh ssh, struct Packet *pktin)