[lttng-dev] [PATCH lttng-tools] Fix: channel names are not validated

[Jérémie Galarneau]

Merged all the way to 2.4 with a couple of minor changes.

Read on for comments.

On Thu, Nov 27, 2014 at 5:35 PM, Philippe Proulx
<eeppeliteloop at gmail.com> wrote:
> This patch ensures:
>
>   1. A channel name does not contain any '/' character, since
>      relative paths may be injected in the channel name
>      otherwise (knowing that the channel name is eventually
>      part of a file name)
>   2. A channel name does not start with a '.' character, since
>      trace readers (Babeltrace is one of them) could interpret
>      files starting with a dot as hidden files and ignore
>      them when opening the CTF trace
>
> Fixes: #751
>
> Signed-off-by: Philippe Proulx <eeppeliteloop at gmail.com>
> ---
>  include/lttng/lttng-error.h              |  1 +
>  src/bin/lttng-sessiond/cmd.c             |  6 ++++++
>  src/bin/lttng/commands/enable_channels.c | 20 +++++++++++++++++---
>  src/common/error.c                       |  1 +
>  4 files changed, 25 insertions(+), 3 deletions(-)
>
> diff --git a/include/lttng/lttng-error.h b/include/lttng/lttng-error.h
> index 1220e48..efdca65 100644
> --- a/include/lttng/lttng-error.h
> +++ b/include/lttng/lttng-error.h
> @@ -133,6 +133,7 @@ enum lttng_error_code {
>         LTTNG_ERR_EXCLUSION_INVAL        = 110, /* Invalid event exclusion data */
>         LTTNG_ERR_EXCLUSION_NOMEM        = 111, /* Lack of memory while processing event exclusions */
>         LTTNG_ERR_INVALID_EVENT_NAME     = 112, /* Invalid event name */
> +       LTTNG_ERR_INVALID_CHANNEL_NAME   = 113, /* Invalid channel name */
>
>         /* MUST be last element */
>         LTTNG_ERR_NR,                           /* Last element */
> diff --git a/src/bin/lttng-sessiond/cmd.c b/src/bin/lttng-sessiond/cmd.c
> index 33ab492..1ede96f 100644
> --- a/src/bin/lttng-sessiond/cmd.c
> +++ b/src/bin/lttng-sessiond/cmd.c
> @@ -18,6 +18,7 @@
>  #define _GNU_SOURCE
>  #define _LGPL_SOURCE
>  #include <assert.h>
> +#include <string.h>
>  #include <inttypes.h>
>  #include <urcu/list.h>
>  #include <urcu/uatomic.h>
> @@ -944,6 +945,11 @@ int cmd_enable_channel(struct ltt_session *session,
>         assert(attr);
>         assert(domain);
>
> +       /* Validate channel name */
> +       if (attr->name[0] == '.' || strchr(attr->name, '/') != NULL) {

Changed strchr() to memchr() which takes a length argument.

> +               return LTTNG_ERR_INVALID_CHANNEL_NAME;

changed this to
ret = LTTNG_ERR_INVALID_CHANNEL_NAME;
goto end; (end is _after_ the rcu_read_unlock())

We prefer having a single exit point in functions (whenever it makes sense).

> +       }
> +
>         DBG("Enabling channel %s for session %s", attr->name, session->name);
>
>         rcu_read_lock();
> diff --git a/src/bin/lttng/commands/enable_channels.c b/src/bin/lttng/commands/enable_channels.c
> index f8272e9..b49c20d 100644
> --- a/src/bin/lttng/commands/enable_channels.c
> +++ b/src/bin/lttng/commands/enable_channels.c
> @@ -275,9 +275,16 @@ static int enable_channel(char *session_name)
>         /* Strip channel list (format: chan1,chan2,...) */
>         channel_name = strtok(opt_channels, ",");
>         while (channel_name != NULL) {
> -               /* Copy channel name and normalize it */
> -               strncpy(chan.name, channel_name, NAME_MAX);
> -               chan.name[NAME_MAX - 1] = '\0';
> +               /* Validate channel name's length */
> +               if (strlen(channel_name) >= NAME_MAX) {
> +                       ERR("Channel name is too long (max. %u characters)",
> +                           NAME_MAX - 1);

Please use tabs for indents.

> +                       error = 1;
> +                       goto skip_enable;
> +               }
> +
> +               /* Copy channel name */
> +               strcpy(chan.name, channel_name);
>
>                 DBG("Enabling channel %s", channel_name);
>
> @@ -292,6 +299,12 @@ static int enable_channel(char *session_name)
>                                                 lttng_strerror(ret), session_name);
>                                 warn = 1;
>                                 break;
> +                       case LTTNG_ERR_INVALID_CHANNEL_NAME:
> +                               ERR("Invalid channel name: \"%s\". "
> +                                   "Channel names may not start with '.', and "
> +                                   "may not contain '/'.", channel_name);

Use tabs to indent.

Thanks!
Jérémie

> +                               error = 1;
> +                               break;
>                         default:
>                                 ERR("Channel %s: %s (session %s)", channel_name,
>                                                 lttng_strerror(ret), session_name);
> @@ -304,6 +317,7 @@ static int enable_channel(char *session_name)
>                         success = 1;
>                 }
>
> +skip_enable:
>                 if (lttng_opt_mi) {
>                         /* Mi print the channel element and leave it open */
>                         ret = mi_lttng_channel(writer, &chan, 1);
> diff --git a/src/common/error.c b/src/common/error.c
> index 2cebbf9..d3e3b94 100644
> --- a/src/common/error.c
> +++ b/src/common/error.c
> @@ -166,6 +166,7 @@ static const char *error_string_array[] = {
>         [ ERROR_INDEX(LTTNG_ERR_MI_IO_FAIL) ] = "IO error while writing MI output",
>         [ ERROR_INDEX(LTTNG_ERR_MI_NOT_IMPLEMENTED) ] = "Mi feature not implemented",
>         [ ERROR_INDEX(LTTNG_ERR_INVALID_EVENT_NAME) ] = "Invalid event name",
> +       [ ERROR_INDEX(LTTNG_ERR_INVALID_CHANNEL_NAME) ] = "Invalid channel name",
>
>         /* Last element */
>         [ ERROR_INDEX(LTTNG_ERR_NR) ] = "Unknown error code"
> --
> 2.1.3
>
>
> _______________________________________________
> lttng-dev mailing list
> lttng-dev at lists.lttng.org
> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev



-- 
Jérémie Galarneau
EfficiOS Inc.
http://www.efficios.com