Hello,
I think it's a bad idea to patch extension for flaws not in PHP itself.
Furthermore, this adds a performance loss to every query. Did you do any
benchmarks with it? Anyway... I still think it's a bad idea, and from what
I've heard Zak is talking with MySQL about this.
Derick
On Wed, 27 Feb 2002, Jim Segrave wrote:
> On Tue 26 Feb 2002 (21:58 +0100), Jim Segrave wrote:
> >
> > I was looking at the posting from "James E. Flemer" <[EMAIL PROTECTED]>
> > and decided to try adding a bit more to it.
> >
> > ** This code is compiled, but not tested **
>
> I now have a patch against
> /* $Id: php_mysql.c,v 1.97.2.3 2001/12/09 13:53:56 zeev Exp $ */
> which appears to do what's needed. Initial testing indicates that it
> blocks the exploit, and only allows load data local infile 'path'
> queries where the path is valid for the php script owner (and I know a
> bit more about the regex library). The same caveats still apply for a
> threaded environment - the compile of the regex takes place on the
> first query processed, so it would need mutex'ing if threaded.
>
> Unified diff of a workding version follows:
> ========================================
> --- php_mysql.c.save Wed Feb 27 13:22:35 2002
> +++ php_mysql.c Wed Feb 27 12:16:00 2002
> @@ -55,6 +55,9 @@
>
> #include "php_ini.h"
>
> +#include <regex.h>
> +#include <syslog.h>
> +
> #if HAVE_MYSQL_MYSQL_H
> # include <mysql/mysql.h>
> #else
> @@ -211,9 +214,122 @@
>
> #define CHECK_LINK(link) { if (link==-1) { php_error(E_WARNING, "MySQL: A link to
>the server could not be established"); RETURN_FALSE; } }
>
> +/* see if a file in a query includedes
> + * LOAD DATA [LOW_PRIORITY | CONCURRENT] [LOCAL] INFILE "path"
> + * if so, see if we should allow it to be run (is the file specified by path
> + * valid for php
> + */
> +
> +static int php_mysql_safety_check(const char *p, int len);
> +
> +static volatile int regex_is_ready = 0;
> +
> +/* regex for mysql queries we want to be careful with. Note that the
> + * file path we're looking for is any file name beginning with a
> + * single or double quote and assumed to end with either quote type. This
> + * will fail for file names containing one or the other quote types. File
> + * names like this won't be protected from the exploit, but the risk is
> + * small IMHO
> + */
> +
> +static char* query_regex =
>".*[[:<:]]LOAD[[:>:]].*[[:<:]]DATA[[:>:]].*[[:<:]]LOCAL[[:>:]].*[[:<:]]INFILE[[:>:]][^'\"]*['\"]([^'\"]*).*";
> +
> +
> +static regex_t preg;
> +
> +/* compile this regex on the first SQL query seen */
> +static int init_regex (void)
> +{
> + int res;
> +
> + /*
> + * XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
> + * FIXME
> + * In a threaded environment, this would need a mutex and all
> + * sorts of other frippery. Then the result could be made
> + * available to all other threads.
> + */
> +
> + res = regcomp (&preg, query_regex, REG_EXTENDED | REG_ICASE);
> + if (res != 0) {
> + syslog (LOG_ERR, "init_regex: regcomp returned %d", res);
> + return 0;
> + }
> +
> + regex_is_ready = 1;
> +}
> +
> +/* look for mysql queries in the form LOAD DATA LOCAL INFILE 'path'
> + * if found, accept them (return 1) if a php script would be allowed
> + * to read the file
> + */
> +static int php_mysql_safety_check(const char *query, int len)
> +{
> + int res;
> + char *copy;
> + regmatch_t pmatch[2]; /* 0 = string, 1 = path */
> + char *path;
> +
> + /* only compile the regex once */
> + if (!regex_is_ready) {
> + if (init_regex () == 0) {
> + /* didn't compile - don't accept query - that encourages
> + fixing things */
> + return 0;
> + }
> + }
> +
> + /* dunno if I can trust the query to be null terminated. If I can,
> + why is there a length?
> + */
> + if ((copy = emalloc (len + 1)) == 0) {
> + syslog (LOG_ERR, "php_mysql_safety_check: Can't malloc query space");
> +
> + }
> +
> + strncpy (copy, query, len);
> +
> + /* don't allow queries with embedded nulls - to catch a potential
> + * nasty hiding of paths
> + */
> + if ((len - strlen (copy)) > 1)
> + return 0;
> +
> + res = regexec (&preg, query, 2, pmatch, 0);
> +
> + /* it's not one of the dangerous queries - let it pass */
> + if (res == REG_NOMATCH) {
> + efree (copy);
> + return 1;
> + }
> +
> + /* regexec didn't like the query, so we don't either */
> + if (res != 0) {
> + efree (copy);
> + return 0;
> + }
> +
> + /* it matched, pmatch[1].rm_so is the offset of the start of the path
> + and pmatch[1].rm_eo is the offset of the end of the path
> + */
> + path = copy + pmatch[1].rm_so;
> + *(copy + pmatch[1].rm_eo) = 0;
> +
> + /* path is the file path that the LOAD DATA... will use */
> + res = php_checkuid (path, "r", 0);
> +
> + /* php_checkuid returns 0 if it's forbidden */
> + if (res == 0) {
> + syslog (LOG_ERR, "php_mysql_safety_check: suspect query for %s", path);
> + }
> +
> + efree (copy);
> + return res;
> +}
> +
> /* {{{ _free_mysql_result
> * This wrapper is required since mysql_free_result() returns an integer, and
> - * thus, cannot be used directly
> + * thus, cannot be used directly
> */
> static void _free_mysql_result(zend_rsrc_list_entry *rsrc TSRMLS_DC)
> {
> @@ -1005,6 +1121,15 @@
> } while(0);
>
> convert_to_string_ex(query);
> +
> + /* disallow LOAD DATA LOCAL INFILE if safe_mode on */
> + if (PG(safe_mode)) {
> + if (!php_mysql_safety_check(Z_STRVAL_PP(query), Z_STRLEN_PP(query))) {
> + php_error(E_WARNING, "MySQL: Statement failed safe mode
>checks");
> + RETURN_FALSE;
> + }
> + }
> +
> /* mysql_query is binary unsafe, use mysql_real_query */
> #if MYSQL_VERSION_ID > 32199
> if (mysql_real_query(&mysql->conn, Z_STRVAL_PP(query), Z_STRLEN_PP(query))!=0)
>{
>
> ========================================
>
> --
> Jim Segrave [EMAIL PROTECTED]
>
> --
> PHP Development Mailing List <http://www.php.net/>
> To unsubscribe, visit: http://www.php.net/unsub.php
>
--
PHP Development Mailing List <http://www.php.net/>
To unsubscribe, visit: http://www.php.net/unsub.php
