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

[Stefan Dösinger]

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

Hi,

This patch is a lot more readable than the past version IMO. I'll look at the other patches tomorrow.

Am 2014-10-27 13:10, schrieb Jonathan Vollebregt:
> +static WCHAR *sanitize_path(WCHAR *key){
> Inconsistent curly bracket placement. The other patches have more cases of that.
> +        else
> +            reg_message(STRING_INVALID_KEY);
> Doesn't get_rootkey() implicitly take care of this? I don't see why you need an extra check for a single leading backslash.

There's one (intended?) side effect of this check though: It ensures that the code below doesn't crash on the input string "\\".
> +    i = strlenW(key);
> +    if (key[i - 1] == '\\')
> +        key[i - 1] = 0;
> Why do you need this? The existing code and the todo_wine test suggests that RegCreateKey accepts any number of trailing slashes. Adding a test for this in dlls/advapi32/tests/registry.c would be a good idea.

This is the only case where you modify the input string. If it indeed is not necessary you can change the parameter to a const WCHAR * and return a BOOL.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJUTs1oAAoJEN0/YqbEcdMw0rsP/3XocRwBZ2zPgs/0ERHMOL0q
84+09LlpOMMBzEQDue4WnxLgNwT6EUdP1W+CpMLtjdh6r35qePWRuHwrieuRoKsF
7SoZhAqB1XGfSTfHC5yaSlC3a3didsGn8wCvb9R66wFcBstWY+wW9cdnyUzRhXeK
oowmLBioiKlWGHBX84k7pvxLEzokeW+yEj0D/NXaVeFJ5ATfsqRynIiMJOeMJzJj
XcnrH9YBDzvOy36zSREclwEtwt73dZ9dRYiLO4fHaaqsBMM0xiuZYl5CkfUxGjtm
/c33mOOUl4l9EEpQLdu6kMtrT6kF13XbT2ume0AcqTbL4KrJVl7j173wck0vt1e7
kaLS1ViU4TGxHHo7mkqfMUjDpJM1tR5etV9DqQGQYui+BQ1ZOTpISrmuFxbXPAOd
ecwvZGXEePGWrZlsV4lECc25/1BJQfEpxKK/5pc6fTv0TYd+JGOOnOSlTR9xlOSW
rs/mjTuedQDNIFr+RvfMK+OpJIXXA9ve0jkpDX0XLnl3lZWG0p+rGholQXwHC5VS
1YUucH8moIW/8kN1s39/RSzcBgkxvAEDgiFsIigYqyEnPpafTbSQn3rNIVD5DQ0X
+Fy7gBffr+uaOSKAh8ShdMGvRFGipd4WNQTelemCVcD1Pku2X01ImBImyHeMxBzW
2JdXxGTtZT8PribxigHn
=eTay
-----END PGP SIGNATURE-----

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

[Jonathan Vollebregt]

> Inconsistent curly bracket placement. The other patches have more cases of that.

Should the arrays also have the opening brace on it's own line? A quick 
grep shows both styles for arrays in wine.
> > +        else
> > +            reg_message(STRING_INVALID_KEY);
> > Doesn't get_rootkey() implicitly take care of this? I don't see why you need an extra check for a single leading backslash.
>
> There's one (intended?) side effect of this check though: It ensures that the code below doesn't crash on the input string "\\".

The tests show that having any backslash at the start results in an 
error. That particular if just picks which error message to show.
> > +    i = strlenW(key);
> > +    if (key[i - 1] == '\\')
> > +        key[i - 1] = 0;
> > Why do you need this? The existing code and the todo_wine test suggests that RegCreateKey accepts any number of trailing slashes. Adding a test for this in dlls/advapi32/tests/registry.c would be a good idea.

reg/tests/reg.c:109 shows only the winXP version accepts multiple 
trailing backslashes.

The reason for stripping a single trailing backslash was for output 
consistency with query (As yet unimplemented) Specifically whether query 
outputs "HKEY_CURRENT_USER\\key" or "HKEY_CURRENT_USER\\key\\" as the 
key name.

Should I still make it const and handle the backslash stripping in a 
different function?
> > +static inline int path_rootname_cmp(const WCHAR *path, const WCHAR *name)
> > +{
> > +    DWORD length = strlenW(name);
> > +
> > +    return (strncmpiW(path, name, length) == 0 && (path[length] == 0 || path[length] == '\\'));
> > +}
> > I don't like the names "path" and "name". I wondered why you're relying on the length of name. A better name for "path" could be "input", but I can't think of anything for "name".

Because the input is the full path we have to stop comparing after the 
root key or it will always return 0 unless it's passed a root key.

I think the "path" parameter name makes this more obvious, but I could 
throw a comment in if that helps.
> > +    path = strchrW(path, '\\');
> > +    if (!path)
> > +        return k;
> > I see that this is why you need the trailing backslash elimination in the previous patch. But still, do you need this? What happens when you call e.g. RegOpenKey(HKEY_CURRENT_USER, "\\", &k)? If it returns HKEY_CURRENT_USER or a handle that points to HKEY_CURRENT_USER you're fine. Similarly for RegOpenKey(HKEY_CURRENT_USER, "", &k).

Actually this solves 2 separate problems. RegOpenKey takes the path 
after the root key but the path provided includes the root key, so 
strchrW skips the root key in the path in advance of passing it to 
RegOpenKey.

On the other hand, if someone inputs only a root key IE "HKCU" it will 
return NULL. We already have the key from path_get_rootkey, so we just 
return it here.

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

[Stefan Dösinger]

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

Am 2014-10-28 12:21, schrieb Jonathan Vollebregt:
> > Inconsistent curly bracket placement. The other patches have more
> >  cases of that.
>
> Should the arrays also have the opening brace on it's own line? A 
> quick grep shows both styles for arrays in wine.
> The one line arrays (e.g. short_hklm) no, the two-line or longer ones yes.
> The tests show that having any backslash at the start results in an
>  error. That particular if just picks which error message to show.
> Yes, but you could just check for two backslashes and proceed normally
> otherwise. You'll then write the same error message because you cannot
> find "\\HKEY_CURRENT_USER" in your root key table.
> reg/tests/reg.c:109 shows only the winXP version accepts multiple 
> trailing backslashes.
>
> The reason for stripping a single trailing backslash was for output
>  consistency with query (As yet unimplemented) Specifically whether
>  query outputs "HKEY_CURRENT_USER\\key" or
> "HKEY_CURRENT_USER\\key\\" as the key name.
> It'd be better to add this functionality together with query support
> then. Maybe keep it inside query only.

Is there a advapi32 function go get a string name for an open HKEY
handle? You could use that to normalize printed paths. That could also
take care of e.g. upper and lower case letters.
> Because the input is the full path we have to stop comparing after
>  the root key or it will always return 0 unless it's passed a root
>  key.
> I think the "path" parameter name makes this more obvious, but I 
> could throw a comment in if that helps.
> Nah, it should be possible to make this function readable without a
> comment. How about the names "input_path" and "root_key_name"?
> Actually this solves 2 separate problems. RegOpenKey takes the path
>  after the root key but the path provided includes the root key, so
>  strchrW skips the root key in the path in advance of passing it to
>  RegOpenKey.
> Yes, you need the strchrW call.
> On the other hand, if someone inputs only a root key IE "HKCU" it 
> will return NULL. We already have the key from path_get_rootkey, so
>  we just return it here.
> I don't think you need the NULL check with the early return, and I
> don't think you need to strip the trailing \\ in sanitize_path.
> Removing those (not accounting for reg query for now) will remove two
> code branches that need testing and make the code more difficult to read.

That's assuming RegOpenKey behaves as intended in those cases:

Msdn says RegOpenKey(key_in, NULL, &key) returns key == key_in, so no
need for the early return in the case that strchrW returns NULL. If
the behavior is the same in RegOpenKey(key_in, "\\", &key) there's no
need to strip trailing backslashes.

Maybe RegOpenKey(NULL, xxx, &key) returns an error that you can
propagate to main() and translate into an error message there. In that
case you don't have to write an error message from a helper function.

The RegOpenKey behavior may need additional tests in advapi32.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJUUAG+AAoJEN0/YqbEcdMwD+QP/3sHl1+Xs1pMBDjThviSv5mg
ohPDgCBIKFLlAT6bP+jF+ds/B6coVtLFCdr8ocUXZrFN9v6nN2k8r5z87QdN6xdb
oVfjj9sabZUvDd/PSrtzXAHz7Vfk4Db8kN6YuuZJMrNAhcYb/63cn25r5Zc3YvtV
UV8rJtx79p/xzT4vRXo3ESWOrUcjMknylSsEzypLIn3ihvZYDWODhGPg2Qwk7V9n
lyru5I/rpak1W33a+sO7/5Z4hKHTseyZjdChlXVFmip8JPLG8mImweMNSrxgdqF/
/hh7DD+krbl1+IJPJ7dxrBklocjeMtpaDZH2a1NHDzCDBCV39MZgmj0h3WQkGH39
rZraihHKEbXDnZP4ZVZVC8CUZskDfQt8dqZ5eW0IUqil0upJPFvhm8Dl9QpYu2mr
lw1CV5+6ZTmS/S3sy4Q6DQ6v5t1HjNvMBbUPKVWWt6HW7mgdEgaZmdFOdHqwKgCS
hVzO0u7qjPp7hPPLcWS/+ZgeImN4enhBzuB9GZ1NLKe+fix9FkLwU+jjeWWrTcJU
g8Q+Oa3r8Xf6drW894V4uCkPkHFH+VjPllpQpiSjnIammb5QXBW/cCZHdT0RF5eG
7qv5YcBc3jlerllzVrcj8uHR+jmhtm7aDIKOh87e6M+RuPGHG2C5rLTEdMEf5HW0
OuNrs5CvHNMmv7Mn+3UH
=Mbi0
-----END PGP SIGNATURE-----

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

[Jonathan Vollebregt]

On 10/28/2014 09:51 PM, Stefan Dösinger wrote:
> I don't think you need the NULL check with the early return

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.

We could replace the return with the path increment so that RegOpenKey 
gets NULL as it's parameter like this:
> path = strchrW(path, '\\');
>     if (path)
>         path++;
>
> if (RegOpenKeyW(k, path, &k) != ERROR_SUCCESS)

I suppose removing a line is worth the extra effort of having root keys 
go through RegOpenKey

How's this draft?
> https://github.com/jnvsor/wine/compare/4bb80af...280d772

[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-----

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

[Jonathan Vollebregt]

On 10/29/2014 04:38 PM, Stefan Dösinger wrote:
> 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.

How would I add reg.exe specific errors? Just make them ints over the 
16k used by system error codes?

In any case implementing this would require a big rewrite of most of the 
patches in the series, so I'll probably make a patch for the end of this 
series.
> > +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;
> > 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".

This checks for backslashes at the end of the key not the start - tests 
show more than one should produce an error.
> > -        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?

It's possible for something calling this to want a different default, so 
I decided to let the caller handle it.
> 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.

Does this mean I'm allowed to use malloc too?
> 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.

The idea is presumably only to prompt for overwrite if the value already 
exists. That's what the RegQueryValue is for. Presumably a y/n prompt 
should go here but I have my hands full with the rest of it.
> 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.

Should it prompt for new key creation as well as overwriting? I'm not 
sure what the native behavior is

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

[Stefan Dösinger]

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

Am 2014-10-29 19:56, schrieb Jonathan Vollebregt:
> > 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.
>
> How would I add reg.exe specific errors? Just make them ints over the
> 16k used by system error codes?
> That depends. Printing error messages from the top level reg_add, reg_query, etc is fine. What I don't like is printing them from functions at many different levels.

In some cases (e.g. invalid root key) you may be able to reuse some of the existing error values.
> In any case implementing this would require a big rewrite of most of the
> patches in the series, so I'll probably make a patch for the end of this
> series.
> Works for me.
> This checks for backslashes at the end of the key not the start - tests
> show more than one should produce an error.
> Ooops, I should have read this more carefully :-\ .
> > > -        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?
>
> It's possible for something calling this to want a different default, so
> I decided to let the caller handle it.
> I'd say keep it in get_regtype until you find a specific case that needs it.
> > 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.
>
> Does this mean I'm allowed to use malloc too?
> No, what I meant is write your own strdup-like function that uses HeapAlloc.
> The idea is presumably only to prompt for overwrite if the value already
> exists. That's what the RegQueryValue is for. Presumably a y/n prompt
> should go here but I have my hands full with the rest of it.
> Sure, I'm not saying that you should add a prompt now. I'm saying a /* FIXME: Check if the value exists and prompt for overwrite */ works just as well as calling RegQueryValue for (currently) no reason.
> > 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.
>
> Should it prompt for new key creation as well as overwriting? I'm not
> sure what the native behavior is
> What I meant is that you are doing some error checks *after* you already modified the registry. Leaving modifications behind (or creating them in the first place) if invalid operands are specified is most likely wrong.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJUUUJZAAoJEN0/YqbEcdMwU74P/3pkski0so1+OF2P7p5nLrh4
Pmj8Z9PCLt7z1MwlzdxPwYRlZ3Av62B9a/SpetKWgOKHJ6RnPMDT6mTBz0arWaii
j00TDwA8GPoa52yUzsoDtpovpm/NPp62AEUnHSmKpoym2Yg2N/wWmvD4RV2JGfQe
zONj22F1St8iQ2TRnTh5hFrWt9RyBWqmhxMgjcJfNGGqhZJYutZt6wLwHn3JeC8W
Vj6AjpUvS7uY8Poa8Um0xY1CxfiUiYWVu75vuytjgk0u5rJumwvW8lDdxIx93UYF
+dMkPs940CL4IaBgV79rhr2mGqIalGo0cTRSyJMrAB82gelHA7acJHPmojdFLnuf
jNEUB1qLrrV+LrPmP3cgug5DBPzfNYfJ4mtf/JrSYdY/ULnPTFoeZe44h6Y3FDmV
sqYEbLFeLuULONm37upYCKDzf5GIdHP0QrEaugxNQ0544lLgF/IC5qtV35GAed75
oaw19SYSuYTz6kogJ6vBhRjx5IvWXOZ+qZdGub7Wb02JEDHlAa8fiS57PkkGCQbP
Ps+nbpVT8EPvd4UP2WUvrmzO2lu3xDHivYPFpVeMkcckhRnsXS9pWa7s2Fb5BlSL
s/YmoUvP6AMprGht6AJ2UfxU7dcPvSY2lFYZAkOM3Ll+97+RP6sJu73Tu6T/sNee
dz+NFFzWrMKBwRKHP9ZL
=Cwmi
-----END PGP SIGNATURE-----