php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #40600 [PATCH]:getgrgid_r, getgrnam_r etc functions regression(crash)
Submitted: 2007-02-22 23:32 UTC Modified: 2007-02-26 15:43 UTC
From: stas at FreeBSD dot org Assigned: iliaa (profile)
Status: Not a bug Package: POSIX related
PHP Version: 5.2.1 OS: FreeBSD
Private report: No CVE-ID: None
View Developer Edit
Welcome! If you don't have a Git account, you can't do anything here.
If you reported this bug, you can edit this bug over here.
(description)
Block user comment
Status: Assign to:
Package:
Bug Type:
Summary:
From: stas at FreeBSD dot org
New email:
PHP Version: OS:

 

 [2007-02-22 23:32 UTC] stas at FreeBSD dot org
Description:
------------
This module has problems with functions like getgrgid_r etc. It tries to find out limits using sysconf, but FreeBSD doesn't have, e.g. _SC_GETPW_R_SIZE_MAX. Since it does't try to check the return value it effectively leads to attempt to allocate (size_t)-1 bytes, which obviously fails, since trying to allocate (size_t)-1 bytes exceeds any limits.

Reproduce code:
---------------
$groupinfo = posix_getgrgid(0);
print_r($groupinfo);

Expected result:
----------------
something meaningful


Patches

Pull Requests

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2007-02-22 23:34 UTC] stas at FreeBSD dot org
The patch itself:
----------------------------------------------------------------
--- posix.c.orig        Fri Jan 12 04:46:11 2007
+++ posix.c     Thu Feb 22 14:56:56 2007
@@ -837,9 +837,8 @@

 #if defined(ZTS) && defined(HAVE_GETGRNAM_R) && defined(_SC_GETGR_R_SIZE_MAX)
        buflen = sysconf(_SC_GETGR_R_SIZE_MAX);
-       if (buflen < 1) {
-               RETURN_FALSE;
-       }
+       if (buflen < 0)
+               buflen = 1024;
        buf = emalloc(buflen);
        g = &gbuf;

@@ -887,6 +886,8 @@
 #ifdef HAVE_GETGRGID_R

        grbuflen = sysconf(_SC_GETGR_R_SIZE_MAX);
+       if (grbuflen < 0)
+               grbuflen = 1024;
        grbuf = emalloc(grbuflen);

        ret = getgrgid_r(gid, &_g, grbuf, grbuflen, &retgrptr);
@@ -950,9 +951,9 @@

 #if defined(ZTS) && defined(_SC_GETPW_R_SIZE_MAX) && defined(HAVE_GETPWNAM_R)
        buflen = sysconf(_SC_GETPW_R_SIZE_MAX);
-       if (buflen < 1) {
-               RETURN_FALSE;
-       }
+       if (buflen < 0)
+               buflen = 1024;
+
        buf = emalloc(buflen);
        pw = &pwbuf;

@@ -999,9 +1000,8 @@
        }
 #if defined(ZTS) && defined(_SC_GETPW_R_SIZE_MAX) && defined(HAVE_GETPWUID_R)
        pwbuflen = sysconf(_SC_GETPW_R_SIZE_MAX);
-       if (pwbuflen < 1) {
-               RETURN_FALSE;
-       }
+       if (pwbuflen < 0)
+               pwbuflen = 1024;
        pwbuf = emalloc(pwbuflen);

        ret = getpwuid_r(uid, &_pw, pwbuf, pwbuflen, &retpwptr);
--------------------------------------------------------------
 [2007-02-22 23:39 UTC] tony2001@php.net
+       if (grbuflen < 0)
+               grbuflen = 1024;

I definitely agree with this part of the patch.
But other parts look to me as a "workaround" for FreeBSD problems.

-       if (buflen < 1) {
-               RETURN_FALSE;
-       }
+       if (buflen < 0)
+               buflen = 1024;

It might be safe to do it on FreeBSD when you know for sure that this functionality is missing and it's safe to use 1K buffer, but other systems might behave differently.
 [2007-02-23 10:47 UTC] stas at FreeBSD dot org
-       if (buflen < 1) {
-               RETURN_FALSE;
-       }
+       if (buflen < 0)
+               buflen = 1024;

