Mailing List Archive

[PATCH 1/2 v2] terminate config reading on EOT/Ctl-D instead of just on pipe close
vpnc's config file processing logic uses EOF to determine when to stop
processing the config input, but if stdin is actually a pipe from a
controlling process, EOF only happens if the pipe is closed. Which
means the controlling process can't respond to any interactive requests
for information. So we need to add some other mechanism to indicate
that config processing is done that does not rely on closing stdin to
indicate this.

Also, getline() only returns on EOF (which has the problems described
above) or when it encounters sufficient newline characters;
unfortunately this precludes using getline() to handle single bytes.
Switch to fgetc() and build up the line ourselves so that we can
recognize a custom EOF character, EOT (Ctl-D).
---
trunk/config.c | 60 +++++++++++++++++++++++++++++++++++++++++++++------
trunk/vpnc.8.template | 10 +++++++++
2 files changed, 63 insertions(+), 7 deletions(-)

diff --git a/trunk/config.c b/trunk/config.c
index 0351e9b..c7776ae 100644
--- a/trunk/config.c
+++ b/trunk/config.c
@@ -24,14 +24,15 @@
#include <stdio.h>
#include <stdlib.h>
#include <stdarg.h>
#include <unistd.h>
#include <string.h>
#include <errno.h>
#include <sys/utsname.h>
+#include <sys/ttydefaults.h>

#include <gcrypt.h>

#include "sysdep.h"
#include "config.h"
#include "vpnc.h"
#include "supp.h"
@@ -93,14 +94,51 @@ void hex_dump(const char *str, const void *data, ssize_t len, const struct debug
else if (i && !(i % 4))
printf(" ");
printf("%02x", p[i]);
}
printf("\n");
}

+#define GETLINE_MAX_BUFLEN 200
+
+/* mostly match getline() semantics but:
+ * 1) accept CEOT (Ctrl-D, 0x04) as a line terminator
+ * 2) always allocate the buffer
+ * 3) max line size of 200 bytes
+ */
+static char *vpnc_getline(size_t *out_size, FILE *stream)
+{
+ char *buf;
+ const size_t buflen = GETLINE_MAX_BUFLEN;
+ size_t llen = 0;
+ int c;
+
+ buf = malloc(buflen);
+ if (buf == NULL)
+ return NULL;
+
+ /* Read a line from the input */
+ while (llen < buflen - 1) {
+ c = fgetc (stream);
+ if (c == EOF || feof (stream)) {
+ if (llen == 0) {
+ free(buf);
+ return NULL;
+ }
+ break;
+ }
+ buf[llen++] = (char) c;
+ if ((char) c == '\n' || (char) c == '\r' || (char) c == CEOT)
+ break;
+ }
+ buf[llen] = '\0';
+ *out_size = llen;
+ return buf;
+}
+
static void config_deobfuscate(int obfuscated, int clear)
{
int ret, len = 0;
char *bin = NULL;

if (config[obfuscated] == NULL)
return;
@@ -481,15 +519,14 @@ static char *get_config_filename(const char *name, int add_dot_conf)
return realname;
}

