[Openswan dev] openswan fixes for issues found in coverity scan (fwd)
Paul Wouters
paul at xelerance.com
Mon May 23 19:05:01 EDT 2011
Thanks for the report and the comments. The simple changes have been commited.
The pem.c and the osw_process_secret_records() have not yet been addressed.
Paul
---------- Forwarded message ----------
Date: Fri, 20 May 2011 02:28:21 -0400 (EDT)
From: D. Hugh Redelmeier <hugh at mimosa.com>
To: Paul Wouters <paul at xelerance.com>
Subject: Re: openswan fixes for issues found in coverity scan (fwd)
| From: Paul Wouters <paul at xelerance.com>
| | I'll go over these as well. havent looked myself yet.
Do you wish to keep this conversation private? From the list? From
the original participants? I don't care if you make this message more public.
I'm having a super quick and sloppy look. Don't trust what I say.
My comments come after the code they comment on.
When I say "looks like a fix", I've not looked at anything but the
patch. So the patch could be quite wrong.
| | diff --git a/lib/libipsecconf/confread.c b/lib/libipsecconf/confread.c
| index 43bcb12..c4b4527 100644
| --- a/lib/libipsecconf/confread.c
| +++ b/lib/libipsecconf/confread.c
| @@ -869,8 +869,8 @@ static int load_conn (struct starter_config *cfg
| for(alsosize=0; alsos[alsosize]!=NULL; alsosize++);
| | alsoplace = 0;
| - while(alsos != NULL
| - && alsoplace < alsosize && alsos[alsoplace] != NULL | + /*alsos
is equal to conn->alsos that has been already veirfied for NULL*/
veirfied for NULL => known to be non-NULL
I haven't read the code to see if this is true. I don't think that it
is since inside the loop alsos is assigned the result of an unchecked
call to xrealloc which is, in turn, an unchecked call to realloc,
which can return NULL. Of course that call should not be unchecked.
The test in the while condition isn't sufficient.
What is the point of xrealloc?
I'm stopping looking at this point. I don't have a lot of time and
the code looks ugly.
| + while(alsoplace < alsosize && alsos[alsoplace] != NULL |
&& alsoplace < ALSO_LIMIT)
| {
| /*
| @@ -1160,11 +1160,11 @@ struct starter_conn *alloc_add_conn(struct
starter_config *cfg, char *name, err_
| struct starter_conn *conn;
| | conn = (struct starter_conn *)malloc(sizeof(struct
starter_conn));
| - memset(conn, 0, sizeof(struct starter_conn));
| if (!conn) {
| if (perr) *perr = xstrdup("can't allocate mem in confread_load()");
| return NULL;
| }
| + memset(conn, 0, sizeof(struct starter_conn));
| | conn_default(name, conn, &cfg->conn_default);
| conn->name = xstrdup(name);
Looks like a fix.
| @@ -1249,7 +1249,11 @@ struct starter_config *confread_load(const char *file
| */
| err += load_setup(cfg, cfgp, perr);
| | - if(err) {return NULL;}
| + if(err) {
| + parser_free_conf(cfgp);
| + confread_free(cfg);
| + return NULL;
| + }
| | if(!setuponly) {
| /**
Looks like a fix (but I haven't checked).
| diff --git a/lib/libopenswan/alg_info.c b/lib/libopenswan/alg_info.c
| index b68396e..259cabf 100644
| --- a/lib/libopenswan/alg_info.c
| +++ b/lib/libopenswan/alg_info.c
| @@ -658,7 +658,8 @@ parser_alg_info_add(struct parser_context *p_ctx
| p_ctx->modp_buf,
| modp_id));
| | - if (modp_id && !(gd=lookup_group(modp_id))) {
| + gd=lookup_group(modp_id);
| + if (modp_id && !gd ) {
| p_ctx->err="found modp group id, but not supported";
| goto out;
| }
This looks like a non-fix.
gd is never used. I'd delete its declaration and change the test to
if (modp_id && !lookup_group(modp_id)) {
That doesn't change the original meaning. It won't fix any bug.
But the fix proposed doesn't fix any bug either (as far as I can see).
| diff --git a/lib/libopenswan/atoaddr.c b/lib/libopenswan/atoaddr.c
| index 1c02938..4e0c0cf 100644
| --- a/lib/libopenswan/atoaddr.c
| +++ b/lib/libopenswan/atoaddr.c
| @@ -83,10 +83,18 @@ struct in_addr *addrp;
| | /* next, check that it's a vaguely legal name */
| for (q = p; *q != '\0'; q++)
| - if (!isprint(*q))
| + if (!isprint(*q)) {
| + if (p != namebuf) {
| + FREE(p);
| + }
| return "unprintable character in name";
| - if (strspn(p, namechars) != srclen)
| + }
| + if (strspn(p, namechars) != srclen) {
| + if (p != namebuf) {
| + FREE(p);
| + }
| return "illegal (non-DNS-name) character in name";
| + }
| | /* try as host name, failing that as /etc/networks network name */
| h = gethostbyname(p);
Looks like a fix.
| diff --git a/lib/libopenswan/optionsfrom.c b/lib/libopenswan/optionsfrom.c
| index 8038d71..2d1671e 100644
| --- a/lib/libopenswan/optionsfrom.c
| +++ b/lib/libopenswan/optionsfrom.c
| @@ -97,8 +97,10 @@ int optind; /* current optind,
number of next argument */
| | newargc = *argcp + SOME;
| newargv = malloc((newargc+1) * sizeof(char *));
| - if (newargv == NULL)
| + if (newargv == NULL) {
| + fclose(f);
| return "unable to allocate memory";
| + }
| memcpy(newargv, *argvp, optind * sizeof(char *));
| room = SOME;
| next = optind;
Looks like a fix.
| diff --git a/lib/libopenswan/oswconf.c b/lib/libopenswan/oswconf.c
| index 228767c..93058a5 100644
| --- a/lib/libopenswan/oswconf.c
| +++ b/lib/libopenswan/oswconf.c
| @@ -119,7 +119,7 @@ void osw_conf_setdefault(void)
| | if((env = getenv("IPSEC_CONFFILE")) != NULL) {
| pfree(conffile);
| - ipsec_conf_dir = clone_str(env, "ipsec.conf");
| + conffile = clone_str(env, "ipsec.conf");
| }
| | global_oco.rootdir = "";
Looks serious. Like this should have been found before.
| @@ -203,18 +203,20 @@ bool Pluto_IsFIPS(void)
| FILE *fd=fopen("/proc/sys/crypto/fips_enabled","r");
| | if(fd!=NULL) {
| - n = fread ((void *)fips_flag, 1, 1, fd);
| - if(n==1) {
| + n = fread ((void *)fips_flag, 1, 1, fd);
| + if(n==1) {
| if(fips_flag[0]=='1') {
| + fclose(fd);
| return TRUE;
| - }
| + }
| else {
| openswan_log("Non-fips mode set in
/proc/sys/crypto/fips_enabled");
| }
| }
| else {
| openswan_log("error in reading
/proc/sys/crypto/fips_enabled, returning non-fips mode");
| - } | + }
| + fclose(fd); | }
| else {
| openswan_log("Not able to open /proc/sys/crypto/fips_enabled,
returning non-fips mode");
Looks like a fix.
Why is fips_flag two characters long? Only 1 char is ever used.
| diff --git a/lib/libopenswan/pem.c b/lib/libopenswan/pem.c
| index 396725c..27da8cd 100644
| --- a/lib/libopenswan/pem.c
| +++ b/lib/libopenswan/pem.c
| @@ -277,6 +277,8 @@ pem_decrypt(chunk_t *blob, chunk_t *iv
| | pass->prompt(RC_ENTERSECRET, "need passphrase for '%s'", label);
| | + clonetochunk(blob_copy, blob->ptr, blob->len, "blob copy");
| +
| for (i = 0; i < MAX_PROMPT_PASS_TRIALS; i++)
| {
| int n;
| @@ -302,8 +304,6 @@ pem_decrypt(chunk_t *blob, chunk_t *iv
| return ugh;
| }
| | - clonetochunk(blob_copy, blob->ptr, blob->len, "blob copy");
| -
| if (pem_decrypt_3des(blob, iv, pass->secret))
| {
| pass->prompt(RC_SUCCESS, "valid passphrase, private key loaded
successfully");
| @@ -313,9 +313,10 @@ pem_decrypt(chunk_t *blob, chunk_t *iv
| | /* blob is useless after wrong decryption, restore the
original */
| pfree(blob->ptr);
| - *blob = blob_copy;
| + clonetochunk(*blob, blob_copy.ptr, blob_copy.len, "blob copy");
| }
| pass->prompt(RC_LOG_SERIOUS, "%s", ugh);
| + pfree(blob_copy.ptr);
| return ugh;
| }
| else
I think that this *introduces* a leak. blob_copy.ptr isn't freed in
every path that ends in a return.
| diff --git a/lib/libopenswan/secrets.c b/lib/libopenswan/secrets.c
| index 0dee906..108e9f5 100644
| --- a/lib/libopenswan/secrets.c
| +++ b/lib/libopenswan/secrets.c
| @@ -338,6 +338,7 @@ allocate_RSA_public_key(const cert_t cert)
| break;
| default:
| openswan_log("RSA public key allocation error");
| + pfreeany(pk)
| return NULL;
| }
Looks like a fix
| @@ -1257,6 +1258,7 @@ osw_process_secret_records(struct secret **psecrets,
int verbose,
| /* expecting a list of indices and then the key info */
| s = alloc_thing(struct secret, "secret");
| | + if (s != NULL) {
| s->ids = NULL;
| s->pks.kind = PPK_PSK; /* default */
| setchunk(s->pks.u.preshared_secret, NULL, 0);
alloc_thing is never supposed to return NULL. So this addition looks
pointless.
The line before this chunk defines s and initializes it to NULL. But
that initialization is pointless since it is immediately followed by
an assignment to s.
Who writes code like that? (Actually, it looks like some of the code
was written by me.)
| @@ -1267,7 +1269,8 @@ osw_process_secret_records(struct secret **psecrets,
int verbose,
| s->pks.u.RSA_private_key.pub.nssCert = NULL;
| #endif
| | - while(s != NULL)
| + //while(s != NULL)
| + while(1)
| {
| struct id id;
| err_t ugh;
| @@ -1277,7 +1280,7 @@ osw_process_secret_records(struct secret **psecrets,
int verbose,
| /* found key part */
| shift(); /* discard explicit separator */
| process_secret(psecrets, verbose, s, pass);
| - s = NULL;
| + //s = NULL;
| break;
| }
| | @@ -1330,9 +1333,11 @@ osw_process_secret_records(struct secret
**psecrets, int verbose,
| /* unexpected Record Boundary or EOF */
| loglog(RC_LOG_SERIOUS, "\"%s\" line %d: unexpected end of
id list"
| , flp->filename, flp->lino);
| + pfree(s);
| break;
| }
| }
| + }
| }
| }
| }
This doesn't look right. s has substructure that is allocated, so a
simple pfree(s) won't do the trick.
The original code doesn't look right either: s isn't freed.
| diff --git a/lib/libopenswan/x509dn.c b/lib/libopenswan/x509dn.c
| index c1658c5..7a0c96a 100644
| --- a/lib/libopenswan/x509dn.c
| +++ b/lib/libopenswan/x509dn.c
| @@ -1583,10 +1583,11 @@ gntoid(struct id *id, const generalName_t *gn)
| case GN_IP_ADDRESS: /* ID type: ID_IPV4_ADDR */
| {
| const struct af_info *afi = &af_inet4_info;
| - err_t ugh = NULL;
| + //err_t ugh = NULL;
| | id->kind = afi->id_addr;
| - ugh = initaddr(gn->name.ptr, gn->name.len, afi->af, &id->ip_addr);
| + //ugh = initaddr(gn->name.ptr, gn->name.len, afi->af,
&id->ip_addr);
| + initaddr(gn->name.ptr, gn->name.len, afi->af, &id->ip_addr);
| }
| break;
| case GN_RFC822_NAME: /* ID type: ID_USER_FQDN */
I admit that ugh isn't used. so not setting it seems like an
improvement. Better yet: I think that it should be used in some kind
of diagnostic. I admit that I don't know the context.
| diff --git a/lib/libpluto/readwhackmsg.c b/lib/libpluto/readwhackmsg.c
| index 6f27e9c..4735177 100644
| --- a/lib/libpluto/readwhackmsg.c
| +++ b/lib/libpluto/readwhackmsg.c
| @@ -42,12 +42,14 @@ void readwhackmsg(char *infile)
| if(abuflen > sizeof(m1)) {
| fprintf(stderr, "whackmsg file has too big a record=%zu > %zu\n"
| , abuflen, sizeof(m1));
| + fclose(record);
| exit(6);
| }
| | if((iocount=fread(&m1, abuflen, 1, record)) != 1) {
| if(feof(record)) break;
| perror(infile);
| + fclose(record);
| exit(5);
| }
|
| @@ -79,6 +81,7 @@ void readwhackmsg(char *infile)
| if(iocount != 0 || !feof(record)) {
| perror(infile);
| }
| + fclose(record);
| }
Looks like a fix. Perhaps there is a tidier way of fixing this. Mind
you, fclose before exit looks unnecessary. So only the last new
fclose is important.
| /*
| diff --git a/programs/addconn/addconn.c b/programs/addconn/addconn.c
| index b91f7ea..f423e91 100644
| --- a/programs/addconn/addconn.c
| +++ b/programs/addconn/addconn.c
| @@ -260,8 +260,10 @@ main(int argc, char *argv[])
| configfile, err);
| exit(3);
| }
| - else if(checkconfig)
| + else if(checkconfig) {
| + confread_free(cfg);
| exit(0);
| + }
| | if(defaultroute) {
| err_t e;
| @@ -343,6 +345,7 @@ main(int argc, char *argv[])
| char *sep="";
| if((argc-optind) < 2 ) {
| printf("%s_confreadstatus=failed\n", varprefix);
| + confread_free(cfg);
| exit(3);
| }
| | @@ -364,6 +367,7 @@ main(int argc, char *argv[])
| }
| }
| printf("\"\n");
| + confread_free(cfg);
| exit(0);
| | } else if(typeexport) {
| @@ -412,6 +416,7 @@ main(int argc, char *argv[])
| }
| }
| | + confread_free(cfg);
| exit(0);
| | } else {
| @@ -481,6 +486,7 @@ main(int argc, char *argv[])
| if(verbose) printf("\n");
| }
| | + confread_free(cfg);
| exit(exit_status);
| }
Free before exit seems unimportant. Might make for nicer
leak-detective reports.
| diff --git a/programs/readwriteconf/readwriteconf.c
b/programs/readwriteconf/readwriteconf.c
| index b781e30..78df8ca 100644
| --- a/programs/readwriteconf/readwriteconf.c
| +++ b/programs/readwriteconf/readwriteconf.c
| @@ -185,7 +185,7 @@ main(int argc, char *argv[])
| }
| | confwrite(cfg, stdout);
| -
| + confread_free(cfg);
| exit(0);
| }
Free before exit seems unimportant. Might make for nicer
leak-detective reports.
More information about the Dev
mailing list