From 061ca8d8449e9b96ca684a60f509f143fd1111bc Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Tue, 23 Jul 2019 18:51:59 +0100 Subject: [PATCH] ssh2userauth: be more careful about s->ki_scc being NULL. Coverity points out that we're inconsistent about checking it for NULL: seat_stripctrl_new() *can* return NULL, so in principle, we can go through the initialisation code for s->ki_scc and have it still be NULL afterwards. I check that in the code that uses it to sanitise the prompt strings out of USERAUTH_INFO_REQUEST, but not in the code that sanitises the name or instruction strings. Now all uses are checked in the same way. This is only a latent bug, because the four main Seat implementations (GUI and console, on Windows and Unix) never return NULL. The only implementation of seat_stripctrl_new which _can_ return NULL is the trivial nullseat_stripctrl_new, currently only used by Uppity. However, of course, if that changes in future, this latent bug could turn into a real one, so better to fix it before that happens. Thanks Coverity. --- ssh2userauth.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/ssh2userauth.c b/ssh2userauth.c index aaaadd0b..89e62252 100644 --- a/ssh2userauth.c +++ b/ssh2userauth.c @@ -1302,10 +1302,14 @@ static void ssh2_userauth_process_queue(PacketProtocolLayer *ppl) sb = strbuf_new(); if (name.len) { - stripctrl_retarget(s->ki_scc, BinarySink_UPCAST(sb)); - put_datapl(s->ki_scc, name); - stripctrl_retarget(s->ki_scc, NULL); - + if (s->ki_scc) { + stripctrl_retarget(s->ki_scc, + BinarySink_UPCAST(sb)); + put_datapl(s->ki_scc, name); + stripctrl_retarget(s->ki_scc, NULL); + } else { + put_datapl(sb, name); + } s->cur_prompt->name_reqd = true; } else { put_datapl(sb, PTRLEN_LITERAL( @@ -1316,10 +1320,14 @@ static void ssh2_userauth_process_queue(PacketProtocolLayer *ppl) sb = strbuf_new(); if (inst.len) { - stripctrl_retarget(s->ki_scc, BinarySink_UPCAST(sb)); - put_datapl(s->ki_scc, inst); - stripctrl_retarget(s->ki_scc, NULL); - + if (s->ki_scc) { + stripctrl_retarget(s->ki_scc, + BinarySink_UPCAST(sb)); + put_datapl(s->ki_scc, inst); + stripctrl_retarget(s->ki_scc, NULL); + } else { + put_datapl(sb, inst); + } s->cur_prompt->instr_reqd = true; } else { s->cur_prompt->instr_reqd = false;