static void read_config_file(const char *name, const char **configs, int missingok)
{
FILE *f;
char *line = NULL;
- size_t line_length = 0;
int linenum = 0;
char *realname;

if (!strcmp(name, "-")) {
f = stdin;
realname = strdup("stdin");
} else {
@@ -504,22 +541,30 @@ static void read_config_file(const char *name, const char **configs, int missing
free(realname);
return;
}
if (f == NULL)
error(1, errno, "couldn't open `%s'", realname);
}
for (;;) {
- ssize_t llen;
+ size_t llen = 0;
int i;

- llen = getline(&line, &line_length, f);
- if (llen == -1 && feof(f))
+ line = vpnc_getline(&llen, f);
+ if (line == NULL) {
+ if (feof (f))
+ break;
+
+ if (ferror (f))
+ error(1, errno, "reading `%s'", realname);
+ error(1, errno, "out of memory reading `%s'", realname);
+ }
+
+ /* EOT terminates reading configuration in addition to EOF */
+ if (llen == 1 && line[0] == CEOT)
break;
- if (llen == -1)
- error(1, errno, "reading `%s'", realname);
if (llen > 0 && line[llen - 1] == '\n')
line[--llen] = 0;
if (llen > 0 && line[llen - 1] == '\r')
line[--llen] = 0;
linenum++;
for (i = 0; config_names[i].name != NULL; i++) {
if (strncasecmp(config_names[i].name, line,
@@ -556,14 +601,15 @@ static void read_config_file(const char *name, const char **configs, int missing
error(1, errno, "can't allocate memory");
break;
}
}
if (config_names[i].name == NULL && line[0] != '#' && line[0] != 0)
error(0, 0, "warning: unknown configuration directive in %s at line %d",
realname, linenum);
+ free(line);
}
free(line);
free(realname);
if (strcmp(name, "-"))
fclose(f);
}

@@ -820,15 +866,15 @@ void do_config(int argc, char **argv)
case CONFIG_IPSEC_SECRET:
case CONFIG_XAUTH_PASSWORD:
s = strdup(getpass(""));
break;
case CONFIG_IPSEC_GATEWAY:
case CONFIG_IPSEC_ID:
case CONFIG_XAUTH_USERNAME:
- getline(&s, &s_len, stdin);
+ s = vpnc_getline(&s_len, stdin);
}
if (s != NULL && strlen(s) > 0 && s[strlen(s) - 1] == '\n')
s[strlen(s) - 1] = 0;
config[i] = s;
}

