[PATCH] avoid crashing when given invalid user/group ids on win32

[Garrett Rooney <ro...@electricjellyfish.net>]

I'm not sure this is entirely correct, but here's a quick patch to
correct the problem I reported earlier about crashing in testuser.c
when we pass bogus uid/gid values into apr_uid_name_get and
apr_gid_name_get.

The fix is to use IsValidSid to confirm the validity of the uid/gid
before we try to call LookupAccountSid.

The one thing I'm really not sure of is what should be done on non-NT
systems.  The MSDN docs say that IsValidSid didn't show up until NT
workstation 3.1.  Then again, they say the same thing about
LookupAccountSid, and we seem to use that unconditionally, so perhaps
it's ok.

Anyway, if a win32-savy person could take a quick look at the patch
and tell me if it looks sane to them I'd appreciate it.

-garrett

Re: [PATCH] avoid crashing when given invalid user/group ids on win32

["William A. Rowe, Jr." <wr...@rowe-clan.net>]

Andreas Magnusson wrote:
> But the entrypoint in the dll is the same as for LockServiceDatabase and 
> IsValidAcl (and many more), so they might not do anything on those old 
> versions.

That would be consistent (in the case of IsValidAcl) with testing for the
pattern not the meaning.

Dunno why LockServiceDatabase would match to the same, perhaps you are right
that it's an entirely bogus SUCCESS stub.  But one assumes that if there is
nothing to test, it probably wouldn't cause a crash, either.

Bill

Re: [PATCH] avoid crashing when given invalid user/group ids on win32

[Andreas Magnusson <an...@home.se>]

Garrett Rooney wrote:
> I'm not sure this is entirely correct, but here's a quick patch to
> correct the problem I reported earlier about crashing in testuser.c
> when we pass bogus uid/gid values into apr_uid_name_get and
> apr_gid_name_get.
> 
> The fix is to use IsValidSid to confirm the validity of the uid/gid
> before we try to call LookupAccountSid.
> 
> The one thing I'm really not sure of is what should be done on non-NT
> systems.  The MSDN docs say that IsValidSid didn't show up until NT
> workstation 3.1.  Then again, they say the same thing about
> LookupAccountSid, and we seem to use that unconditionally, so perhaps
> it's ok.
> 
> Anyway, if a win32-savy person could take a quick look at the patch
> and tell me if it looks sane to them I'd appreciate it.
> 
> -garrett

Hi,
First: the patch look very sane.
Second: You're right, the documentation does look weird for IsValidSid 
(and all other security functions). However, IsValidSid does exists as 
an exported function from advapi32.dll on Windows ME, so a dll would at 
least not refuse to load on Win9X.
But the entrypoint in the dll is the same as for LockServiceDatabase and 
IsValidAcl (and many more), so they might not do anything on those old 
versions.
Unfortunately I don't have a running system to check on.

/Andreas

Re: [PATCH] avoid crashing when given invalid user/group ids on win32

["William A. Rowe, Jr." <wr...@rowe-clan.net>]

Garrett Rooney wrote:
> 
> The test is trying to send a nonexistant UID/GID (it hardcodes 9999999
> as the value, FWIW), that seems like "faulty data" to me.  If you
> aren't allowed to just throw random crap at these functions that's
> also fine, but given that we actually have a test for that behavior it
> sure implies that it's allowed.  If not, the test should go away and
> we should probably document that these values are platform specific,
> and that the only portable way to get one is via an APR function that
> goes from name to UID or name to GID.

I guess the point/question comes down to this, is 9999999 a valid
apr_uid_t / apr_gid_t?  If they are then yes, I concur with your patch.
But nothing on the win32 side could return such a value.  Those types
are pointers to data.

Now could we have an invalid user/group on win32?  Sure, but it wouldn't
be a numeric constant on win32 like this example.  It would be a pointer
to a SID that was unrecognized.

Bill

Re: [PATCH] avoid crashing when given invalid user/group ids on win32

[Garrett Rooney <ro...@electricjellyfish.net>]

