Re: [nmap-svn] r31378 - nmap-exp/d33tah/ncat-env-conninfo/ncat

[Henri Doreau <henri.doreau () gmail com>]

Hi,

comments inline.

2013/7/17  <commit-mailer () nmap org>:
> Author: d33tah
> Date: Wed Jul 17 01:08:11 2013
> New Revision: 31378
>
> Log:
> add some Windows code. (how about moving the setup_environment to, say, ncat_core.c and using some #defines to make 
> addenv  have the same interface as setenv?)
That sounds good to me.

> Modified:
>    nmap-exp/d33tah/ncat-env-conninfo/ncat/ncat_exec_win.c
>
> Modified: nmap-exp/d33tah/ncat-env-conninfo/ncat/ncat_exec_win.c
> ==============================================================================
> --- nmap-exp/d33tah/ncat-env-conninfo/ncat/ncat_exec_win.c      (original)
> +++ nmap-exp/d33tah/ncat-env-conninfo/ncat/ncat_exec_win.c      Wed Jul 17 01:08:11 2013
> @@ -195,6 +195,72 @@
>      ncat_assert(rc != 0);
>  }
>
> +/* Add a new environment variable to the string, reallocating it and
> +   ovewriting its size for future use. */
> +static void addenv(LPTSTR* env, int *env_size, LPTSTR name, LPTSTR value) {
Please set the appropriate const qualifiers.

> +    /* We'll need two more bytes - one for \0 finishing the pair, the other
> +       finishing the whole set. */
> +    int added_value_len = strlen(name) + strlen(value) + 2;
> +
size_t added_value_len = ...

> +    *env = (LPTSTR) safe_realloc(*env, *env_size+added_value_len);
> +
> +    Snprintf(*env + *env_size-1, added_value_len, "%s=%s", name, value);
> +
> +    *env_size += added_value_len;
> +    (*env)[*env_size] = '\0';
> +    (*env)[*env_size-1] = '\0';
Why do you zero the last *two* indices? Safety? You might want to
check/use Snprintf return value too.

> +}
> +
> +static void setup_environment(struct fdinfo *info, LPTSTR* env, int *env_size)
> +{
> +    char *dest_addr=NULL;
> +    char dest_port[10];
> +    union sockaddr_u su;
> +    unsigned short port;
> +    char str[16];
> +    char ip[4 * INET6_ADDRSTRLEN];
Why is ip 4*INET6_ADDRSTRLEN?

> +    socklen_t alen = sizeof(su);
> +
> +    if (getpeername(info->fd, &su.sockaddr, &alen) != 0) {
> +        bye("getpeername failed: %s", strerror(errno));
If I read that correctly, this can also run on windows. On windows the
error code should be retrieved via WSAGetLastError instead of errno.
Use nbase wrappers socket_errno and socket_strerror.

> +    }
> +
> +    dest_addr = (char*) inet_socktop(&su);
(style) (char *)inet_socktop

> +    port = inet_port(&su);
> +    sprintf(dest_port, "%d", port);
> +
> +    addenv(env, env_size, "NCAT_REMOTE_ADDR", dest_addr);
> +    addenv(env, env_size, "NCAT_REMOTE_PORT", dest_port);
> +
> +    if (o.ssl)
> +        addenv(env, env_size, "NCAT_SSL", "1");
> +
> +    switch(o.proto) {
> +        case IPPROTO_TCP:
> +            addenv(env, env_size, "NCAT_PROTO", "TCP");
> +            break;
> +        case IPPROTO_SCTP:
> +            addenv(env, env_size, "NCAT_PROTO", "SCTP");
> +            break;
> +        case IPPROTO_UDP:
> +            addenv(env, env_size, "NCAT_PROTO", "UDP");
> +            break;
> +    }
> +
> +    if (getsockname (info->fd, (struct sockaddr *) &su, &alen) < 0) {
> +        bye("getsockname failed: %s", strerror(errno));
> +    }
> +
> +    if (getnameinfo ((struct sockaddr *) &su, alen, ip, sizeof (ip),
> +            str, sizeof (str), NI_NUMERICHOST | NI_NUMERICSERV)==0) {
> +        addenv(env, env_size, "NCAT_LOCAL_HOST", ip);
> +        addenv(env, env_size, "NCAT_LOCAL_PORT", str);
> +    } else {
> +        bye("getsockname failed: %s", strerror(errno));
getnameinfo() doesn't set errno but returns the error code directly.

> +    }
> +}
> +
> +
>  /* Run a command and redirect its input and output handles to a pair of
>     anonymous pipes.  The process handle and pipe handles are returned in the
>     info struct. Returns the PID of the new process, or -1 on error. */
> @@ -206,6 +272,26 @@
>      SECURITY_ATTRIBUTES sa;
>      STARTUPINFO si;
>      PROCESS_INFORMATION pi;
> +    int env_size;
> +    LPTSTR lpszVariable, new_env, lpvEnv;
> +
> +    lpszVariable = lpvEnv = (LPTSTR)GetEnvironmentStrings();
> +    if (lpvEnv == NULL) {
> +        bye("GetEnvironmentStrings failed (%d)\n", GetLastError());
> +    }
> +
> +    /* Calculate the size of the environment. */
> +    while (*lpszVariable) {
> +        lpszVariable += lstrlen(lpszVariable) + 1;
> +    }
> +
> +    /* + 1 is for the second '\0' the while loop'll miss */
> +    env_size = lpszVariable - lpvEnv + 1;
> +
> +    new_env = (LPTSTR) safe_malloc(env_size);
> +    memcpy(new_env, lpvEnv, env_size);
> +    FreeEnvironmentStrings(lpvEnv);
> +    setup_environment(&info->fdn, &new_env, &env_size);
>
>      /* Make the pipe handles inheritable. */
>      sa.nLength = sizeof(sa);
> @@ -261,7 +347,7 @@
>
>      memset(&pi, 0, sizeof(pi));
>
> -    if (CreateProcess(NULL, cmdexec, NULL, NULL, TRUE, 0, NULL, NULL, &si, &pi) == 0) {
> +    if (CreateProcess(NULL, cmdexec, NULL, NULL, TRUE, 0, new_env, NULL, &si, &pi) == 0) {
>          if (o.verbose)
>              logdebug("Error in CreateProcess: %d\n", GetLastError());
>          CloseHandle(info->child_in_r);

Also, I'm nitpicking but even though your error messages look
consistent in this patch, I saw other ones with parenthesis after
function name. Try to stick to a single format (whichever you like).

--
Henri
_______________________________________________
Sent through the dev mailing list
http://nmap.org/mailman/listinfo/dev
Archived at http://seclists.org/nmap-dev/