if (print_config) {
diff --git a/trunk/vpnc.8.template b/trunk/vpnc.8.template
index 38b3319..c059da9 100644
--- a/trunk/vpnc.8.template
+++ b/trunk/vpnc.8.template
@@ -75,14 +75,24 @@ If no configuration file
is specified on the command-line
at all, both
.B /etc/vpnc/default.conf
and
.B /etc/vpnc.conf
will be loaded.

+.PP
+
+Additionally, if the configuration
+file "-" is specified on the command-line
+vpnc will read configuration from
+stdin. The configuration is parsed and
+the connection proceeds when stdin is
+closed or the special character CEOT
+(CTRL-D) is read.
+
.SH OPTIONS
The program options can be either given as arguments (but not all of them
for security reasons) or be stored in a configuration file.
.PD 0
.\" ###makeman.pl: Insert options from help-output here!

.HP
--
1.8.3.1


_______________________________________________
vpnc-devel mailing list
vpnc-devel@unix-ag.uni-kl.de
https://lists.unix-ag.uni-kl.de/mailman/listinfo/vpnc-devel
http://www.unix-ag.uni-kl.de/~massar/vpnc/
Re: [PATCH 1/2 v2] terminate config reading on EOT/Ctl-D instead of just on pipe close [ In reply to ]
On Sat, Dec 14, 2013 at 2:41 AM, Dan Williams <dcbw@redhat.com> wrote:
> vpnc's config file processing logic uses EOF to determine when to stop
> processing the config input, but if stdin is actually a pipe from a
> controlling process, EOF only happens if the pipe is closed. Which
> means the controlling process can't respond to any interactive requests
> for information. So we need to add some other mechanism to indicate
> that config processing is done that does not rely on closing stdin to
> indicate this.
>
> Also, getline() only returns on EOF (which has the problems described
> above) or when it encounters sufficient newline characters;
> unfortunately this precludes using getline() to handle single bytes.
> Switch to fgetc() and build up the line ourselves so that we can
> recognize a custom EOF character, EOT (Ctl-D).
> ---

Good, just few minor fix that I will adjust by myself during commit.
If no complain, would commit shortly.

> trunk/config.c | 60 +++++++++++++++++++++++++++++++++++++++++++++------
> trunk/vpnc.8.template | 10 +++++++++
> 2 files changed, 63 insertions(+), 7 deletions(-)
>
> diff --git a/trunk/config.c b/trunk/config.c
> index 0351e9b..c7776ae 100644
> --- a/trunk/config.c
> +++ b/trunk/config.c
> @@ -24,14 +24,15 @@
> #include <stdio.h>
> #include <stdlib.h>
> #include <stdarg.h>
> #include <unistd.h>
> #include <string.h>
> #include <errno.h>
> #include <sys/utsname.h>
> +#include <sys/ttydefaults.h>
>
> #include <gcrypt.h>
>
> #include "sysdep.h"
> #include "config.h"
> #include "vpnc.h"
> #include "supp.h"
> @@ -93,14 +94,51 @@ void hex_dump(const char *str, const void *data, ssize_t len, const struct debug
> else if (i && !(i % 4))
> printf(" ");
> printf("%02x", p[i]);
> }
> printf("\n");
> }
>
> +#define GETLINE_MAX_BUFLEN 200
> +
> +/* mostly match getline() semantics but:
> + * 1) accept CEOT (Ctrl-D, 0x04) as a line terminator
> + * 2) always allocate the buffer
> + * 3) max line size of 200 bytes
> + */
> +static char *vpnc_getline(size_t *out_size, FILE *stream)
> +{
> + char *buf;
> + const size_t buflen = GETLINE_MAX_BUFLEN;
> + size_t llen = 0;
> + int c;
> +
> + buf = malloc(buflen);
> + if (buf == NULL)
> + return NULL;
> +
> + /* Read a line from the input */
> + while (llen < buflen - 1) {
> + c = fgetc (stream);

remove space between function name and parenthesis

> + if (c == EOF || feof (stream)) {

same as above

> + if (llen == 0) {
> + free(buf);
> + return NULL;
> + }
> + break;
> + }
> + buf[llen++] = (char) c;
> + if ((char) c == '\n' || (char) c == '\r' || (char) c == CEOT)
> + break;
> + }
> + buf[llen] = '\0';
> + *out_size = llen;
> + return buf;
> +}
> +
> static void config_deobfuscate(int obfuscated, int clear)
> {
> int ret, len = 0;
> char *bin = NULL;
>
> if (config[obfuscated] == NULL)
> return;
> @@ -481,15 +519,14 @@ static char *get_config_filename(const char *name, int add_dot_conf)
> return realname;
> }
>
> static void read_config_file(const char *name, const char **configs, int missingok)
> {
> FILE *f;
> char *line = NULL;
> - size_t line_length = 0;
> int linenum = 0;
> char *realname;
>
> if (!strcmp(name, "-")) {
> f = stdin;
> realname = strdup("stdin");
> } else {
> @@ -504,22 +541,30 @@ static void read_config_file(const char *name, const char **configs, int missing
> free(realname);
> return;
> }
> if (f == NULL)
> error(1, errno, "couldn't open `%s'", realname);
> }
> for (;;) {
> - ssize_t llen;
> + size_t llen = 0;
> int i;
>
> - llen = getline(&line, &line_length, f);
> - if (llen == -1 && feof(f))
> + line = vpnc_getline(&llen, f);
> + if (line == NULL) {
> + if (feof (f))

ditto

> + break;
> +
> + if (ferror (f))

ditto

> + error(1, errno, "reading `%s'", realname);
> + error(1, errno, "out of memory reading `%s'", realname);
> + }
> +
> + /* EOT terminates reading configuration in addition to EOF */
> + if (llen == 1 && line[0] == CEOT)
> break;
> - if (llen == -1)
> - error(1, errno, "reading `%s'", realname);
> if (llen > 0 && line[llen - 1] == '\n')
> line[--llen] = 0;
> if (llen > 0 && line[llen - 1] == '\r')
> line[--llen] = 0;
> linenum++;
> for (i = 0; config_names[i].name != NULL; i++) {
> if (strncasecmp(config_names[i].name, line,
> @@ -556,14 +601,15 @@ static void read_config_file(const char *name, const char **configs, int missing
> error(1, errno, "can't allocate memory");
> break;
> }
> }
> if (config_names[i].name == NULL && line[0] != '#' && line[0] != 0)
> error(0, 0, "warning: unknown configuration directive in %s at line %d",
> realname, linenum);
> + free(line);
> }
> free(line);
> free(realname);
> if (strcmp(name, "-"))
> fclose(f);
> }
>
> @@ -820,15 +866,15 @@ void do_config(int argc, char **argv)
> case CONFIG_IPSEC_SECRET:
> case CONFIG_XAUTH_PASSWORD:
> s = strdup(getpass(""));
> break;
> case CONFIG_IPSEC_GATEWAY:
> case CONFIG_IPSEC_ID:
> case CONFIG_XAUTH_USERNAME:
> - getline(&s, &s_len, stdin);
> + s = vpnc_getline(&s_len, stdin);
> }
> if (s != NULL && strlen(s) > 0 && s[strlen(s) - 1] == '\n')
> s[strlen(s) - 1] = 0;
> config[i] = s;
> }
>
> if (print_config) {
> diff --git a/trunk/vpnc.8.template b/trunk/vpnc.8.template
> index 38b3319..c059da9 100644
> --- a/trunk/vpnc.8.template
> +++ b/trunk/vpnc.8.template
> @@ -75,14 +75,24 @@ If no configuration file
> is specified on the command-line
> at all, both
> .B /etc/vpnc/default.conf
> and
> .B /etc/vpnc.conf
> will be loaded.
>
> +.PP
> +
> +Additionally, if the configuration
> +file "-" is specified on the command-line
> +vpnc will read configuration from
> +stdin. The configuration is parsed and
> +the connection proceeds when stdin is
> +closed or the special character CEOT
> +(CTRL-D) is read.
> +

I'm not a native English speaker. I would write it in a different way
but, since not qualified, I'll keep it in this way.

Then, my proposal for replacing getpass() uses fgets().
Of course, would be better to use your vpnc_getline(). Will check it.

Thanks,
Antonio

> .SH OPTIONS
> The program options can be either given as arguments (but not all of them
> for security reasons) or be stored in a configuration file.
> .PD 0
> .\" ###makeman.pl: Insert options from help-output here!
>
> .HP
> --
> 1.8.3.1
>
>
_______________________________________________
vpnc-devel mailing list
vpnc-devel@unix-ag.uni-kl.de
https://lists.unix-ag.uni-kl.de/mailman/listinfo/vpnc-devel
http://www.unix-ag.uni-kl.de/~massar/vpnc/
Re: [PATCH 1/2 v2] terminate config reading on EOT/Ctl-D instead of just on pipe close [ In reply to ]
On Sun, Dec 15, 2013 at 1:14 PM, Antonio Borneo
<borneo.antonio@gmail.com> wrote:
> On Sat, Dec 14, 2013 at 2:41 AM, Dan Williams <dcbw@redhat.com> wrote:
>> +/* mostly match getline() semantics but:
>> + * 1) accept CEOT (Ctrl-D, 0x04) as a line terminator

Dan,
another point.
CEOT is not a line terminator.
It is supposed to be recognized as End Of Transmission only if it
appears as first char of a line.
Should this request be part of the patch? Would it work in your system?

Antonio
_______________________________________________
vpnc-devel mailing list
vpnc-devel@unix-ag.uni-kl.de
https://lists.unix-ag.uni-kl.de/mailman/listinfo/vpnc-devel
http://www.unix-ag.uni-kl.de/~massar/vpnc/
Re: [PATCH 1/2 v2] terminate config reading on EOT/Ctl-D instead of just on pipe close [ In reply to ]
On Sun, 2013-12-15 at 15:29 +0800, Antonio Borneo wrote:
> On Sun, Dec 15, 2013 at 1:14 PM, Antonio Borneo
> <borneo.antonio@gmail.com> wrote:
> > On Sat, Dec 14, 2013 at 2:41 AM, Dan Williams <dcbw@redhat.com> wrote:
> >> +/* mostly match getline() semantics but:
> >> + * 1) accept CEOT (Ctrl-D, 0x04) as a line terminator
>
> Dan,
> another point.
> CEOT is not a line terminator.
> It is supposed to be recognized as End Of Transmission only if it
> appears as first char of a line.
> Should this request be part of the patch? Would it work in your system?

I'm fine with modifying the patch to only accept EOT at the start of a
line. I can modify my system any way we'd like, since it needs to match
what upstream vpnc expects. I will submit a revised patch.

Dan

_______________________________________________
vpnc-devel mailing list
vpnc-devel@unix-ag.uni-kl.de
https://lists.unix-ag.uni-kl.de/mailman/listinfo/vpnc-devel
http://www.unix-ag.uni-kl.de/~massar/vpnc/