[PATCH 1/6] reg: Sanitize key paths in main

[Stefan Dösinger]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am 2014-10-29 00:19, schrieb Jonathan Vollebregt:
> RegOpenKey always returns an error if path starts with '\\', which
> is why theres an increment on path right after the strchrW
> 
> Of course if path_get_key returns NULL we'll be passing in the 
> equivalent of `(WCHAR *)2` so the NULL check prevents segfaults.
Right, my bad.

> We could replace the return with the path increment so that 
> RegOpenKey gets NULL as it's parameter like this:
It has one less branch (no early return), but still has the if, so I guess it is an improvement, although not as much as I hoped. I'll leave this up to you.

Another alternative idea, but I don't know how well this can work: path_get_key already looks at the '\\' behind the root key. You could change this function to return a root key and a WCHAR that points to the rest of the path sans \\. But don't bother too much about this, I think that's a fairly minor detail.

Other than that you're still writing error messages from inside helper functions. Please try to return useful error codes and write the error messages from main() or the top level reg_*() functions. This should be a separate patch though, and it may mean adding some error case tests to advapi32.

The thinking behind this is: There are plenty of reasons why Reg* functions can fail, not just that the key does not exist, e.g. permission denied, maybe others. Thus you will need some code that writes error messages that correspond to the Reg*Key return values anyway.

There will always be errors that are specific to reg.exe though. E.g. it's not possible to pass a negative DWORD to RegSetValue.

Other things:
> +static BOOL sane_path(const WCHAR *key)
> +{
> +    int i = strlenW(key);
> +
> +    if (i < 3 || (key[i - 1] == '\\' && key[i - 2] == '\\'))
> +    {
> +        reg_message(STRING_INVALID_KEY);
> +        return 0;
Please return TRUE or FALSE from a function that returns a BOOL type.

I still don't see why you need this check. If you remove it path_get_key should produce the same error message because it can't find a root node "\FOOBAR".

The remote check ("\\FOOBAR") is necessary though - it writes a different error message.

>          key_name = argvW[2];
> +        if (!sane_path(argvW[2]))
> +            return 1;
Minor detail: Pass key_name to sane_path.

Patch 3:
> +        if (strcmpiW(type_rels[i].name, type_name) == 0)
> +            return type_rels[i].type;
if (!strcmpiW(...)) is preferred over if (strcmpiW(...) == 0)

> -        reg_type = get_regtype(type);
> +        if (!type)
> +            reg_type = REG_SZ;
> +        else
> +            reg_type = wchar_get_type(type);
Why not keep the !type check inside get_regtype?

Patch 4:
> +            i = (strlenW(input) + 1) * sizeof(WCHAR);
> +            output = HeapAlloc(GetProcessHeap(), 0, i);
> +            lstrcpyW((WCHAR *) output, input);
Since you know the length, you can use memcpy. You could also think about a strdupW_heap() helper function if you need this in multiple places, but then reusing the length is more difficult.

Patch 5:
> +        if (!force && RegQueryValueW(key, value_name, NULL, NULL) == ERROR_SUCCESS)
>          {
> -            if (RegQueryValueW(subkey,value_name,NULL,NULL)==ERROR_SUCCESS)
> -            {
> -                /* FIXME:  Prompt for overwrite */
> -            }
> +            /* FIXME:  Prompt for overwrite */
>          }
Afaics this was and still is dead code. Unless RegQueryValue has a side effect either just keep the comment or write an actual FIXME.

If the subkey does not exist, and invalid data is passed, you're creating the subkey without adding the value and returning an error. This is likely not the correct behavior.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJUUQnrAAoJEN0/YqbEcdMwv1IQAILPNqW9AP52QSGoF1YzuVT2
wDq4P4EI9XZCl2htcsI15WhcKnuSTyAwzXnBOBpPHMcxlT36njMDf2NHyGkOnhD0
Av+zEsW9vMPuMwV4/A0V8SqztB0+qgGE5j7pYHe1Bj+x5gph/qPlq94rU/EwjBiN
Kaqz97joKOAr0Iz/S90c6JY0iAmBAYh5EObriYjYF9xa1hlrhFVExxq4gqLLO4us
gy2KXorRMky9rLhZ/M87K6v7SgaEI280r6OI9NyD0ncssOLE2rT1tmiv1fjMdC2k
Ksut9T9ghh+RRBueceXdNqX42VWAnWowqlbJ4tsQ323rlD944WwVrP9dk04Ev4Qi
UOWTcu3/ReoDofGsVJsfDPpizkU/IZX/FtwYmwmg5DWjnj2M6XKVi37PhpOzHuDm
Z95Yn4Xi1yHf+sw4G9acI9pctJoppRRnusDeQ6OIV0oxHY1v2yHgrMWWlMa2i3iY
lEvBKJVUEXEmkT6fTJrfCmiUEs1VWEVmAOz7OUNlQjUVXiHzY06jQefhgkmt7hs5
S/k6neK7/5urikJq3HgdXp31ojqeYaRj/w5UyhftyeFArriQ6D2oRqvOXDUYXNKz
TdyVlURO7i+1aS9/RV4qdBaMDEwSKGwy4zEISlJo+gQ+sPCUSZQDEwxddN9u64MT
hq5w7qt2PYQbZlMZLFTk
=EVnL
-----END PGP SIGNATURE-----