On 1/22/06, William A. Rowe, Jr. <wr...@rowe-clan.net> wrote:
> Garrett Rooney wrote:
> > The fix is to use IsValidSid to confirm the validity of the uid/gid
> > before we try to call LookupAccountSid.
>
> I'd disagree.  The test tried to force a hardcode platform specific value
> at apr_uid_get etc... that's invalid and should crash the users' code.
>
> We have a general principal in apr that faulty code produces crashes, while
> faulty data produces errors.  I'd disagree that the data was faulty.
>
> What uid/gid were they trying to test?  Should we have some static helpers
> to get the root / everyone sorts of ID's on a platform-by-platform basis?

The test is trying to send a nonexistant UID/GID (it hardcodes 9999999
as the value, FWIW), that seems like "faulty data" to me.  If you
aren't allowed to just throw random crap at these functions that's
also fine, but given that we actually have a test for that behavior it
sure implies that it's allowed.  If not, the test should go away and
we should probably document that these values are platform specific,
and that the only portable way to get one is via an APR function that
goes from name to UID or name to GID.

> > The one thing I'm really not sure of is what should be done on non-NT
> > systems.  The MSDN docs say that IsValidSid didn't show up until NT
> > workstation 3.1.
>
> Rule of thumb; Win95 is bare minimum baseline, and we aren't really even
> trying to support anything pre-WinNT (although if folks continue to offer
> patches for 9x series platforms I guess they will always be welcomed while
> we can apply them without hurting a more sophisticated implementation.)

Considering that other functions we use in this code also have the
same "when did it show up" language in the MSDN docs I don't think
it's a reason not to use the code.

-garrett

Re: [PATCH] avoid crashing when given invalid user/group ids on win32

["William A. Rowe, Jr." <wr...@rowe-clan.net>]

Garrett Rooney wrote:
> The fix is to use IsValidSid to confirm the validity of the uid/gid
> before we try to call LookupAccountSid.

I'd disagree.  The test tried to force a hardcode platform specific value
at apr_uid_get etc... that's invalid and should crash the users' code.

We have a general principal in apr that faulty code produces crashes, while
faulty data produces errors.  I'd disagree that the data was faulty.

What uid/gid were they trying to test?  Should we have some static helpers
to get the root / everyone sorts of ID's on a platform-by-platform basis?

> The one thing I'm really not sure of is what should be done on non-NT
> systems.  The MSDN docs say that IsValidSid didn't show up until NT
> workstation 3.1.

Rule of thumb; Win95 is bare minimum baseline, and we aren't really even
trying to support anything pre-WinNT (although if folks continue to offer
patches for 9x series platforms I guess they will always be welcomed while
we can apply them without hurting a more sophisticated implementation.)

Re: [PATCH] avoid crashing when given invalid user/group ids on win32

["William A. Rowe, Jr." <wr...@rowe-clan.net>]

There's no way for a function which expects a pointer to start accepting
arbitrary numbers.  This is fixed (differently) by eliminating the test
on APR 0.9/1.2 branches and trunk.  Thanks for the pointers.

Garrett Rooney wrote:
> I'm not sure this is entirely correct, but here's a quick patch to
> correct the problem I reported earlier about crashing in testuser.c
> when we pass bogus uid/gid values into apr_uid_name_get and
> apr_gid_name_get.
> 
> The fix is to use IsValidSid to confirm the validity of the uid/gid
> before we try to call LookupAccountSid.
> 
> The one thing I'm really not sure of is what should be done on non-NT
> systems.  The MSDN docs say that IsValidSid didn't show up until NT
> workstation 3.1.  Then again, they say the same thing about
> LookupAccountSid, and we seem to use that unconditionally, so perhaps
> it's ok.
> 
> Anyway, if a win32-savy person could take a quick look at the patch
> and tell me if it looks sane to them I'd appreciate it.
> 
> -garrett

Re: [PATCH] avoid crashing when given invalid user/group ids on win32

[Joe Orton <jo...@redhat.com>]

