Hello Matt,
On Wed, Feb 13, 2002 at 07:13:48PM +0000, Matt Kern wrote:
> There have been discussions on many php lists about the problems when
> compiling in the session handling elements of php while trying to run
> multiple invocations of apache/mod_php4.
> There has been talk that a patch to allow a per user session_mm.sem
> would be useful but I have yet to see a patch which actually does
> this... so I wrote one.
> I have attached the patch which will allow multiple instances of
> apache/php4 to operate on the same machine (bypassing the
> /tmp/session_mm.sem problems). The solution is obviously not ideal,
> but it does appear to work. I hope it is useful to some people.
> Basically, the patch allows the PS_MM_PATH to contain a %u which is
> replaced with the uid of the php4 process. Then, before initialising
> the mm it checks that the named directory exists and has secure modes
> (i.e. the directory and all parents must be owned by the uid/group of
> the php4 process or root -- there are a few permissions checks as
> well). The patch is not particularly pretty; it does make use of two
> fixed length buffers, however these buffers are only used for
> manipulating compiled in data and are easily large enough for the job.
> I have forwarded this patch onto Petr Cech (to forward upstream if he
> feels it is useful enough) but I haven't had a reply so I guess he is
> away at the moment.
Since Petr has recently orphaned the php4 packages, I don't know that
he's spending much time replying to php4-related emails right now.
For the most part, your patch looks good. I will note that as a
general-purpose routine, your recursive directory-creation loop contains
a race condition that could allow the owner of a parent directory to
yoink a newly-created directory out from under the php process and cause
problems later when trying to create the session file; but for the
specific case of creating subdirectories under /tmp (or /var/tmp), it
should be ok. In addition, your length parameter in strncpy() should be
bounded by the size of the destination buffer, not the size of the
source string. I believe both of these issues should be addressed
before submitting to upstream.
In the meantime, I don't see any reason not to include the patch in the
Debian package. Has this patch been submitted to the BTS? That's
probably the best place for it so that whoever is uploading the PHP4
packages in the future has it available.
Regards,
Steve Langasek
postmodern programmer
> *** mod_mm.c Fri Oct 12 00:52:00 2001
> --- mod_mm.c.new Sun Feb 10 00:07:46 2002
> ***************
> *** 27,32 ****
> --- 27,33 ----
> #include <sys/stat.h>
> #include <sys/types.h>
> #include <fcntl.h>
> + #include <unistd.h>
>
> #include "php_session.h"
> #include "mod_mm.h"
> ***************
> *** 35,41 ****
> # error mm is not thread-safe
> #endif
>
> ! #define PS_MM_PATH "/tmp/session_mm"
>
> /* For php_uint32 */
> #include "ext/standard/basic_functions.h"
> --- 36,42 ----
> # error mm is not thread-safe
> #endif
>
> ! #define PS_MM_PATH "/var/tmp/php4-%u/session_mm"
>
> /* For php_uint32 */
> #include "ext/standard/basic_functions.h"
> ***************
> *** 207,212 ****
> --- 208,270 ----
>
> static int ps_mm_initialize(ps_mm *data, const char *path)
> {
> + char buffer[256];
> + const char *ptr=path+1;
> + struct stat sbuf;
> + uid_t myuid=getuid();
> + gid_t mygid=getgid();
> + uid_t myeuid=geteuid();
> + gid_t myegid=getegid();
> + int errcode;
> +
> + /* Create the directory if it doesn't exist */
> +
> + while (*ptr!='\0' && strchr(ptr, '/')) {
> + ptr = strchr(ptr, '/')+1;
> + strcpy(buffer, path);
> + buffer[ptr-1-path] = '\0';
> +
> + /* Check that the directory exists and is secure */
> +
> + errcode = lstat(buffer, &sbuf);
> +
> + if (errcode==-1) {
> + if (errno==ENOENT) {
> + if (mkdir(buffer, 0700)==0) {
> + continue;
> + }
> + }
> + return FAILURE;
> + }
> +
> + if (S_ISLNK(sbuf.st_mode)) {
> + return FAILURE;
> + }
> +
> + if (sbuf.st_uid!=0 && sbuf.st_uid!=myuid && sbuf.st_uid!=myeuid) {
> + return FAILURE;
> + }
> +
> + if (sbuf.st_gid!=0 && sbuf.st_gid!=mygid && sbuf.st_gid!=myegid) {
> + return FAILURE;
> + }
> +
> + if (sbuf.st_uid==0) {
> + if (sbuf.st_mode==040755 || sbuf.st_mode==040775 ||
> + sbuf.st_mode==041777 || sbuf.st_mode==040700) {
> + continue;
> + } else {
> + return FAILURE;
> + }
> + } else {
> + if (sbuf.st_mode==040700){
> + continue;
> + } else {
> + return FAILURE;
> + }
> + }
> + }
> +
> data->owner = getpid();
> data->mm = mm_create(0, path);
> if (!data->mm) {
> ***************
> *** 247,254 ****
>
> PHP_MINIT_FUNCTION(ps_mm)
> {
> ps_mm_instance = calloc(sizeof(*ps_mm_instance), 1);
> ! if (ps_mm_initialize(ps_mm_instance, PS_MM_PATH) != SUCCESS) {
> ps_mm_instance = NULL;
> return FAILURE;
> }
> --- 305,327 ----
>
> PHP_MINIT_FUNCTION(ps_mm)
> {
> + const char *index;
> + char psmmpath_buffer[256];
> +
> + /* Add the uid into the PS_MM_PATH if necessary */
> +
> + index = strstr(PS_MM_PATH, "%u");
> + if (!index) {
> + strncpy(psmmpath_buffer, PS_MM_PATH, strlen (PS_MM_PATH)+1);
> + } else {
> + if (index>PS_MM_PATH) {
> + strncpy(psmmpath_buffer, PS_MM_PATH, index-PS_MM_PATH);
> + }
> + sprintf(psmmpath_buffer+(index-PS_MM_PATH), "%d%s\0", getuid (), index+2);
> + }
> +
> ps_mm_instance = calloc(sizeof(*ps_mm_instance), 1);
> ! if (ps_mm_initialize(ps_mm_instance, psmmpath_buffer) != SUCCESS) {
> ps_mm_instance = NULL;
> return FAILURE;
> }
Attachment:
pgpCVXWa9RuT2.pgp
Description: PGP signature