secur32: Getting real functionality into the NTLM provider

[Alexandre Julliard]

Kai Blin <blin at gmx.net> writes:

> +            unsigned char buffer[SECUR32_MAX_BUF_LEN+6000];
> +            BYTE bin[SECUR32_MAX_BUF_LEN];

You shouldn't allocate fixed size buffers on the stack, especially not
huge ones like that. You should compute the necessary size and
allocate a properly sized buffer from the heap.

> +    SEC_CHAR client_user_arg[512];
> +    SEC_CHAR client_domain_arg[512];
> +    SEC_CHAR passwd[512] = {0};

Same here, there's nothing that guarantees the buffers are the right
size; using snprintf may avoid overflows but it doesn't make the code
behave correctly.

> +SECURITY_STATUS fork_helperA(PNegoHelperA *new_helper, const char *prog,
> +        char * const argv[], char* passwd)
> +{

You should have a single fork helper that takes Unicode, you can't
pass ASCII chars to the Unix app anyway, you have to use
CP_UNIXCP. And more generally, you should do everything in Unicode.

> +    if( ( pipe(pipe_in) < 0 ) || ( pipe(pipe_out) < 0 ) )
> +    {
> +        return SEC_E_INTERNAL_ERROR;
> +    }
> +
> +    helper->helper_pid = fork();
> +
> +    helper->password = passwd;
> +
> +    if(helper->helper_pid == -1)
> +    {
> +        return SEC_E_INTERNAL_ERROR;
> +    }

You are leaking file descriptors in the error cases.

> +    if(helper->helper_pid == 0)
> +    {
> +        /* We're in the child now */
> +        close(0);
> +        close(1);
> +        
> +        dup2(pipe_out[0], 0);
> +        close(pipe_out[0]);
> +        close(pipe_out[1]);
> +
> +        dup2(pipe_in[1], 1);
> +        close(pipe_in[0]);
> +        close(pipe_in[1]);
> +
> +        execvp(prog, argv);
> +
> +        /* Whoops, we shouldn't get here. Big badaboom.*/
> +        return SEC_E_INTERNAL_ERROR;

You shouldn't return from the child, you should pass the error back to
the parent and then exit().

> +    {
> +        helper->pipe_in = fdopen(pipe_in[0], "r");
> +        close(pipe_in[1]);
> +        helper->pipe_out = fdopen(pipe_out[1], "w");
> +        close(pipe_out[0]);
> +    }

You should avoid using stdio.

-- 
Alexandre Julliard
julliard at winehq.org