On Mon, Jan 30, 2006 at 11:36:14AM -0600, William Rowe wrote:
> Joe Orton wrote:
> >Making the apr_{uid,gid}_name_get() test for an arbitrary apr_uid_t 
> >value ifndef WIN32 seems reasonable if it's not possible to obtain such 
> >values legitimately on Win32.
> >
> >It is possible to obtain such values legitimately on Unix (e.g. random 
> >file owner UIDs which show up on an NFS mount); so the test is valid 
> >there.
> 
> My point was, no, it's not possible; where did you get said random UID?
> Perhaps you called apr_file_stat() and it returned a uid to some non
> existant user?

Yes, that's exactly what can happen on Unix.  And these functions all 
crashed on some Unix platforms with legitimate input, because the error 
handling was broken.  So the tests need to be kept for Unix platforms.

If the tests are not appropriate on Win32 then #ifndef WIN32 them, as I 
said already.  There is absolutely nothing wrong with having test cases 
in the test suite which are designed to exercise only particular code 
paths on particular platforms, and it's just silly to argue otherwise.

joe

Re: [PATCH] avoid crashing when given invalid user/group ids on win32

["William A. Rowe, Jr." <wr...@rowe-clan.net>]

Joe Orton wrote:
> On Sat, Jan 21, 2006 at 05:07:21PM -0800, Garrett Rooney wrote:
> 
>>I'm not sure this is entirely correct, but here's a quick patch to
>>correct the problem I reported earlier about crashing in testuser.c
>>when we pass bogus uid/gid values into apr_uid_name_get and
>>apr_gid_name_get.
>>
>>The fix is to use IsValidSid to confirm the validity of the uid/gid
>>before we try to call LookupAccountSid.
>>
>>The one thing I'm really not sure of is what should be done on non-NT
>>systems.  The MSDN docs say that IsValidSid didn't show up until NT
>>workstation 3.1.  Then again, they say the same thing about
>>LookupAccountSid, and we seem to use that unconditionally, so perhaps
>>it's ok.
> 
> 
> Making the apr_{uid,gid}_name_get() test for an arbitrary apr_uid_t 
> value ifndef WIN32 seems reasonable if it's not possible to obtain such 
> values legitimately on Win32.
> 
> It is possible to obtain such values legitimately on Unix (e.g. random 
> file owner UIDs which show up on an NFS mount); so the test is valid 
> there.

My point was, no, it's not possible; where did you get said random UID?
Perhaps you called apr_file_stat() and it returned a uid to some non
existant user?

That's not the same thing as assuming you know that uid is an int.  If you
take one apr-uid and pass it to these functions, they should not crash.  But
if you take one non-apr uid and pass it in, on unix that works, on Win32
it crashes.

On win32 we've 'hidden' the uid as a pointer to uid stored on the pool it
was queried from.  Nothing wrong with that.  The test is simply data-type
abuse.

Re: [PATCH] avoid crashing when given invalid user/group ids on win32

[Joe Orton <jo...@redhat.com>]

On Sat, Jan 21, 2006 at 05:07:21PM -0800, Garrett Rooney wrote:
> I'm not sure this is entirely correct, but here's a quick patch to
> correct the problem I reported earlier about crashing in testuser.c
> when we pass bogus uid/gid values into apr_uid_name_get and
> apr_gid_name_get.
> 
> The fix is to use IsValidSid to confirm the validity of the uid/gid
> before we try to call LookupAccountSid.
> 
> The one thing I'm really not sure of is what should be done on non-NT
> systems.  The MSDN docs say that IsValidSid didn't show up until NT
> workstation 3.1.  Then again, they say the same thing about
> LookupAccountSid, and we seem to use that unconditionally, so perhaps
> it's ok.

Making the apr_{uid,gid}_name_get() test for an arbitrary apr_uid_t 
value ifndef WIN32 seems reasonable if it's not possible to obtain such 
values legitimately on Win32.

It is possible to obtain such values legitimately on Unix (e.g. random 
file owner UIDs which show up on an NFS mount); so the test is valid 
there.

joe