>It might be safe to do it on FreeBSD when you know for sure >that this functionality is missing and it's safe to use 1K >buffer, but other systems might behave differently.

This patch covers two problems:
1) The POSIX says that sysconf will return -1 on failure, thus the ( < 1) check is definitely incorrect
2) It's safe to use the buffer of any size (according to POSIX), since you give the buffer length to these functions. They'll return error if the buffer lenght isn't enough - it's better then give up on retriving this info just in case the sysconf doesn't has these limit values.
 [2007-02-23 12:22 UTC] tony2001@php.net
>This patch covers two problems:
>1) The POSIX says that sysconf will return -1 on failure,
> thus the ( < check is definitely incorrect

Oh? Care to elaborate?

>2) It's safe to use the buffer of any size (according to
> POSIX), since you give the buffer length to these
> functions. 

Yeah, according to POSIX those functions must be implemented.
But they are not.

>it's better then give up on retriving this info just in
>case the sysconf doesn't has these limit values.

I don't think it's any better to use hacks to workaround missing FreeBSD functionality.

 [2007-02-23 13:47 UTC] stas at FreeBSD dot org
>>This patch covers two problems:
>>1) The POSIX says that sysconf will return -1 on failure,
>> thus the ( < check is definitely incorrect
>
>Oh? Care to elaborate?

Yeah... According to susv3:
"If name is an invalid value, sysconf() shall return -1 and set errno to indicate the error. If the variable corresponding to name has no limit, sysconf() shall return -1 without changing the value of errno. Note that indefinite limits do not imply infinite limits; see <limits.h>."

>>2) It's safe to use the buffer of any size (according to
>> POSIX), since you give the buffer length to these
>> functions. 
>
>Yeah, according to POSIX those functions must be >implemented.
>But they are not.

>it's better then give up on retriving this info just in
>case the sysconf doesn't has these limit values.

>I don't think it's any better to use hacks to workaround >missing FreeBSD
> functionality.

Ok, agree. It's open to you.
 [2007-02-23 13:55 UTC] tony2001@php.net
>Yeah... According to susv3:
Yes, I know that, thanks.
But that does not mean "if (buflen < 1)" is incorrect.
I don't think that zero buflen is a correct value (and even if it is, it's useless).
 [2007-02-23 14:07 UTC] tony2001@php.net
Ilia, please take a look at this, IIRC you added those sysconf() patches.
 [2007-02-23 23:53 UTC] iliaa@php.net
Thank you for taking the time to write to us, but this is not
a bug. Please double-check the documentation available at
http://www.php.net/manual/ and the instructions on how to report
a bug at http://bugs.php.net/how-to-report.php

The current code is fine, we should not hardcode buffer sizes 
if they cannot be retrieved, this could lead to exploitable 
situations. Also if the return buffer length of 0 it probably 
indicates a problem. 
 [2007-02-24 09:03 UTC] stas at FreeBSD dot org
> The current code is fine, we should not hardcode buffer     > sizes if they cannot be retrieved, this could lead to       > exploitable situations. Also if the return buffer length of > 0 it probably indicates a problem. 

1) According to POSIX it's not a problem
2) Besides that one check is missing (take a look at patch), so you're effectively trying to malloc (size_t)-1 bytes on FreeBSD currently, which leads to crash.
 [2007-02-25 23:04 UTC] stas at FreeBSD dot org
The bug is still here.
 [2007-02-26 01:59 UTC] iliaa@php.net
buflen check disallows values that are less then 1, so how 
will you ever allocate a negative value?

RETURN_FALSE will terminate the function.
 [2007-02-26 14:03 UTC] nlopess@php.net
He is refering to this part of the patch:
@@ -887,6 +886,8 @@
 #ifdef HAVE_GETGRGID_R

        grbuflen = sysconf(_SC_GETGR_R_SIZE_MAX);
+       if (grbuflen < 0)
+               grbuflen = 1024;


so there is no check there for a negative return value.
 [2007-02-26 15:43 UTC] tony2001@php.net
Nuno, this part of the patch has been applied 3 days ago.
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved.
Last updated: Tue Dec 10 21:01:27 2024 UTC