#38993 [Opn->Csd]: Extra options in session.save_path cause verification on update to fail

ID: 38993 Updated by: iliaa@php.net Reported By: roel at qsp dot nl -Status: Open +Status: Closed Bug Type: Session related Operating System: FreeBSD 6 PHP Version: 5.1.6 New Comment: This bug has been fixed in CVS. Snapshots of the sources are packaged every three hours; this change will be in the next snapshot. You can grab the snapshot at http://snaps.php.net/. Thank you for the report, and for helping us make PHP better. Previous Comments: ------------------------------------------------------------------------ [2006-09-29 14:31:32] roel at qsp dot nl Description: ------------ Introduction ------------ The file based session handler uses the session.save_path ini variable to determine where to write it's session files. However, it also attempts to parse two other bits of information out of session.save_path, iff session.save_path contains one or two semi-colons. The format is: "[x;[y;]]pathname" Where both x and y are optional and x is a directory hashing depth for session files (defaults to 0) and y is the create mode for new session files, defaults to 0700. Whenever session.save_path is updated, a verification function is called. For this variable, this is the funtion "static PHP_INI_MH(OnUpdateSaveDir)" contained in ext/session/session.c. There is no verification in this function if PHP is running in regular mode. However, when in safe-mode, this function performs two verifications. First, it attempts to check ownership of the directory, and secondly, it does a php_check_open_basedir() on the path supplied. Problem description ------------------- In this function, OnUpdateSaveDir() in ext/session/session.c, no attempt is made to recognize and take out the optional parameters contained in the supplied new session.save_path value. So, when setting session.save_path to "0;0707;/whatever", PHP will supply php_checkuid() and php_check_open_basedir with the entire string, not just with /whatever. This causes the check to fail. Patch ------ The following patch should be applied to ext/session/session.c to fix this problem: *** ext/session/session.orig.c Fri Sep 29 11:35:05 2006 --- ext/session/session.c Fri Sep 29 12:39:26 2006 *************** *** 133,145 **** static PHP_INI_MH(OnUpdateSaveDir) { /* Only do the safemode/open_basedir check at runtime */ if (stage == PHP_INI_STAGE_RUNTIME) { ! if (PG(safe_mode) && (!php_checkuid(new_value, NULL, CHECKUID_ALLOW_ONLY_DIR))) { return FAILURE; } ! if (php_check_open_basedir(new_value TSRMLS_CC)) { return FAILURE; } } --- 133,150 ---- static PHP_INI_MH(OnUpdateSaveDir) { + char *p, *q; + int i; /* Only do the safemode/open_basedir check at runtime */ if (stage == PHP_INI_STAGE_RUNTIME) { ! /* Parse out optional hash level & file mode */ ! for (i=0, q=new_value; i<2 && (p=strchr(q, ';'))!=NULL; q=p+1, i++); ! ! if (PG(safe_mode) && (!php_checkuid(q, NULL, CHECKUID_ALLOW_ONLY_DIR))) { return FAILURE; } ! if (php_check_open_basedir(q TSRMLS_CC)) { return FAILURE; } } Remarks -------- Both parameters are, at least for the most part, undocumented. It would not be a bad idea to update the session.save_path description with information about these two variables. Submitted: 29/9/2006 Patch author: Roel Bouwman <roel@bouwman.net> Reproduce code: --------------- <?php /* Preconditions: * - server running in safe mode * - /whatever and this script owned by samed user. */ ini_set("session.save_path","0;0707;/whatever"); ?> Expected result: ---------------- 1. PHP checking wether "/whatever" is owned by script owner. 2. PHP performing basepath check for "/whatever" 3. session.save_path set to "0;0707;/whatever". Actual result: -------------- PHP will produce an error claiming that it cannot open the save_path directory as the uid check is performed not just for /whatever, but for the entire string. Result is that session.save_path will not be modified. ------------------------------------------------------------------------ -- Edit this bug report at http://bugs.php.net/?id=38993&edit=1