From 1a009ab2e9d9f0baa5ff98d295d4ec7afd9ff2f9 Mon Sep 17 00:00:00 2001 From: Ben Harris Date: Tue, 6 Oct 2015 11:02:52 +0100 Subject: [PATCH 01/22] Fuzzable terminal emulator. --- Recipe | 3 ++ fuzzterm.c | 156 +++++++++++++++++++++++++++++++++++++++++++++++++++++ terminal.c | 2 +- 3 files changed, 160 insertions(+), 1 deletion(-) create mode 100644 fuzzterm.c diff --git a/Recipe b/Recipe index 89609df6..bba63a9c 100644 --- a/Recipe +++ b/Recipe @@ -310,3 +310,6 @@ pageant : [X] uxpgnt uxagentc pageant sshrsa sshpubk sshdes sshbn sshmd5 PuTTY : [MX] osxmain OSXTERM OSXMISC CHARSET U_BE_ALL NONSSH UXSSH + ux_x11 uxpty uxsignal testback putty.icns info.plist + +fuzzterm : [U] UXTERM CHARSET misc uxmisc uxucs fuzzterm time settings + + uxstore be_none diff --git a/fuzzterm.c b/fuzzterm.c new file mode 100644 index 00000000..f1dcb1ab --- /dev/null +++ b/fuzzterm.c @@ -0,0 +1,156 @@ +#include +#include +#include + +#define PUTTY_DO_GLOBALS +#include "putty.h" +#include "terminal.h" + +int main(int argc, char **argv) +{ + char blk[512]; + size_t len; + Terminal *term; + Conf *conf; + struct unicode_data ucsdata; + + conf = conf_new(); + do_defaults(NULL, conf); + init_ucs(&ucsdata, conf_get_str(conf, CONF_line_codepage), + conf_get_int(conf, CONF_utf8_override), + CS_NONE, conf_get_int(conf, CONF_vtmode)); + + term = term_init(conf, &ucsdata, NULL); + term_size(term, 24, 80, 10000); + term->ldisc = NULL; + while (!feof(stdin)) { + len = fread(blk, 1, sizeof(blk), stdin); + term_data(term, 0, blk, len); + } + return 0; +} + +int from_backend(void *frontend, int is_stderr, const char *data, int len) +{ return 0; } + +/* functions required by terminal.c */ + +void request_resize(void *frontend, int x, int y) { } +void do_text(Context a, int b, int c, wchar_t * d, int e, unsigned long f, int g) { } +void do_cursor(Context a, int b, int c, wchar_t * d, int e, unsigned long f, int g) { } +int char_width(Context ctx, int uc) { return 1; } +void set_title(void *frontend, char *t) { } +void set_icon(void *frontend, char *t) { } +void set_sbar(void *frontend, int a, int b, int c) { } + +void ldisc_send(void *handle, const char *buf, int len, int interactive) {} +void ldisc_echoedit_update(void *handle) {} +Context get_ctx(void *frontend) { return NULL; } +void free_ctx(Context ctx) { } +void palette_set(void *frontend, int a, int b, int c, int d) { } +void palette_reset(void *frontend) { } +void write_clip(void *frontend, wchar_t *a, int *b, int c, int d) { } +void get_clip(void *frontend, wchar_t **w, int *i) { } +void set_raw_mouse_mode(void *frontend, int m) { } +void request_paste(void *frontend) { } +void do_beep(void *frontend, int a) { } +void sys_cursor(void *frontend, int x, int y) { } +void fatalbox(const char *fmt, ...) { exit(0); } +void modalfatalbox(const char *fmt, ...) { exit(0); } +void nonfatal(const char *fmt, ...) { } + +void set_iconic(void *frontend, int iconic) { } +void move_window(void *frontend, int x, int y) { } +void set_zorder(void *frontend, int top) { } +void refresh_window(void *frontend) { } +void set_zoomed(void *frontend, int zoomed) { } +int is_iconic(void *frontend) { return 0; } +void get_window_pos(void *frontend, int *x, int *y) { *x = 0; *y = 0; } +void get_window_pixels(void *frontend, int *x, int *y) { *x = 0; *y = 0; } +char *get_window_title(void *frontend, int icon) { return "moo"; } + +/* needed by timing.c */ +void timer_change_notify(unsigned long next) { } + +/* needed by config.c and sercfg.c */ + +void dlg_radiobutton_set(union control *ctrl, void *dlg, int whichbutton) { } +int dlg_radiobutton_get(union control *ctrl, void *dlg) { return 0; } +void dlg_checkbox_set(union control *ctrl, void *dlg, int checked) { } +int dlg_checkbox_get(union control *ctrl, void *dlg) { return 0; } +void dlg_editbox_set(union control *ctrl, void *dlg, char const *text) { } +char *dlg_editbox_get(union control *ctrl, void *dlg) { return dupstr("moo"); } +void dlg_listbox_clear(union control *ctrl, void *dlg) { } +void dlg_listbox_del(union control *ctrl, void *dlg, int index) { } +void dlg_listbox_add(union control *ctrl, void *dlg, char const *text) { } +void dlg_listbox_addwithid(union control *ctrl, void *dlg, + char const *text, int id) { } +int dlg_listbox_getid(union control *ctrl, void *dlg, int index) { return 0; } +int dlg_listbox_index(union control *ctrl, void *dlg) { return -1; } +int dlg_listbox_issel(union control *ctrl, void *dlg, int index) { return 0; } +void dlg_listbox_select(union control *ctrl, void *dlg, int index) { } +void dlg_text_set(union control *ctrl, void *dlg, char const *text) { } +void dlg_filesel_set(union control *ctrl, void *dlg, Filename *fn) { } +Filename *dlg_filesel_get(union control *ctrl, void *dlg) { return NULL; } +void dlg_fontsel_set(union control *ctrl, void *dlg, FontSpec *fn) { } +FontSpec *dlg_fontsel_get(union control *ctrl, void *dlg) { return NULL; } +void dlg_update_start(union control *ctrl, void *dlg) { } +void dlg_update_done(union control *ctrl, void *dlg) { } +void dlg_set_focus(union control *ctrl, void *dlg) { } +void dlg_label_change(union control *ctrl, void *dlg, char const *text) { } +union control *dlg_last_focused(union control *ctrl, void *dlg) { return NULL; } +void dlg_beep(void *dlg) { } +void dlg_error_msg(void *dlg, const char *msg) { } +void dlg_end(void *dlg, int value) { } +void dlg_coloursel_start(union control *ctrl, void *dlg, + int r, int g, int b) { } +int dlg_coloursel_results(union control *ctrl, void *dlg, + int *r, int *g, int *b) { return 0; } +void dlg_refresh(union control *ctrl, void *dlg) { } + +/* miscellany */ +void logevent(void *frontend, const char *msg) { } +int askappend(void *frontend, Filename *filename, + void (*callback)(void *ctx, int result), void *ctx) { return 0; } + +const char *const appname = "FuZZterm"; +const int ngsslibs = 0; +const char *const gsslibnames[0] = { }; +const struct keyvalwhere gsslibkeywords[0] = { }; + +/* + * Default settings that are specific to Unix plink. + */ +char *platform_default_s(const char *name) +{ + if (!strcmp(name, "TermType")) + return dupstr(getenv("TERM")); + if (!strcmp(name, "SerialLine")) + return dupstr("/dev/ttyS0"); + return NULL; +} + +int platform_default_i(const char *name, int def) +{ + return def; +} + +FontSpec *platform_default_fontspec(const char *name) +{ + return fontspec_new(""); +} + +Filename *platform_default_filename(const char *name) +{ + if (!strcmp(name, "LogFileName")) + return filename_from_str("putty.log"); + else + return filename_from_str(""); +} + +char *x_get_default(const char *key) +{ + return NULL; /* this is a stub */ +} + + diff --git a/terminal.c b/terminal.c index ca47c833..8e5c07bf 100644 --- a/terminal.c +++ b/terminal.c @@ -4688,7 +4688,7 @@ static void term_out(Terminal *term) } term_print_flush(term); - if (term->logflush) + if (term->logflush && term->logctx) logflush(term->logctx); } From e3fe709a8f6a633647088e9ed7264be5fb740426 Mon Sep 17 00:00:00 2001 From: Ben Harris Date: Wed, 7 Oct 2015 23:54:39 +0100 Subject: [PATCH 02/22] More robust control sequence parameter handling. Parameters are now accumulated in unsigned integers and carefully checked for overflow (which is turned into saturation). Things that consume them now have explicit range checks (again, saturating) to ensure that their inputs are sane. This should make it much harder to cause overflow by supplying ludicrously large numbers. Fixes two bugs found with the help of afl-fuzz. One of them may be exploitable and is CVE-2015-5309. --- terminal.c | 49 +++++++++++++++++++++++++++++++++++++++++++------ terminal.h | 2 +- 2 files changed, 44 insertions(+), 7 deletions(-) diff --git a/terminal.c b/terminal.c index 8e5c07bf..7aafb58b 100644 --- a/terminal.c +++ b/terminal.c @@ -5,6 +5,7 @@ #include #include #include +#include #include #include @@ -3493,8 +3494,15 @@ static void term_out(Terminal *term) if (term->esc_nargs <= ARGS_MAX) { if (term->esc_args[term->esc_nargs - 1] == ARG_DEFAULT) term->esc_args[term->esc_nargs - 1] = 0; - term->esc_args[term->esc_nargs - 1] = - 10 * term->esc_args[term->esc_nargs - 1] + c - '0'; + if (term->esc_args[term->esc_nargs - 1] <= + UINT_MAX / 10 && + term->esc_args[term->esc_nargs - 1] * 10 <= + UINT_MAX - c - '0') + term->esc_args[term->esc_nargs - 1] = + 10 * term->esc_args[term->esc_nargs - 1] + + c - '0'; + else + term->esc_args[term->esc_nargs - 1] = UINT_MAX; } term->termstate = SEEN_CSI; } else if (c == ';') { @@ -3510,8 +3518,10 @@ static void term_out(Terminal *term) term->esc_query = c; term->termstate = SEEN_CSI; } else +#define CLAMP(arg, lim) ((arg) = ((arg) > (lim)) ? (lim) : (arg)) switch (ANSI(c, term->esc_query)) { case 'A': /* CUU: move up N lines */ + CLAMP(term->esc_args[0], term->rows); move(term, term->curs.x, term->curs.y - def(term->esc_args[0], 1), 1); seen_disp_event(term); @@ -3520,6 +3530,7 @@ static void term_out(Terminal *term) compatibility(ANSI); /* FALLTHROUGH */ case 'B': /* CUD: Cursor down */ + CLAMP(term->esc_args[0], term->rows); move(term, term->curs.x, term->curs.y + def(term->esc_args[0], 1), 1); seen_disp_event(term); @@ -3535,23 +3546,27 @@ static void term_out(Terminal *term) compatibility(ANSI); /* FALLTHROUGH */ case 'C': /* CUF: Cursor right */ + CLAMP(term->esc_args[0], term->cols); move(term, term->curs.x + def(term->esc_args[0], 1), term->curs.y, 1); seen_disp_event(term); break; case 'D': /* CUB: move left N cols */ + CLAMP(term->esc_args[0], term->cols); move(term, term->curs.x - def(term->esc_args[0], 1), term->curs.y, 1); seen_disp_event(term); break; case 'E': /* CNL: move down N lines and CR */ compatibility(ANSI); + CLAMP(term->esc_args[0], term->rows); move(term, 0, term->curs.y + def(term->esc_args[0], 1), 1); seen_disp_event(term); break; case 'F': /* CPL: move up N lines and CR */ compatibility(ANSI); + CLAMP(term->esc_args[0], term->rows); move(term, 0, term->curs.y - def(term->esc_args[0], 1), 1); seen_disp_event(term); @@ -3559,12 +3574,14 @@ static void term_out(Terminal *term) case 'G': /* CHA */ case '`': /* HPA: set horizontal posn */ compatibility(ANSI); + CLAMP(term->esc_args[0], term->cols); move(term, def(term->esc_args[0], 1) - 1, term->curs.y, 0); seen_disp_event(term); break; case 'd': /* VPA: set vertical posn */ compatibility(ANSI); + CLAMP(term->esc_args[0], term->rows); move(term, term->curs.x, ((term->dec_om ? term->marg_t : 0) + def(term->esc_args[0], 1) - 1), @@ -3575,6 +3592,8 @@ static void term_out(Terminal *term) case 'f': /* HVP: set horz and vert posns at once */ if (term->esc_nargs < 2) term->esc_args[1] = ARG_DEFAULT; + CLAMP(term->esc_args[0], term->rows); + CLAMP(term->esc_args[1], term->cols); move(term, def(term->esc_args[1], 1) - 1, ((term->dec_om ? term->marg_t : 0) + def(term->esc_args[0], 1) - 1), @@ -3610,6 +3629,7 @@ static void term_out(Terminal *term) break; case 'L': /* IL: insert lines */ compatibility(VT102); + CLAMP(term->esc_args[0], term->rows); if (term->curs.y <= term->marg_b) scroll(term, term->curs.y, term->marg_b, -def(term->esc_args[0], 1), FALSE); @@ -3617,6 +3637,7 @@ static void term_out(Terminal *term) break; case 'M': /* DL: delete lines */ compatibility(VT102); + CLAMP(term->esc_args[0], term->rows); if (term->curs.y <= term->marg_b) scroll(term, term->curs.y, term->marg_b, def(term->esc_args[0], 1), @@ -3626,11 +3647,13 @@ static void term_out(Terminal *term) case '@': /* ICH: insert chars */ /* XXX VTTEST says this is vt220, vt510 manual says vt102 */ compatibility(VT102); + CLAMP(term->esc_args[0], term->cols); insch(term, def(term->esc_args[0], 1)); seen_disp_event(term); break; case 'P': /* DCH: delete chars */ compatibility(VT102); + CLAMP(term->esc_args[0], term->cols); insch(term, -def(term->esc_args[0], 1)); seen_disp_event(term); break; @@ -3708,6 +3731,8 @@ static void term_out(Terminal *term) compatibility(VT100); if (term->esc_nargs <= 2) { int top, bot; + CLAMP(term->esc_args[0], term->rows); + CLAMP(term->esc_args[1], term->rows); top = def(term->esc_args[0], 1) - 1; bot = (term->esc_nargs <= 1 || term->esc_args[1] == 0 ? @@ -4063,6 +4088,7 @@ static void term_out(Terminal *term) } break; case 'S': /* SU: Scroll up */ + CLAMP(term->esc_args[0], term->rows); compatibility(SCOANSI); scroll(term, term->marg_t, term->marg_b, def(term->esc_args[0], 1), TRUE); @@ -4070,6 +4096,7 @@ static void term_out(Terminal *term) seen_disp_event(term); break; case 'T': /* SD: Scroll down */ + CLAMP(term->esc_args[0], term->rows); compatibility(SCOANSI); scroll(term, term->marg_t, term->marg_b, -def(term->esc_args[0], 1), TRUE); @@ -4112,6 +4139,7 @@ static void term_out(Terminal *term) /* XXX VTTEST says this is vt220, vt510 manual * says vt100 */ compatibility(ANSIMIN); + CLAMP(term->esc_args[0], term->cols); { int n = def(term->esc_args[0], 1); pos cursplus; @@ -4145,6 +4173,7 @@ static void term_out(Terminal *term) break; case 'Z': /* CBT */ compatibility(OTHER); + CLAMP(term->esc_args[0], term->cols); { int i = def(term->esc_args[0], 1); pos old_curs = term->curs; @@ -4205,7 +4234,7 @@ static void term_out(Terminal *term) break; case ANSI('F', '='): /* set normal foreground */ compatibility(SCOANSI); - if (term->esc_args[0] >= 0 && term->esc_args[0] < 16) { + if (term->esc_args[0] < 16) { long colour = (sco2ansicolour[term->esc_args[0] & 0x7] | (term->esc_args[0] & 0x8)) << @@ -4219,7 +4248,7 @@ static void term_out(Terminal *term) break; case ANSI('G', '='): /* set normal background */ compatibility(SCOANSI); - if (term->esc_args[0] >= 0 && term->esc_args[0] < 16) { + if (term->esc_args[0] < 16) { long colour = (sco2ansicolour[term->esc_args[0] & 0x7] | (term->esc_args[0] & 0x8)) << @@ -4343,7 +4372,11 @@ static void term_out(Terminal *term) case '7': case '8': case '9': - term->esc_args[0] = 10 * term->esc_args[0] + c - '0'; + if (term->esc_args[0] <= UINT_MAX / 10 && + term->esc_args[0] * 10 <= UINT_MAX - c - '0') + term->esc_args[0] = 10 * term->esc_args[0] + c - '0'; + else + term->esc_args[0] = UINT_MAX; break; case 'L': /* @@ -4425,7 +4458,11 @@ static void term_out(Terminal *term) case '7': case '8': case '9': - term->esc_args[0] = 10 * term->esc_args[0] + c - '0'; + if (term->esc_args[0] <= UINT_MAX / 10 && + term->esc_args[0] * 10 <= UINT_MAX - c - '0') + term->esc_args[0] = 10 * term->esc_args[0] + c - '0'; + else + term->esc_args[0] = UINT_MAX; break; default: term->termstate = OSC_STRING; diff --git a/terminal.h b/terminal.h index 135ef45a..01d5f57a 100644 --- a/terminal.h +++ b/terminal.h @@ -172,7 +172,7 @@ struct terminal_tag { #define ARGS_MAX 32 /* max # of esc sequence arguments */ #define ARG_DEFAULT 0 /* if an arg isn't specified */ #define def(a,d) ( (a) == ARG_DEFAULT ? (d) : (a) ) - int esc_args[ARGS_MAX]; + unsigned esc_args[ARGS_MAX]; int esc_nargs; int esc_query; #define ANSI(x,y) ((x)+((y)<<8)) From f69b371bcd476a084639cddfb5cddfd3765413bc Mon Sep 17 00:00:00 2001 From: Ben Harris Date: Sat, 10 Oct 2015 00:11:15 +0100 Subject: [PATCH 03/22] ecdsa_newkey: fix a crash where the second curve name is missing or corrupt. Bug found with the help of afl-fuzz. --- sshecc.c | 1 + 1 file changed, 1 insertion(+) diff --git a/sshecc.c b/sshecc.c index 5f170215..9f188df8 100644 --- a/sshecc.c +++ b/sshecc.c @@ -1770,6 +1770,7 @@ static void *ecdsa_newkey(const struct ssh_signkey *self, /* Curve name is duplicated for Weierstrass form */ if (curve->type == EC_WEIERSTRASS) { getstring(&data, &len, &p, &slen); + if (!p) return NULL; if (!match_ssh_id(slen, p, curve->name)) return NULL; } From 63b47ed9d504b37ac2e903715ae7bf40036473a1 Mon Sep 17 00:00:00 2001 From: Ben Harris Date: Sat, 10 Oct 2015 00:20:51 +0100 Subject: [PATCH 04/22] Another ecdsa_newkey crash: initialise ec->privateKey earlier. This one might be exploitable, since without the fix, ecdsa_freekey() tries to wipe the bignum pointed to by an uninitialised pointer. Bug found with the help of afl-fuzz. --- sshecc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sshecc.c b/sshecc.c index 9f188df8..bc842d0b 100644 --- a/sshecc.c +++ b/sshecc.c @@ -1782,11 +1782,11 @@ static void *ecdsa_newkey(const struct ssh_signkey *self, ec->publicKey.x = NULL; ec->publicKey.y = NULL; ec->publicKey.z = NULL; + ec->privateKey = NULL; if (!getmppoint(&data, &len, &ec->publicKey)) { ecdsa_freekey(ec); return NULL; } - ec->privateKey = NULL; if (!ec->publicKey.x || !ec->publicKey.y || bignum_cmp(ec->publicKey.x, curve->p) >= 0 || From 7707aa24d672de5d03ae7e66fbc2fd525dc9a24d Mon Sep 17 00:00:00 2001 From: Ben Harris Date: Sat, 10 Oct 2015 00:58:11 +0100 Subject: [PATCH 05/22] rsa2_pubkey_bits: Cope correctly with a NULL return from rsa2_newkey() Dereferencing it is not correct. Bug found with the help of afl-fuzz. --- sshrsa.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/sshrsa.c b/sshrsa.c index 850204c7..e565a64a 100644 --- a/sshrsa.c +++ b/sshrsa.c @@ -773,6 +773,8 @@ static int rsa2_pubkey_bits(const struct ssh_signkey *self, int ret; rsa = rsa2_newkey(self, (const char *) blob, len); + if (!rsa) + return -1; ret = bignum_bitcount(rsa->modulus); rsa2_freekey(rsa); From c0e19ca19d5be1fea5bc6f75bc18c0e2c4462b64 Mon Sep 17 00:00:00 2001 From: Ben Harris Date: Sat, 10 Oct 2015 22:59:38 +0100 Subject: [PATCH 06/22] In get_ssh_string, don't get confused by lengths >= 0x80000000. "confused" meaning "reading off the end of the input". Bug found with the help of afl-fuzz. --- misc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/misc.c b/misc.c index b1bfb361..6af441c3 100644 --- a/misc.c +++ b/misc.c @@ -1064,7 +1064,7 @@ int match_ssh_id(int stringlen, const void *string, const char *id) void *get_ssh_string(int *datalen, const void **data, int *stringlen) { void *ret; - int len; + unsigned int len; if (*datalen < 4) return NULL; From 4f340599029715d863b84bdfc0407f582114a23c Mon Sep 17 00:00:00 2001 From: Ben Harris Date: Sun, 11 Oct 2015 09:27:55 +0100 Subject: [PATCH 07/22] bignum_set_bit: Don't abort if asked to clear an inaccessible bit All those bits are clear anyway. Bug found with the help of afl-fuzz. --- sshbn.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/sshbn.c b/sshbn.c index fd9e5c0a..3afea467 100644 --- a/sshbn.c +++ b/sshbn.c @@ -1311,9 +1311,9 @@ int bignum_bit(Bignum bn, int i) */ void bignum_set_bit(Bignum bn, int bitnum, int value) { - if (bitnum < 0 || bitnum >= (int)(BIGNUM_INT_BITS * bn[0])) - abort(); /* beyond the end */ - else { + if (bitnum < 0 || bitnum >= (int)(BIGNUM_INT_BITS * bn[0])) { + if (value) abort(); /* beyond the end */ + } else { int v = bitnum / BIGNUM_INT_BITS + 1; BignumInt mask = (BignumInt)1 << (bitnum % BIGNUM_INT_BITS); if (value) From 19d1ad3887feb53e7b03e46c011322353f939eb7 Mon Sep 17 00:00:00 2001 From: Ben Harris Date: Sun, 11 Oct 2015 09:49:38 +0100 Subject: [PATCH 08/22] fuzzterm: Try enabling deferred implementation under afl-clang-fast --- fuzzterm.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/fuzzterm.c b/fuzzterm.c index f1dcb1ab..a4755d8f 100644 --- a/fuzzterm.c +++ b/fuzzterm.c @@ -23,6 +23,10 @@ int main(int argc, char **argv) term = term_init(conf, &ucsdata, NULL); term_size(term, 24, 80, 10000); term->ldisc = NULL; + /* Tell american fuzzy lop that this is a good place to fork. */ +#ifdef __AFL_HAVE_MANUAL_CONTROL + __AFL_INIT(); +#endif while (!feof(stdin)) { len = fread(blk, 1, sizeof(blk), stdin); term_data(term, 0, blk, len); From b94a076955cba4de8e9ad495a35abfd5506733b6 Mon Sep 17 00:00:00 2001 From: Ben Harris Date: Mon, 12 Oct 2015 23:25:16 +0100 Subject: [PATCH 09/22] Since we have bn_restore_invariant, we may as well use it more. --- sshbn.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/sshbn.c b/sshbn.c index 3afea467..7aa10697 100644 --- a/sshbn.c +++ b/sshbn.c @@ -1142,8 +1142,7 @@ Bignum bignum_from_bytes(const unsigned char *data, int nbytes) (BignumInt)byte << (8*i % BIGNUM_INT_BITS); } - while (result[0] > 1 && result[result[0]] == 0) - result[0]--; + bn_restore_invariant(result); return result; } @@ -1165,8 +1164,7 @@ Bignum bignum_from_bytes_le(const unsigned char *data, int nbytes) (BignumInt)byte << (8*i % BIGNUM_INT_BITS); } - while (result[0] > 1 && result[result[0]] == 0) - result[0]--; + bn_restore_invariant(result); return result; } From 0629f1dfa53fe63bce41eaefd9358ea8c7227eeb Mon Sep 17 00:00:00 2001 From: Ben Harris Date: Mon, 12 Oct 2015 23:43:49 +0100 Subject: [PATCH 10/22] Fix an assertion failure when loading Ed25519 keys. "amax == 0 || a[amax] != 0" Essentially, when decodepoint_ed() clears the top bit of the key, it needs to call bn_restore_invariant() in case that left the high-order word zero. Bug found with the help of afl-fuzz. --- sshecc.c | 1 + 1 file changed, 1 insertion(+) diff --git a/sshecc.c b/sshecc.c index bc842d0b..541dd63c 100644 --- a/sshecc.c +++ b/sshecc.c @@ -1648,6 +1648,7 @@ static int decodepoint_ed(const char *p, int length, struct ec_point *point) /* Read x bit and then reset it */ negative = bignum_bit(point->y, point->curve->fieldBits - 1); bignum_set_bit(point->y, point->curve->fieldBits - 1, 0); + bn_restore_invariant(point->y); /* Get the x from the y */ point->x = ecp_edx(point->curve, point->y); From 5171dcb98226332ba27c4d037367100b8564997b Mon Sep 17 00:00:00 2001 From: Ben Harris Date: Tue, 13 Oct 2015 20:33:12 +0100 Subject: [PATCH 11/22] Check the x argument to check_boundary() more carefully. This is a minimal fix for CVE-2015-5309, and while it's probably unnecessary now, it seems worth committing for defence in depth and to give downstreams something reasonably non-intrusive to cherry-pick. --- terminal.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/terminal.c b/terminal.c index 7aafb58b..26a3f0c9 100644 --- a/terminal.c +++ b/terminal.c @@ -2344,7 +2344,7 @@ static void check_boundary(Terminal *term, int x, int y) termline *ldata; /* Validate input coordinates, just in case. */ - if (x == 0 || x > term->cols) + if (x <= 0 || x > term->cols) return; ldata = scrlineptr(y); From 7924aa945a04f2efd5967390474771f377cfbcc3 Mon Sep 17 00:00:00 2001 From: Ben Harris Date: Fri, 16 Oct 2015 23:41:11 +0100 Subject: [PATCH 12/22] Add a -fuzznet option to Unix plink. It just sets the proxy command to "cat %host", which is crude and slow but seems like a good starting point. --- unix/uxplink.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/unix/uxplink.c b/unix/uxplink.c index 8351a6ea..836a37a6 100644 --- a/unix/uxplink.c +++ b/unix/uxplink.c @@ -690,6 +690,10 @@ int main(int argc, char **argv) } } else if (!strcmp(p, "-shareexists")) { just_test_share_exists = TRUE; + } else if (!strcmp(p, "-fuzznet")) { + conf_set_int(conf, CONF_proxy_type, PROXY_CMD); + conf_set_str(conf, CONF_proxy_telnet_command, + "cat %host"); } else { fprintf(stderr, "plink: unknown option \"%s\"\n", p); errors = 1; From 389eb4b7e0e067ce1c425f77abc0d3d774e83c79 Mon Sep 17 00:00:00 2001 From: Ben Harris Date: Sat, 17 Oct 2015 12:12:23 +0100 Subject: [PATCH 13/22] When checking for an existing log, store the FILE * in a local variable. It's not used outside logfopen, and leaving an invalid file pointer lying around in the log context caused a segfault if the user cancelled logging. Bug found by afl-fuzz before it had even started fuzzing. --- logging.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/logging.c b/logging.c index 563dcded..865fe9b8 100644 --- a/logging.c +++ b/logging.c @@ -164,6 +164,7 @@ void logfopen(void *handle) { struct LogContext *ctx = (struct LogContext *)handle; struct tm tm; + FILE *fp; int mode; /* Prevent repeat calls */ @@ -183,10 +184,10 @@ void logfopen(void *handle) conf_get_str(ctx->conf, CONF_host), conf_get_int(ctx->conf, CONF_port), &tm); - ctx->lgfp = f_open(ctx->currlogfilename, "r", FALSE); /* file already present? */ - if (ctx->lgfp) { + fp = f_open(ctx->currlogfilename, "r", FALSE); /* file already present? */ + if (fp) { int logxfovr = conf_get_int(ctx->conf, CONF_logxfovr); - fclose(ctx->lgfp); + fclose(fp); if (logxfovr != LGXF_ASK) { mode = ((logxfovr == LGXF_OVR) ? 2 : 1); } else From b9cb75e8c5fa84f7837ea1a335381881d2227ca4 Mon Sep 17 00:00:00 2001 From: Ben Harris Date: Sat, 17 Oct 2015 12:25:36 +0100 Subject: [PATCH 14/22] Add __AFL_INIT() to uxplink to allow afl-fuzz to skip some startup overhead. --- unix/uxplink.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/unix/uxplink.c b/unix/uxplink.c index 836a37a6..6d402c94 100644 --- a/unix/uxplink.c +++ b/unix/uxplink.c @@ -25,7 +25,7 @@ #define MAX_STDIN_BACKLOG 4096 -void *logctx; +static void *logctx; static struct termios orig_termios; @@ -992,6 +992,11 @@ int main(int argc, char **argv) /* nodelay is only useful if stdin is a terminal device */ int nodelay = conf_get_int(conf, CONF_tcp_nodelay) && isatty(0); + /* This is a good place for a fuzzer to fork us. */ +#ifdef __AFL_HAVE_MANUAL_CONTROL + __AFL_INIT(); +#endif + error = back->init(NULL, &backhandle, conf, conf_get_str(conf, CONF_host), conf_get_int(conf, CONF_port), From f6b81af006cd4a9211bf97ced0fcbfb153897035 Mon Sep 17 00:00:00 2001 From: Ben Harris Date: Sat, 17 Oct 2015 14:06:06 +0100 Subject: [PATCH 15/22] Add an explicit PROXY_FUZZ that just feeds a file into the backend. This saves the need to fork and exec "cat", which should speed things up. It also ensures that the network output goes to /dev/null, which should avoid problems with blocking when writing to a full pipe. --- putty.h | 2 +- unix/uxplink.c | 4 +-- unix/uxproxy.c | 94 ++++++++++++++++++++++++++++++-------------------- 3 files changed, 59 insertions(+), 41 deletions(-) diff --git a/putty.h b/putty.h index c854d944..3ccd96cb 100644 --- a/putty.h +++ b/putty.h @@ -291,7 +291,7 @@ enum { * Proxy types. */ PROXY_NONE, PROXY_SOCKS4, PROXY_SOCKS5, - PROXY_HTTP, PROXY_TELNET, PROXY_CMD + PROXY_HTTP, PROXY_TELNET, PROXY_CMD, PROXY_FUZZ }; enum { diff --git a/unix/uxplink.c b/unix/uxplink.c index 6d402c94..b8c55489 100644 --- a/unix/uxplink.c +++ b/unix/uxplink.c @@ -691,9 +691,9 @@ int main(int argc, char **argv) } else if (!strcmp(p, "-shareexists")) { just_test_share_exists = TRUE; } else if (!strcmp(p, "-fuzznet")) { - conf_set_int(conf, CONF_proxy_type, PROXY_CMD); + conf_set_int(conf, CONF_proxy_type, PROXY_FUZZ); conf_set_str(conf, CONF_proxy_telnet_command, - "cat %host"); + "%host"); } else { fprintf(stderr, "plink: unknown option \"%s\"\n", p); errors = 1; diff --git a/unix/uxproxy.c b/unix/uxproxy.c index 0399245d..8c916dc9 100644 --- a/unix/uxproxy.c +++ b/unix/uxproxy.c @@ -250,13 +250,12 @@ Socket platform_new_connection(SockAddr addr, const char *hostname, }; Local_Proxy_Socket ret; - int to_cmd_pipe[2], from_cmd_pipe[2], pid; + int to_cmd_pipe[2], from_cmd_pipe[2], pid, proxytype; - if (conf_get_int(conf, CONF_proxy_type) != PROXY_CMD) + proxytype = conf_get_int(conf, CONF_proxy_type); + if (proxytype != PROXY_CMD && proxytype != PROXY_FUZZ) return NULL; - cmd = format_telnet_command(addr, port, conf); - ret = snew(struct Socket_localproxy_tag); ret->fn = &socket_fn_table; ret->plug = plug; @@ -266,46 +265,65 @@ Socket platform_new_connection(SockAddr addr, const char *hostname, bufchain_init(&ret->pending_input_data); bufchain_init(&ret->pending_output_data); - /* - * Create the pipes to the proxy command, and spawn the proxy - * command process. - */ - if (pipe(to_cmd_pipe) < 0 || - pipe(from_cmd_pipe) < 0) { - ret->error = dupprintf("pipe: %s", strerror(errno)); - sfree(cmd); - return (Socket)ret; - } - cloexec(to_cmd_pipe[1]); - cloexec(from_cmd_pipe[0]); + if (proxytype == PROXY_CMD) { + cmd = format_telnet_command(addr, port, conf); - pid = fork(); + /* + * Create the pipes to the proxy command, and spawn the proxy + * command process. + */ + if (pipe(to_cmd_pipe) < 0 || + pipe(from_cmd_pipe) < 0) { + ret->error = dupprintf("pipe: %s", strerror(errno)); + sfree(cmd); + return (Socket)ret; + } + cloexec(to_cmd_pipe[1]); + cloexec(from_cmd_pipe[0]); + + pid = fork(); + + if (pid < 0) { + ret->error = dupprintf("fork: %s", strerror(errno)); + sfree(cmd); + return (Socket)ret; + } else if (pid == 0) { + close(0); + close(1); + dup2(to_cmd_pipe[0], 0); + dup2(from_cmd_pipe[1], 1); + close(to_cmd_pipe[0]); + close(from_cmd_pipe[1]); + noncloexec(0); + noncloexec(1); + execl("/bin/sh", "sh", "-c", cmd, (void *)NULL); + _exit(255); + } + + sfree(cmd); - if (pid < 0) { - ret->error = dupprintf("fork: %s", strerror(errno)); - sfree(cmd); - return (Socket)ret; - } else if (pid == 0) { - close(0); - close(1); - dup2(to_cmd_pipe[0], 0); - dup2(from_cmd_pipe[1], 1); close(to_cmd_pipe[0]); close(from_cmd_pipe[1]); - noncloexec(0); - noncloexec(1); - execl("/bin/sh", "sh", "-c", cmd, (void *)NULL); - _exit(255); + + ret->to_cmd = to_cmd_pipe[1]; + ret->from_cmd = from_cmd_pipe[0]; + } else { + cmd = format_telnet_command(addr, port, conf); + ret->to_cmd = open("/dev/null", O_WRONLY); + if (ret->to_cmd == -1) { + ret->error = dupprintf("/dev/null: %s", strerror(errno)); + sfree(cmd); + return (Socket)ret; + } + ret->from_cmd = open(cmd, O_RDONLY); + if (ret->from_cmd == -1) { + ret->error = dupprintf("%s: %s", cmd, strerror(errno)); + sfree(cmd); + return (Socket)ret; + } + sfree(cmd); } - sfree(cmd); - - close(to_cmd_pipe[0]); - close(from_cmd_pipe[1]); - - ret->to_cmd = to_cmd_pipe[1]; - ret->from_cmd = from_cmd_pipe[0]; - if (!localproxy_by_fromfd) localproxy_by_fromfd = newtree234(localproxy_fromfd_cmp); if (!localproxy_by_tofd) From 1d20c1b396738e66612bbdffda5dcd85b28e5267 Mon Sep 17 00:00:00 2001 From: Ben Harris Date: Sat, 17 Oct 2015 16:26:51 +0100 Subject: [PATCH 16/22] Add FUZZING build option that disables the random number generator. Starting up the random number generator is by far the slowest part of plink's startup, and randomness is bad for fuzzing, so disabling it should make fuzzing more effective. --- Recipe | 6 ++++++ sshrand.c | 18 +++++++++++++++++- 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/Recipe b/Recipe index bba63a9c..0bc27c7f 100644 --- a/Recipe +++ b/Recipe @@ -125,6 +125,12 @@ # show up as GPFs at the point of failure rather than appearing # later on as second-level damage. # +# - XFLAGS=/DFUZZING +# Builds a version of PuTTY with some tweaks to make fuzz testing +# easier: the SSH random number generator is replaced by one that +# always returns the same thing. Note that this makes SSH +# completely insecure -- a FUZZING build should never be used to +# connect to a real server. !end # ------------------------------------------------------------ diff --git a/sshrand.c b/sshrand.c index ead39a9b..0fbefb48 100644 --- a/sshrand.c +++ b/sshrand.c @@ -45,8 +45,23 @@ struct RandPool { int stir_pending; }; -static struct RandPool pool; int random_active = 0; + +#ifdef FUZZING +/* + * Special dummy version of the RNG for use when fuzzing. + */ +void random_add_noise(void *noise, int length) { } +void random_add_heavynoise(void *noise, int length) { } +void random_ref(void) { } +void random_unref(void) { } +int random_byte(void) +{ + return 0x45; /* Chosen by eight fair coin tosses */ +} +void random_get_savedata(void **data, int *len) { } +#else /* !FUZZING */ +static struct RandPool pool; long next_noise_collection; #ifdef RANDOM_DIAGNOSTICS @@ -326,3 +341,4 @@ void random_get_savedata(void **data, int *len) *data = buf; random_stir(); } +#endif From 5471539a6738484b48fb938c88dce547a3e4b299 Mon Sep 17 00:00:00 2001 From: Ben Harris Date: Sat, 17 Oct 2015 21:00:31 +0100 Subject: [PATCH 17/22] Handle packets with no type byte by returning SSH_MSG_UNIMPLEMENTED. The previous assertion failure is obviously wrong, but RFC 4253 doesn't explicitly declare them to be a protocol error. Currently, the incoming packet isn't logged, which might cause some confusion for log parsers. Bug found with the help of afl-fuzz. --- ssh.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/ssh.c b/ssh.c index 5d6514b5..1f8a214b 100644 --- a/ssh.c +++ b/ssh.c @@ -364,6 +364,7 @@ static void do_ssh2_authconn(Ssh ssh, const unsigned char *in, int inlen, struct Packet *pktin); static void ssh2_channel_check_close(struct ssh_channel *c); static void ssh_channel_destroy(struct ssh_channel *c); +static void ssh2_msg_something_unimplemented(Ssh ssh, struct Packet *pktin); /* * Buffer management constants. There are several of these for @@ -1834,6 +1835,15 @@ static struct Packet *ssh2_rdpkt(Ssh ssh, const unsigned char **data, } } + /* + * RFC 4253 doesn't explicitly say that completely empty packets + * with no type byte are forbidden, so treat them as deserving + * an SSH_MSG_UNIMPLEMENTED. + */ + if (st->pktin->length <= 5) { /* == 5 we hope, but robustness */ + ssh2_msg_something_unimplemented(ssh, st->pktin); + crStop(NULL); + } /* * pktin->body and pktin->length should identify the semantic * content of the packet, excluding the initial type byte. From af1460d6e5044a3344aaacd15c91cfdcb58578e7 Mon Sep 17 00:00:00 2001 From: Ben Harris Date: Sun, 18 Oct 2015 13:04:58 +0100 Subject: [PATCH 18/22] Add FUZZING support to ssh.c. This adds the "none" cipher and MAC, and also disables kex signure verification and host-key checking. Since a client like this is completely insecure, it also rewrites the client version string to start "ISH", which should make it fail to interoperate with a real SSH server. The server version string is still expected to begin "SSH" so the real packet captures can be used against it. --- ssh.c | 85 +++++++++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 62 insertions(+), 23 deletions(-) diff --git a/ssh.c b/ssh.c index 1f8a214b..288616fd 100644 --- a/ssh.c +++ b/ssh.c @@ -3017,6 +3017,10 @@ static void ssh_send_verstring(Ssh ssh, const char *protoname, char *svers) } ssh_fix_verstring(verstring + strlen(protoname)); +#ifdef FUZZING + /* FUZZING make PuTTY insecure, so make live use difficult. */ + verstring[0] = 'I'; +#endif if (ssh->version == 2) { size_t len; @@ -4050,6 +4054,9 @@ static int do_ssh1_login(Ssh ssh, const unsigned char *in, int inlen, "rsa", keystr, fingerprint, ssh_dialog_callback, ssh); sfree(keystr); +#ifdef FUZZING + s->dlgret = 1; +#endif if (s->dlgret < 0) { do { crReturn(0); @@ -6487,6 +6494,11 @@ static void do_ssh2_transport(Ssh ssh, const void *vin, int inlen, /* List encryption algorithms (client->server then server->client). */ for (k = KEXLIST_CSCIPHER; k <= KEXLIST_SCCIPHER; k++) { warn = FALSE; +#ifdef FUZZING + alg = ssh2_kexinit_addalg(s->kexlists[k], "none"); + alg->u.cipher.cipher = NULL; + alg->u.cipher.warn = warn; +#endif /* FUZZING */ for (i = 0; i < s->n_preferred_ciphers; i++) { const struct ssh2_ciphers *c = s->preferred_ciphers[i]; if (!c) warn = TRUE; @@ -6500,6 +6512,11 @@ static void do_ssh2_transport(Ssh ssh, const void *vin, int inlen, } /* List MAC algorithms (client->server then server->client). */ for (j = KEXLIST_CSMAC; j <= KEXLIST_SCMAC; j++) { +#ifdef FUZZING + alg = ssh2_kexinit_addalg(s->kexlists[j], "none"); + alg->u.mac.mac = NULL; + alg->u.mac.etm = FALSE; +#endif /* FUZZING */ for (i = 0; i < s->nmacs; i++) { alg = ssh2_kexinit_addalg(s->kexlists[j], s->maclist[i]->name); alg->u.mac.mac = s->maclist[i]; @@ -6772,8 +6789,8 @@ static void do_ssh2_transport(Ssh ssh, const void *vin, int inlen, { int csbits, scbits; - csbits = s->cscipher_tobe->real_keybits; - scbits = s->sccipher_tobe->real_keybits; + csbits = s->cscipher_tobe ? s->cscipher_tobe->real_keybits : 0; + scbits = s->sccipher_tobe ? s->sccipher_tobe->real_keybits : 0; s->nbits = (csbits > scbits ? csbits : scbits); } /* The keys only have hlen-bit entropy, since they're based on @@ -7113,8 +7130,10 @@ static void do_ssh2_transport(Ssh ssh, const void *vin, int inlen, !ssh->hostkey->verifysig(s->hkey, s->sigdata, s->siglen, (char *)s->exchange_hash, ssh->kex->hash->hlen)) { +#ifndef FUZZING bombout(("Server's host key did not match the signature supplied")); crStopV; +#endif } s->keystr = ssh->hostkey->fmtkey(s->hkey); @@ -7139,6 +7158,9 @@ static void do_ssh2_transport(Ssh ssh, const void *vin, int inlen, ssh->hostkey->keytype, s->keystr, s->fingerprint, ssh_dialog_callback, ssh); +#ifdef FUZZING + s->dlgret = 1; +#endif if (s->dlgret < 0) { do { crReturnV; @@ -7171,8 +7193,10 @@ static void do_ssh2_transport(Ssh ssh, const void *vin, int inlen, * the one we saw before. */ if (strcmp(ssh->hostkey_str, s->keystr)) { +#ifndef FUZZING bombout(("Host key was different in repeat key exchange")); crStopV; +#endif } sfree(s->keystr); } @@ -7206,13 +7230,14 @@ static void do_ssh2_transport(Ssh ssh, const void *vin, int inlen, if (ssh->cs_cipher_ctx) ssh->cscipher->free_context(ssh->cs_cipher_ctx); ssh->cscipher = s->cscipher_tobe; - ssh->cs_cipher_ctx = ssh->cscipher->make_context(); + if (ssh->cscipher) ssh->cs_cipher_ctx = ssh->cscipher->make_context(); if (ssh->cs_mac_ctx) ssh->csmac->free_context(ssh->cs_mac_ctx); ssh->csmac = s->csmac_tobe; ssh->csmac_etm = s->csmac_etm_tobe; - ssh->cs_mac_ctx = ssh->csmac->make_context(ssh->cs_cipher_ctx); + if (ssh->csmac) + ssh->cs_mac_ctx = ssh->csmac->make_context(ssh->cs_cipher_ctx); if (ssh->cs_comp_ctx) ssh->cscomp->compress_cleanup(ssh->cs_comp_ctx); @@ -7223,7 +7248,7 @@ static void do_ssh2_transport(Ssh ssh, const void *vin, int inlen, * Set IVs on client-to-server keys. Here we use the exchange * hash from the _first_ key exchange. */ - { + if (ssh->cscipher) { unsigned char *key; key = ssh2_mkkey(ssh, s->K, s->exchange_hash, 'C', @@ -7237,6 +7262,9 @@ static void do_ssh2_transport(Ssh ssh, const void *vin, int inlen, ssh->cscipher->setiv(ssh->cs_cipher_ctx, key); smemclr(key, ssh->cscipher->blksize); sfree(key); + } + if (ssh->csmac) { + unsigned char *key; key = ssh2_mkkey(ssh, s->K, s->exchange_hash, 'E', ssh->csmac->keylen); @@ -7245,12 +7273,14 @@ static void do_ssh2_transport(Ssh ssh, const void *vin, int inlen, sfree(key); } - logeventf(ssh, "Initialised %.200s client->server encryption", - ssh->cscipher->text_name); - logeventf(ssh, "Initialised %.200s client->server MAC algorithm%s%s", - ssh->csmac->text_name, - ssh->csmac_etm ? " (in ETM mode)" : "", - ssh->cscipher->required_mac ? " (required by cipher)" : ""); + if (ssh->cscipher) + logeventf(ssh, "Initialised %.200s client->server encryption", + ssh->cscipher->text_name); + if (ssh->csmac) + logeventf(ssh, "Initialised %.200s client->server MAC algorithm%s%s", + ssh->csmac->text_name, + ssh->csmac_etm ? " (in ETM mode)" : "", + ssh->cscipher->required_mac ? " (required by cipher)" : ""); if (ssh->cscomp->text_name) logeventf(ssh, "Initialised %s compression", ssh->cscomp->text_name); @@ -7278,14 +7308,18 @@ static void do_ssh2_transport(Ssh ssh, const void *vin, int inlen, */ if (ssh->sc_cipher_ctx) ssh->sccipher->free_context(ssh->sc_cipher_ctx); - ssh->sccipher = s->sccipher_tobe; - ssh->sc_cipher_ctx = ssh->sccipher->make_context(); + if (ssh->sccipher) { + ssh->sccipher = s->sccipher_tobe; + ssh->sc_cipher_ctx = ssh->sccipher->make_context(); + } if (ssh->sc_mac_ctx) ssh->scmac->free_context(ssh->sc_mac_ctx); - ssh->scmac = s->scmac_tobe; - ssh->scmac_etm = s->scmac_etm_tobe; - ssh->sc_mac_ctx = ssh->scmac->make_context(ssh->sc_cipher_ctx); + if (ssh->scmac) { + ssh->scmac = s->scmac_tobe; + ssh->scmac_etm = s->scmac_etm_tobe; + ssh->sc_mac_ctx = ssh->scmac->make_context(ssh->sc_cipher_ctx); + } if (ssh->sc_comp_ctx) ssh->sccomp->decompress_cleanup(ssh->sc_comp_ctx); @@ -7296,7 +7330,7 @@ static void do_ssh2_transport(Ssh ssh, const void *vin, int inlen, * Set IVs on server-to-client keys. Here we use the exchange * hash from the _first_ key exchange. */ - { + if (ssh->sccipher) { unsigned char *key; key = ssh2_mkkey(ssh, s->K, s->exchange_hash, 'D', @@ -7310,6 +7344,9 @@ static void do_ssh2_transport(Ssh ssh, const void *vin, int inlen, ssh->sccipher->setiv(ssh->sc_cipher_ctx, key); smemclr(key, ssh->sccipher->blksize); sfree(key); + } + if (ssh->scmac) { + unsigned char *key; key = ssh2_mkkey(ssh, s->K, s->exchange_hash, 'F', ssh->scmac->keylen); @@ -7317,12 +7354,14 @@ static void do_ssh2_transport(Ssh ssh, const void *vin, int inlen, smemclr(key, ssh->scmac->keylen); sfree(key); } - logeventf(ssh, "Initialised %.200s server->client encryption", - ssh->sccipher->text_name); - logeventf(ssh, "Initialised %.200s server->client MAC algorithm%s%s", - ssh->scmac->text_name, - ssh->scmac_etm ? " (in ETM mode)" : "", - ssh->sccipher->required_mac ? " (required by cipher)" : ""); + if (ssh->sccipher) + logeventf(ssh, "Initialised %.200s server->client encryption", + ssh->sccipher->text_name); + if (ssh->scmac) + logeventf(ssh, "Initialised %.200s server->client MAC algorithm%s%s", + ssh->scmac->text_name, + ssh->scmac_etm ? " (in ETM mode)" : "", + ssh->sccipher->required_mac ? " (required by cipher)" : ""); if (ssh->sccomp->text_name) logeventf(ssh, "Initialised %s decompression", ssh->sccomp->text_name); From 12702cb17ebe3c6a79284a3d24e95df745aac5e3 Mon Sep 17 00:00:00 2001 From: Ben Harris Date: Sun, 18 Oct 2015 18:55:42 +0100 Subject: [PATCH 19/22] Fix a null-pointer dereference in ecdsa_verifysig. Bug found with the help of afl-fuzz. --- sshecc.c | 1 + 1 file changed, 1 insertion(+) diff --git a/sshecc.c b/sshecc.c index 541dd63c..3912c5f1 100644 --- a/sshecc.c +++ b/sshecc.c @@ -2268,6 +2268,7 @@ static int ecdsa_verifysig(void *key, const char *sig, int siglen, } getstring(&sig, &siglen, &p, &slen); + if (!p) return 0; if (ec->publicKey.curve->type == EC_EDWARDS) { struct ec_point *r; Bignum s, h; From 7a5cb2838fd04711a0bcfd73f24099d7e2e05bb4 Mon Sep 17 00:00:00 2001 From: Ben Harris Date: Sun, 18 Oct 2015 20:16:39 +0100 Subject: [PATCH 20/22] Emit a distinct error message when the SSH server's host key is invalid. This also means that FUZZING can just ignore host-key verification failure while preserving invalid-host-key errors. --- ssh.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/ssh.c b/ssh.c index 288616fd..de8259ed 100644 --- a/ssh.c +++ b/ssh.c @@ -7126,13 +7126,17 @@ static void do_ssh2_transport(Ssh ssh, const void *vin, int inlen, dmemdump(s->exchange_hash, ssh->kex->hash->hlen); #endif - if (!s->hkey || - !ssh->hostkey->verifysig(s->hkey, s->sigdata, s->siglen, + if (!s->hkey) { + bombout(("Server's host key is invalid")); + crStopV; + } + + if (!ssh->hostkey->verifysig(s->hkey, s->sigdata, s->siglen, (char *)s->exchange_hash, ssh->kex->hash->hlen)) { #ifndef FUZZING bombout(("Server's host key did not match the signature supplied")); - crStopV; + crStopV;f #endif } From 9022dcd5c5b9a0bb3caa576e0ec17506e5fb0d3a Mon Sep 17 00:00:00 2001 From: Ben Harris Date: Sat, 24 Oct 2015 17:56:38 +0100 Subject: [PATCH 21/22] fuzzterm: add some output to allow this to be used for testing. Not very much, but it might be useful for testing that changes don't unexpectedly break things. --- fuzzterm.c | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/fuzzterm.c b/fuzzterm.c index a4755d8f..22cda808 100644 --- a/fuzzterm.c +++ b/fuzzterm.c @@ -31,6 +31,7 @@ int main(int argc, char **argv) len = fread(blk, 1, sizeof(blk), stdin); term_data(term, 0, blk, len); } + term_update(term); return 0; } @@ -40,8 +41,16 @@ int from_backend(void *frontend, int is_stderr, const char *data, int len) /* functions required by terminal.c */ void request_resize(void *frontend, int x, int y) { } -void do_text(Context a, int b, int c, wchar_t * d, int e, unsigned long f, int g) { } -void do_cursor(Context a, int b, int c, wchar_t * d, int e, unsigned long f, int g) { } +void do_text(Context ctx, int x, int y, wchar_t * text, int len, + unsigned long attr, int lattr) +{ + printf("TEXT[attr=%08lx,lattr=%02x]@(%d,%d): %d\n", attr, lattr, x, y, len); +} +void do_cursor(Context ctx, int x, int y, wchar_t * text, int len, + unsigned long attr, int lattr) +{ + printf("CURS[attr=%08lx,lattr=%02x]@(%d,%d): %d\n", attr, lattr, x, y, len); +} int char_width(Context ctx, int uc) { return 1; } void set_title(void *frontend, char *t) { } void set_icon(void *frontend, char *t) { } @@ -49,7 +58,11 @@ void set_sbar(void *frontend, int a, int b, int c) { } void ldisc_send(void *handle, const char *buf, int len, int interactive) {} void ldisc_echoedit_update(void *handle) {} -Context get_ctx(void *frontend) { return NULL; } +Context get_ctx(void *frontend) { + static char x; + + return &x; +} void free_ctx(Context ctx) { } void palette_set(void *frontend, int a, int b, int c, int d) { } void palette_reset(void *frontend) { } From 6627c1ce1390ab14b16cb84ee0605e52ad91f0b5 Mon Sep 17 00:00:00 2001 From: Ben Harris Date: Tue, 27 Oct 2015 20:09:42 +0000 Subject: [PATCH 22/22] fuzzterm: record characters being displayed. --- fuzzterm.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/fuzzterm.c b/fuzzterm.c index 22cda808..0da3b68d 100644 --- a/fuzzterm.c +++ b/fuzzterm.c @@ -44,12 +44,24 @@ void request_resize(void *frontend, int x, int y) { } void do_text(Context ctx, int x, int y, wchar_t * text, int len, unsigned long attr, int lattr) { - printf("TEXT[attr=%08lx,lattr=%02x]@(%d,%d): %d\n", attr, lattr, x, y, len); + int i; + + printf("TEXT[attr=%08lx,lattr=%02x]@(%d,%d):", attr, lattr, x, y); + for (i = 0; i < len; i++) { + printf(" %x", (unsigned)text[i]); + } + printf("\n"); } void do_cursor(Context ctx, int x, int y, wchar_t * text, int len, unsigned long attr, int lattr) { - printf("CURS[attr=%08lx,lattr=%02x]@(%d,%d): %d\n", attr, lattr, x, y, len); + int i; + + printf("CURS[attr=%08lx,lattr=%02x]@(%d,%d):", attr, lattr, x, y); + for (i = 0; i < len; i++) { + printf(" %x", (unsigned)text[i]); + } + printf("\n"); } int char_width(Context ctx, int uc) { return 1; } void set_title(void *frontend, char *t) { }