On Fri, Sep 12, 2014 at 12:38:07AM -0700, Doug Hogan wrote:
> Hmm this doesn't look right to me. ressl_config is not allocated the
> same way as the other variables. The other variables such as ssl,
> sslhost, etc are local to that function and allocated there.
> ressl_config is a global and only allocated in main.c. It is allocated
> once and configured using getopt/getsubopt().
>
> If you free it, you would need to reallocate it each time or set it to
> NULL so libressl will revert back and discard the user's changes.
> I think either way is wrong. The SSL configuration should be retained
> even if a single url_get() fails (which isn't fatal).
>
> The above code is from url_get() which is called in a loop inside
> auto_fetch(). I think the above change would use the getsubopt()
> configuration for the first HTTPS URL argument and then try to use freed
> memory for the others.
Of course, you're right. Is there any reason why ressl_config must be
global? It's only used in url_get. It should be harmless to create a
new one each time as long as we always free it.
Here's a diff for that:
Index: fetch.c
===================================================================
RCS file: /work/cvsroot/src/usr.bin/ftp/fetch.c,v
retrieving revision 1.129
diff -p -u -r1.129 fetch.c
--- fetch.c 25 Aug 2014 15:36:00 -0000 1.129
+++ fetch.c 12 Sep 2014 14:23:51 -0000
@@ -192,6 +192,7 @@ url_get(const char *origline, const char
char *locbase, *full_host = NULL;
const char *scheme;
int ishttpurl = 0, ishttpsurl = 0;
+ struct ressl_config *ressl_config = NULL;
#endif /* !SMALL */
struct ressl *ssl = NULL;
int status;
@@ -605,6 +606,26 @@ again:
fprintf(ttyout, "failed to create SSL client\n");
goto cleanup_url_get;
}
+ if ((ressl_config = ressl_config_new()) == NULL) {
+ fprintf(ttyout, "failed to create ReSSL config\n");
+ goto cleanup_url_get;
+ }
+
+ if (ssl_cafile != NULL)
+ ressl_config_set_ca_file(ressl_config, ssl_cafile);
+ if (ssl_capath != NULL)
+ ressl_config_set_ca_path(ressl_config, ssl_capath);
+ if (ssl_ciphers != NULL)
+ ressl_config_set_ciphers(ressl_config, ssl_ciphers);
+
+ if (ssl_verify)
+ ressl_config_verify(ressl_config);
+ else
+ ressl_config_insecure_no_verify(ressl_config);
+
+ if (ssl_verify_depth)
+ ressl_config_set_verify_depth(ressl_config,
ssl_verify_depth);
+
if (ressl_configure(ssl, ressl_config) != 0) {
fprintf(ttyout, "SSL configuration failure: %s\n",
ressl_error(ssl));
@@ -978,6 +999,7 @@ cleanup_url_get:
#ifndef SMALL
if (ssl != NULL) {
ressl_close(ssl);
+ ressl_config_free(ressl_config);
ressl_free(ssl);
}
free(full_host);
Index: ftp_var.h
===================================================================
RCS file: /work/cvsroot/src/usr.bin/ftp/ftp_var.h,v
retrieving revision 1.35
diff -p -u -r1.35 ftp_var.h
--- ftp_var.h 14 Jul 2014 09:26:27 -0000 1.35
+++ ftp_var.h 12 Sep 2014 13:19:34 -0000
@@ -234,5 +234,9 @@ FILE *ttyout; /* stdout or stderr,
depe
extern struct cmd cmdtab[];
#ifndef SMALL
-extern struct ressl_config *ressl_config;
+char *ssl_cafile;
+char *ssl_capath;
+char *ssl_ciphers;
+int ssl_verify;
+int ssl_verify_depth;
#endif /* !SMALL */
Index: main.c
===================================================================
RCS file: /work/cvsroot/src/usr.bin/ftp/main.c,v
retrieving revision 1.92
diff -p -u -r1.92 main.c
--- main.c 16 Jul 2014 04:52:43 -0000 1.92
+++ main.c 12 Sep 2014 13:38:02 -0000
@@ -76,8 +76,6 @@
#include <string.h>
#include <unistd.h>
-#include <ressl.h>
-
#include "cmds.h"
#include "ftp_var.h"
@@ -97,8 +95,6 @@ char * const ssl_verify_opts[] = {
"depth",
NULL
};
-
-struct ressl_config *ressl_config;
#endif /* !SMALL */
int family = PF_UNSPEC;
@@ -309,12 +305,6 @@ main(volatile int argc, char *argv[])
case 'S':
#ifndef SMALL
- if (ressl_config == NULL) {
- ressl_config = ressl_config_new();
- if (ressl_config == NULL)
- errx(1, "ressl config failed");
- }
-
cp = optarg;
while (*cp) {
char *str;
@@ -322,28 +312,24 @@ main(volatile int argc, char *argv[])
case SSL_CAFILE:
if (str == NULL)
errx(1, "missing CA file");
- ressl_config_set_ca_file(ressl_config,
- str);
+ ssl_cafile = str;
break;
case SSL_CAPATH:
if (str == NULL)
errx(1, "missing CA directory"
" path");
- ressl_config_set_ca_path(ressl_config,
- str);
+ ssl_capath = str;
break;
case SSL_CIPHERS:
if (str == NULL)
errx(1, "missing cipher list");
- ressl_config_set_ciphers(ressl_config,
- str);
+ ssl_ciphers = str;
break;
case SSL_DONTVERIFY:
- ressl_config_insecure_no_verify(
- ressl_config);
+ ssl_verify = 0;
break;
case SSL_DOVERIFY:
- ressl_config_verify(ressl_config);
+ ssl_verify = 1;
break;
case SSL_VERIFYDEPTH:
if (str == NULL)
@@ -354,8 +340,7 @@ main(volatile int argc, char *argv[])
errx(1, "certificate "
"validation depth is %s",
errstr);
- ressl_config_set_verify_depth(
- ressl_config, (int)depth);
+ ssl_verify_depth = (int)depth;
break;
default:
errx(1, "unknown -S suboption `%s'",
