Created attachment 2953 [details] proposed coverity patch Recent run on Coveriry revealed several issues: auth-pam.c * NULL_RETURNS -- missing check before dereferencing allocated buffer clientloop.c * REVERSE_INULL -- check for null after it was already dereferenced --> check for null earlier too digest-openssl.c * NULL_RETURNS -- missing check for null kex.c * RESOURCE_LEAK -- match_list() allocates data which is not freed in several cases readconf.c * RESOURCE_LEAK -- Variable "arg2" going out of scope leaks the storage it points to. servconf.c * DEADCODE -- cannot reach the expression ""none"" sshconnect.c * RESOURCE_LEAK -- Handle variable "auth_sock" going out of scope leaks the handle. sshconnect2.c * RESOURCE_LEAK -- Variable "blob" going out of scope leaks the storage it points to. sshkey.c * IDENTICAL_BRANCHES/DEADCODE Let me know if the patch is acceptable or you need something to improve. There are few more items in the scan that I will have to investigate.
a quick glance at the diff and these look reasonable, put on list for 7.5
Created attachment 2954 [details] 2nd part with lower priority few more reports with lower priority and confidence of the need to be fixed: krl.c * RESOURCE_LEAK: Overwriting "sect" in "sect = NULL" leaks the storage that "sect" points to. * NEGATIVE_RETURNS: "fd" is passed to a parameter that cannot be negative in case the open() fails. readconf.c servconf.c * REVERSE_INULL: bogus NULL checks (can't be NULL in any of the cases) ssh-pkcs11.c * NULL_RETURNS: Sanitize return value of sshkey_new() sshconnect1.c * NULL_RETURNS: Sanitize return value of sshkey_new() sshkey.c * NULL_RETURNS: Directly dereferencing parameter "ret".
Applied the auth-pam.c one. Looking at upstreaming the others.
Comment on attachment 2953 [details] proposed coverity patch >+ if (ret == NULL) >+ return NULL; applied. > response = read_passphrase("Accept updated hostkeys? " > "(yes/no): ", RP_ECHO); >- if (strcasecmp(response, "yes") == 0) >+ if (response != NULL && strcasecmp(response, "yes") == 0) I think this is a false positive. read_passphrase() can only return NULL if given the RP_ALLOW_EOF flag, otherwise the return values all come from xstrdup which will provide a valid pointer or die trying. >- if (dlen > UINT_MAX) >+ if (digest == NULL || dlen > UINT_MAX) looks reasonable, applied. [several memory and fd leak fixes] seem reasonable, looking at them now. > dump_cfg_string(ServerOpCodes code, const char *val) > { >- if (val == NULL) >- return; > printf("%s %s\n", lookup_opcode_name(code), > val == NULL ? "none" : val); not sure what the intent of this was, will need to investigate. >diff --git a/sshkey.c b/sshkey.c >index 85fd1bd..58c1051 100644 >--- a/sshkey.c >+++ b/sshkey.c >@@ -1375,8 +1375,6 @@ sshkey_read(struct sshkey *ret, char **cpp) > retval = 0; > /*XXXX*/ > sshkey_free(k); >- if (retval != 0) >- break; > break; Dunno what that was supposed to be. Damien? 1.1 (djm 24-Jun-14): /*XXXX*/ 1.1 (djm 24-Jun-14): sshkey_free(k); 1.1 (djm 24-Jun-14): if (retval != 0) 1.1 (djm 24-Jun-14): break;
Comment on attachment 2953 [details] proposed coverity patch >+++ b/sshconnect2.c >@@ -1061,6 +1061,7 @@ sign_and_send_pubkey(Authctxt *authctxt, Identity *id) > > if (key_to_blob(id->key, &blob, &bloblen) == 0) { > /* we cannot handle this key */ >+ free(blob); > debug3("sign_and_send_pubkey: cannot handle key"); > return 0; > } >@@ -1170,6 +1171,7 @@ send_pubkey_test(Authctxt *authctxt, Identity *id) > > if (key_to_blob(id->key, &blob, &bloblen) == 0) { > /* we cannot handle this key */ >+ free(blob); > debug3("send_pubkey_test: cannot handle key"); > return 0; > } Damien points out that key_to_blob does not allocate in the failure case and sets blob to NULL so these are not necessary.
Comment on attachment 2954 [details] 2nd part with lower priority >diff --git a/krl.c b/krl.c >index e271a19..69bec99 100644 >--- a/krl.c >+++ b/krl.c >@@ -1089,7 +1089,7 @@ ssh_krl_from_blob(struct sshbuf *buf, struct ssh_krl **krlp, > break; > case KRL_SECTION_SIGNATURE: > /* Handled above, but still need to stay in synch */ >- sshbuf_reset(sect); >+ sshbuf_free(sect); Not sure about this one. Damien? >- filename, linenum, arg ? arg : "<NONE>"); >+ filename, linenum, arg); I'm not sure removing this is a good idea; it might not be possible for arg to be null right now but later? >diff --git a/ssh-pkcs11.c b/ssh-pkcs11.c >index aaf712d..62a76b3 100644 >--- a/ssh-pkcs11.c >+++ b/ssh-pkcs11.c >@@ -536,8 +536,8 @@ pkcs11_fetch_keys_filter(struct pkcs11_provider *p, CK_ULONG slotidx, > X509_free(x509); > } > if (rsa && rsa->n && rsa->e && >- pkcs11_rsa_wrap(p, slotidx, &attribs[0], rsa) == 0) { >- key = sshkey_new(KEY_UNSPEC); >+ pkcs11_rsa_wrap(p, slotidx, &attribs[0], rsa) == 0 && >+ (key = sshkey_new(KEY_UNSPEC)) == NULL) { not sure about this either. Damien? >- host_key = key_new(KEY_RSA1); >+ if ((host_key = key_new(KEY_RSA1)) == NULL) >+ fatal("%s: key_new(KEY_RSA1) failed", __func__); applied (both instances) >+ if (ret == NULL) >+ return SSH_ERR_INVALID_ARGUMENT; applied
Thanks for comments and thoughtful reread. Adding answers where it makes sense. (In reply to Darren Tucker from comment #4) > Comment on attachment 2953 [details] > > response = read_passphrase("Accept updated hostkeys? " > > "(yes/no): ", RP_ECHO); > >- if (strcasecmp(response, "yes") == 0) > >+ if (response != NULL && strcasecmp(response, "yes") == 0) > > I think this is a false positive. > read_passphrase() can only return NULL if given the RP_ALLOW_EOF > flag, otherwise the return values all come from xstrdup which will > provide a valid pointer or die trying. In that case it does not make sense to compare with NULL two lines below (which was the concern of coverity report REVERSE_INULL): > else if (quit_pending || response == NULL || yes. My patch was one possible solution, but there are other (maybe better) ways how to make it clear. > > dump_cfg_string(ServerOpCodes code, const char *val) > > { > >- if (val == NULL) > >- return; > > printf("%s %s\n", lookup_opcode_name(code), > > val == NULL ? "none" : val); > > not sure what the intent of this was, will need to investigate. either the first or the second check for NULL is bogus. We should prefer dumping configuration options with "none" rather than not, but making sure that the options have empty value "none" accepted is a good thing to consider before applying. (In reply to Darren Tucker from comment #6) > Comment on attachment 2954 [details] > 2nd part with lower priority > >- filename, linenum, arg ? arg : "<NONE>"); > >+ filename, linenum, arg); > > I'm not sure removing this is a good idea; it might not be possible > for arg to be null right now but later? That is valid point. But now it is just a bogus check. I don't think these days any of the options allow empty argument (and only case where I left the <NONE> check was in (s|o)LogLevel and sLogFacility, which do not have the check for missing argument in the first place. It quite looked like copy&paste "error" from some other places. Standardizing also the other mentioned case to test for missing argument in first place would probably make sense too.
Move incomplete bugs to openssh-7.6 target since 7.5 shipped a while back. To calibrate expectations, there's little chance all of these are going to make 7.6.
remove 7.5 target
Move to OpenSSH 7.8 tracking bug
Retarget remaining bugs planned for 7.8 release to 7.9
Created attachment 3176 [details] New patch set (openssh-7.8) I am attaching a new set of patches fixing various issues reported by coverity. Let me know if some of them are not clear.
Ignore the following patch: [PATCH 08/11] sshd: Avoid memory leak
Comment on attachment 3176 [details] New patch set (openssh-7.8) I've applied the portable-only bits with the exception of this one, which looks like it's adding a leak of msg? > auth-pam.c | 22 +++++++++++++--------- [...] >- free(msg); >- return 0; >+ rv = 0; >+ goto out; [...] >+out: >+ sshbuf_free(buffer); >+ return (rv); I'll look at upstreaming the other parts.
Retarget unfinished bugs to OpenSSH 8.0
Retarget outstanding bugs at next release
Created attachment 3287 [details] New coverity issues from 8.0p1 release I am attaching new issues found by coverity for the latest release. Nothing significant, but there are few cleanups that would make sense to apply.
Retarget these bugs to 8.2 release
Prepare for 8.2 release; retarget bugs
Retarget bugs to 8.4 release
retarget to 8.6
retarget after 8.6p1 release
Comment on attachment 3287 [details] New coverity issues from 8.0p1 release Need to take a closer look at ssh_api.c but the rest of these seem fixed: >@@ -2228,6 +2228,9 @@ update_krl_from_file(struct passwd *pw, const char *file, int wild_ca, > cp = cp + strspn(cp, " \t"); > hash_to_blob(cp, &blob, &blen, file, lnum); > r = ssh_krl_revoke_key_sha256(krl, blob, blen); >+ freezero(blob, blen); >+ blob = NULL; >+ blen = 0; There's now a call to fatal here, so I think this is fixed albeit in a different way. >From 73bf5d1f21852f8e6ea315c64a6960a27f1c2c0d Mon Sep 17 00:00:00 2001 >From: Jakub Jelen <jjelen@redhat.com> >Date: Mon, 27 May 2019 14:02:12 +0200 >Subject: [PATCH 3/6] Coverity: leaked storage > > 43. openssh-8.0p1/auth-options.c:538: leaked_storage: Variable "opt" going out of scope leaks the storage it points to. This one has been fixed. >From e6e54a94be55110d7b4bf2a220d8f7c2f5d4062d Mon Sep 17 00:00:00 2001 >From: Jakub Jelen <jjelen@redhat.com> >Date: Mon, 27 May 2019 14:06:23 +0200 >Subject: [PATCH 4/6] Coverity: Remove unused variable max > > 1. openssh-8.0p1/ssh-pkcs11-helper.c:356:3: warning: Value stored to 'max' is never read This one has been removed. >From 14eab3a31c68b5aaed307fdf6a3260c3a3035d7f Mon Sep 17 00:00:00 2001 >From: Jakub Jelen <jjelen@redhat.com> >Date: Mon, 27 May 2019 14:14:58 +0200 >Subject: [PATCH 6/6] Coverity: Unused variable r > > 1. openssh-8.0p1/auth2.c:221:2: warning: Value stored to 'r' is never read This one has been fixed.
Comment on attachment 2953 [details] proposed coverity patch >diff --git a/auth-pam.c b/auth-pam.c >index e554ec4..bd16d80 100644 >--- a/auth-pam.c >+++ b/auth-pam.c >@@ -834,6 +834,8 @@ fake_password(const char *wire_password) > fatal("%s: password length too long: %zu", __func__, l); > > ret = malloc(l + 1); >+ if (ret == NULL) >+ return NULL; This already is done. >+++ b/clientloop.c >@@ -2290,7 +2290,7 @@ update_known_hosts(struct hostkeys_update_ctx *ctx) > free(response); > response = read_passphrase("Accept updated hostkeys? " > "(yes/no): ", RP_ECHO); >- if (strcasecmp(response, "yes") == 0) >+ if (response != NULL && strcasecmp(response, "yes") == 0) applied, thanks. >+ if (digest == NULL || dlen > UINT_MAX) This is already done. >diff --git a/kex.c b/kex.c >index a30dabe..7e4a7ab 100644 >--- a/kex.c >+++ b/kex.c >@@ -178,7 +178,7 @@ kex_names_valid(const char *names) I think these have all been fixed. >diff --git a/readconf.c b/readconf.c >index 3e7a5d8..acc1391 100644 >--- a/readconf.c >+++ b/readconf.c >@@ -1500,6 +1500,7 @@ parse_keytypes: > if (r == GLOB_NOMATCH) { > debug("%.200s line %d: include %s matched no " > "files",filename, linenum, arg2); >+ free(arg2); This is now fixed. >diff --git a/servconf.c b/servconf.c >index 6ab1cb4..5f2464a 100644 >--- a/servconf.c >+++ b/servconf.c >@@ -2284,8 +2284,6 @@ dump_cfg_fmtint(ServerOpCodes code, int val) > static void > dump_cfg_string(ServerOpCodes code, const char *val) > { >- if (val == NULL) >- return; > printf("%s %s\n", lookup_opcode_name(code), > val == NULL ? "none" : val); > } This is now fixed. >diff --git a/sshconnect.c b/sshconnect.c >index 07f80cd..7361898 100644 >--- a/sshconnect.c >+++ b/sshconnect.c >@@ -1533,6 +1533,7 @@ maybe_add_key_to_agent(char *authfile, Key *private, char *comment, These are now fixed. >diff --git a/sshconnect2.c b/sshconnect2.c >index f31c24c..aecf765 100644 >--- a/sshconnect2.c >+++ b/sshconnect2.c [...] >+ free(blob); This is now freed on the "out:" path. >diff --git a/sshkey.c b/sshkey.c >index 85fd1bd..58c1051 100644 >--- a/sshkey.c >+++ b/sshkey.c >@@ -1375,8 +1375,6 @@ sshkey_read(struct sshkey *ret, char **cpp) I think this function has changed and this diff is no longer relevant.
Comment on attachment 2954 [details] 2nd part with lower priority >diff --git a/krl.c b/krl.c > /* Handled above, but still need to stay in synch */ >- sshbuf_reset(sect); >+ sshbuf_free(sect); This one is done. >@@ -1288,7 +1288,8 @@ ssh_krl_file_contains_key(const char *path, const struct sshkey *key) > debug2("%s: checking KRL %s", __func__, path); > r = ssh_krl_check_key(krl, key); > out: >- close(fd); >+ if (fd != -1) >+ close(fd); This function doesn't do any descriptor handling any more. >diff --git a/readconf.c b/readconf.c >index acc1391..c4dff15 100644 >--- a/readconf.c >+++ b/readconf.c >@@ -1185,7 +1185,7 @@ parse_int: > value = cipher_number(arg); > if (value == -1) > fatal("%.200s line %d: Bad cipher '%s'.", >- filename, linenum, arg ? arg : "<NONE>"); >+ filename, linenum, arg); This code is gone (I think it was part of SSH1 handling). > if (*activep && *intptr == -1) > *intptr = value; > break; >@@ -1196,7 +1196,7 @@ parse_int: > fatal("%.200s line %d: Missing argument.", filename, linenum); > if (*arg != '-' && !ciphers_valid(*arg == '+' ? arg + 1 : arg)) > fatal("%.200s line %d: Bad SSH2 cipher spec '%s'.", >- filename, linenum, arg ? arg : "<NONE>"); >+ filename, linenum, arg); I think this one is wrong: it'll potentially cause a NULL pointer deref platforms whose string libs don't guard against it (eg Solaris). Same for the following instances of the same thing. >diff --git a/ssh-pkcs11.c b/ssh-pkcs11.c >index aaf712d..62a76b3 100644 >--- a/ssh-pkcs11.c >+++ b/ssh-pkcs11.c >@@ -536,8 +536,8 @@ pkcs11_fetch_keys_filter(struct pkcs11_provider *p, CK_ULONG slotidx, This function has been removed. >diff --git a/sshconnect1.c b/sshconnect1.c SSH1 support has been removed. >diff --git a/sshkey.c b/sshkey.c >index 58c1051..6afacb5 100644 >--- a/sshkey.c >+++ b/sshkey.c >@@ -1239,6 +1239,9 @@ sshkey_read(struct sshkey *ret, char **cpp) > u_long bits; > #endif /* WITH_SSH1 */ > >+ if (ret == NULL) >+ return SSH_ERR_INVALID_ARGUMENT; This has already been fixed.
Comment on attachment 3176 [details] New patch set (openssh-7.8) >@@ -186,11 +186,16 @@ proto_spec(const char *spec) > char * > compat_cipher_proposal(char *cipher_prop) This has been fixed (after some headaches). > if (!(datafellows & SSH_BUG_BIGENDIANAES)) > return cipher_prop; > debug2("%s: original cipher proposal: %s", __func__, cipher_prop); >- if ((cipher_prop = match_filter_blacklist(cipher_prop, "aes*")) == NULL) >+ np = match_filter_blacklist(cipher_prop, "aes*"); >+ if (np == NULL) > fatal("match_filter_blacklist failed"); >+ free(cipher_prop); >+ cipher_prop = np; > debug2("%s: compat cipher proposal: %s", __func__, cipher_prop); > if (*cipher_prop == '\0') > fatal("No supported ciphers found"); >@@ -200,11 +205,16 @@ compat_cipher_proposal(char *cipher_prop) > char * > compat_pkalg_proposal(char *pkalg_prop) This function has been reduced to more or less a no-op. We removed support for these ancient (~20yo) buggy implementations, and when we removed the bug bits we were able to delete almost all of this function. >@@ -779,6 +779,7 @@ deserialise_array(struct sshbuf *m, char ***ap, size_t *np) > n = tmp; > if (n > 0 && (a = calloc(n, sizeof(*a))) == NULL) { > r = SSH_ERR_ALLOC_FAIL; >+ n = 0; > goto out; > } I think this one is now unnecessary, although there's not quite enough context to see, but I think this was fixed later with a NULL test in the out: path. out: if (a != NULL) { for (i = 0; i < n; i++) free(a[i]); free(a); } >diff --git a/openbsd-compat/port-linux.c b/openbsd-compat/port-linux.c [...] >+#include <stdlib.h> This has already been fixed. >diff --git a/openbsd-compat/setproctitle.c b/openbsd-compat/setproctitle.c [...] >+ size_t len = 0; This has already been fixed. > ssh_packet_read_seqnr(struct ssh *ssh, u_char *typep, u_int32_t *seqnr_p) [...] >+ int len, r, ms_remain = 0; Applied, thanks. still need to go through the rest starting from do_setup_env
Comment on attachment 3176 [details] New patch set (openssh-7.8) >--- a/session.c >+++ b/session.c >@@ -1183,6 +1183,7 @@ do_setup_env(struct ssh *ssh, Session *s, const char *shell) > } > *value++ = '\0'; > child_set_env(&env, &envsize, cp, value); >+ free(cp); I think you're right, I've sent it upstream. > sftp.c | 2 ++ > 1 file changed, 2 insertions(+) > >diff --git a/sftp.c b/sftp.c [...] >+ /* FALLTHROUGH */ These are already done. >diff --git a/sshd.c b/sshd.c [...] >+ algs = list_hostkey_types(); >+ myproposal[PROPOSAL_SERVER_HOST_KEY_ALGS] = compat_pkalg_proposal(algs); >+ free(algs); This one also needs to be done. Sent upstream. >diff --git a/channels.c b/channels.c [...] >- host_to_connect = xstrdup(fwd->connect_path); >+ host_to_connect = fwd->connect_path; > port_to_connect = PORT_STREAMLOCAL; These have already been done. >From a3c6626b39bf049ab9ea7e9dc1fa8caea18df4ad Mon Sep 17 00:00:00 2001 >From: Jakub Jelen <jjelen@redhat.com> >Date: Tue, 28 Aug 2018 15:12:01 +0200 >Subject: [PATCH 10/11] auth-pam: Avoid memory leak of buffer > >--- > auth-pam.c | 22 +++++++++++++--------- This was fixed slightly differently in commit ec0e6243660bf2df30c620a6a0d83eded376c9c6. That said, now that we don't have to support SSH1 any more, we don't actually need to accumulate the messages into a single buffer, so we could just pass them through to keyboard-interactive and probably simplify this quite a bit). >diff --git a/auth-options.c b/auth-options.c >index 32e9bda1..bb4410e7 100644 [...] >+ free(opt); This has already been done.