June 2005 - Development - lists.drupal.org

[drupal-devel] [feature] Replace $_SERVER["REMOTE_ADDR"] with a function which understands X-Forwarded-For
by Steven 01 Jun '05

01 Jun '05

Issue status update for http://drupal.org/node/20471 Project: Drupal Version: 4.6.0 Component: base system Category: feature requests Priority: normal Assigned to: degerrit Reported by: degerrit Updated by: Steven Status: patch In Drupal we tend to store the original information in the db, escaping or formatting on output as needed. All places where the hostname is output should be okay after some recent changes. Steven Previous comments: ------------------------------------------------------------------------ April 14, 2005 - 01:42 : degerrit In several modules, replace $_SERVER["REMOTE_ADDR"] with a function which understands X-Forwarded-For. Proxy servers often (sometimes, always?) insert "X-Forwarded-For" headers which contains the "real" client IP address. I think it is useful to log the client IP address in that situation. This little snippet works for me, and I'd suggest something similar be made into a function? $headers = apache_request_headers(); if (array_key_exists('X-Forwarded-For', $headers)){ $hostname=$headers['X-Forwarded-For'] . ' via ' . $_SERVER["REMOTE_ADDR"]; } else { $hostname=$_SERVER["REMOTE_ADDR"]; } I can make a patch proposal if people like the idea. ------------------------------------------------------------------------ May 21, 2005 - 23:44 : degerrit Attachment: http://drupal.org/files/issues/remote_addr.diff (6.07 KB) Here's my proposal for showing user IP addresses behind proxy servers, especially in log files. It adds a function remote_addr() and replaces all the uses of $_SERVER["REMOTE_ADDR"] with that function. This is my first patch for Drupal, don't be too harsh one me :-) A few notes: - I patched session.inc because $_SERVER["REMOTE_ADDR"] is also called in session.inc and common.inc is not available at that time it seems - maybe I replaced too vigorously in common.inc and sessions.inc, it works for me though - maybe it should be renamed http_remote_addr or apache_remote_addr ------------------------------------------------------------------------ May 22, 2005 - 10:37 : Dries Couple comments: * Drupal supports non-Apache webservers: apache_request_headers() might not always be available. * Does this patch need input checking? Depends on how/when/where the remote address data is used. Fact is that we don't want people to inject XSS-stuff through HTTP headers. Make sure to double-check this. * I'd rename the function to remote_address(). We usually don't abbreviate words like 'address'. * The word 'via' can't be translated. * We write } else { and not } else {* Just thinking up loud. Is it easy to fake or spoof X-Forwarded-For-headers? If it is, it would be easy to trick Drupal (eg. in the statistics.module's log pages we group by remote address). For the watchdog it doesn't matter, but for the statistics module it does. Maybe we need a toggle so remote_address() can be told not to return the 'true IP address'. ------------------------------------------------------------------------ May 25, 2005 - 06:37 : Steven X-Forwarded-For can be spoofed without any trouble. Just connect and send the fake header. I don't think we should pay attention to it. If we do, we need to also store the original IP address. Perhaps we could do "real.ip.#.# (forwarded.ip.#.#)" ------------------------------------------------------------------------ May 25, 2005 - 06:39 : Steven Sorry didn't look at the patch carefully enough. Yes, we need XSS protection: in practice, we always strore remote_addr() in the hostname column in the db. Wherever that column is displayed, it needs to be check_plain()ed. ------------------------------------------------------------------------ May 27, 2005 - 00:58 : degerrit I will re-write the patch with Dries' suggestions, I haven't had much time this week. I found some interesting examples of scripts doing roughly the same, none of them seem to do input checking (which I hadn't thought of either, but is obviously very needed!). Good thing there are people like Dries around, eh! There's also another possible header: HTTP_CLIENT_IP, I've never seen it but then I don't log all my clients' headers either ;-) * http://be2.php.net/getenv (the 2nd comment at this time) * http://php.planetmirror.com/manual/en/function.getallheaders.php * http://ip-to-country.webhosting.info/node/view/212 * http://dev.wp-plugins.org/file/wp-contact-form/trunk/wp-contactform.php?rev…

1 0

[drupal-devel] [bug] Statistic information about node reads is shown to unprivileged users
by Steven 01 Jun '05

01 Jun '05

Issue status update for http://drupal.org/node/22565 Project: Drupal Version: 4.6.0 Component: statistics.module Category: bug reports Priority: normal Assigned to: Robin Monks Reported by: massabob Updated by: Steven Status: patch I'm not sure about this patch: often, read counts are shown directly on the site. But if the permission for viewing the counts is the same as the permission for accessing the administrator's detailed logs, then you wouldn't give that to everyone. There is already an option to choose whether counts are displayed. Perhaps we could change that to "No" "For priviledged users" "For everyone". In last case it acts like it is now, it the second case it requires "access statistics" permission. What do you think? Steven Previous comments: ------------------------------------------------------------------------ May 11, 2005 - 09:51 : massabob Information about '%count reads' in node's footer is shown to unprivileged users. I suggest that function statistics_link in statistics.module should be corrected in this way: // Original function statistics_link($type, $node = 0, $main = 0) { global $id; $links = array(); if ($type != 'comment' && variable_get('statistics_display_counter', 0)) { $statistics = statistics_get($node->nid); if ($statistics) { $links[] = format_plural($statistics['totalcount'], '1 read', '%count reads'); } } return $links; } // Fixed function statistics_link($type, $node = 0, $main = 0) { global $id; $links = array(); if ($type != 'comment' && variable_get('statistics_display_counter', 0) && user_access('display statistics')) { $statistics = statistics_get($node->nid); if ($statistics) { $links[] = format_plural($statistics['totalcount'], '1 read', '%count reads'); } } return $links; } The only change is in "&& user_access('display statistics')" on line 98. ------------------------------------------------------------------------ May 27, 2005 - 03:44 : rbarreca Should read user_access('access statistics') not user_access('display statistics'). ------------------------------------------------------------------------ May 27, 2005 - 13:19 : Robin Monks Attachment: http://drupal.org/files/issues/user.access.stats.patch (730 bytes) And here that is in patch form. Robin

1 0

[drupal-devel] [feature] move check_query() into the database interface files
by Steven 01 Jun '05

01 Jun '05

Issue status update for http://drupal.org/node/13180 Project: Drupal Version: cvs Component: database system Category: feature requests Priority: normal Assigned to: chx Reported by: chx Updated by: Steven Status: patch Actually, Drupal does nothing to ensure the database character set is set to UTF-8 (can it?). Thus, escaping with mysql_real_escape_string() is not advised. Steven Previous comments: ------------------------------------------------------------------------ November 19, 2004 - 21:35 : chx Attachment: http://drupal.org/files/issues/check_output_patch.txt (1.54 KB) The check_query documentation says 'Prepare user input for use in a database query, preventing SQL injection attacks.' But the code itself is a simple addslashes. This is simply not correct -- for mysql, yes, but the world is not just mysql. There are _escape_string functions for that: mysql_escape_string, pg_escape_string, sqlite_escape_string... Read the manual for pg_escape_string: "Use of this function is recommended instead of addslashes()." Or read sqlite_escape_string: "addslashes() should NOT be used to quote your strings for SQLite queries; it will lead to strange results when retrieving your data." Ooooops. So I propose moving check_query() declaration from bootstrap.inc to database.*.inc. I've already written a letter to the devel list about this, but drumm said "code is better than talk" so here is a patch. At this moment, this does absolutely nothing, only opens the possibility to use the aforementioned escape_string functions. But database.sqlite.inc simply will not work without this change. ------------------------------------------------------------------------ November 19, 2004 - 21:42 : drumm Looks straightfoward enough. Is there any reason to not put in pg_escape_string() now? ------------------------------------------------------------------------ November 19, 2004 - 21:47 : chx Actually, yes. Read on the mentioned manual page: "Note: This function requires PostgreSQL 7.2 or later." I have no idea how the check it. There is a pg_version() but I will leave this postgres business to someone who has a working postgres install to code and check this thing. ------------------------------------------------------------------------ November 19, 2004 - 22:58 : Chris Johnson PostgreSQL 7.2 was released February 4, 2002. Major production releases 73. and 7.4, along with many point releases, have been issued since then. Current production is 7.4.6 and 8.0 is in the 4th revision of beta. It seems pretty safe to assume that any Drupal installation using PostgreSQL would have version 7.2 or later. Document if a user encounters errors with PostgreSQL that they need to check the version is 7.2 or later. ------------------------------------------------------------------------ November 20, 2004 - 15:42 : Dries +1 I suggest creating a db_escape_string() function, and removing check_query() all together. ------------------------------------------------------------------------ November 20, 2004 - 15:55 : Goba +1 on Dries ------------------------------------------------------------------------ November 20, 2004 - 19:32 : chx Attachment: http://drupal.org/files/issues/check_output_patch_0.txt (1.6 KB) db_escape_string sounds find to me, it's really a better name, thanks Dries. But removing check_query? I'd suggest deprecating it and removing it later. I am afraid It would break a lot of custom modules. Here is a slightly modified patch with pg_escape_string included and documented. ------------------------------------------------------------------------ November 20, 2004 - 19:43 : killes(a)www.drop.org We were never afraid of breakign things for the good of the project. Please don't change this policy. ------------------------------------------------------------------------ November 20, 2004 - 20:26 : chx Attachment: http://drupal.org/files/issues/db_escape_string.txt (20.87 KB) Let's do it. ------------------------------------------------------------------------ November 20, 2004 - 20:29 : chx Attachment: http://drupal.org/files/issues/db_escape_string_0.txt (20.88 KB) Something funny happened to the PostgreSQL db_escape_string comment and I have missed it, sorry. Reposting the patch. ------------------------------------------------------------------------ November 21, 2004 - 10:29 : Dries I committed the patch. Thanks chx. Keep it coming. I've also updated the upgrade notes at http://drupal.org/node/12347. I'll update some contributed modules unless someone objects. ------------------------------------------------------------------------ November 21, 2004 - 13:27 : chx Attachment: http://drupal.org/files/issues/mysql_escape_patch.txt (572 bytes) Keep it coming? Well, let's see the MySQL case... It's far from being trivial, and I expect some controversy over it. ------------------------------------------------------------------------ March 10, 2005 - 01:36 : chx Attachment: http://drupal.org/files/issues/mysql_escape_patch_0.txt (573 bytes) OK, here is mysql version a bit corrected, this should get into 4.6 -- addslashes is not a correct way any more. ------------------------------------------------------------------------ March 10, 2005 - 20:29 : Dries Can we read up on that somewhere? ------------------------------------------------------------------------ March 10, 2005 - 20:37 : Goba Dries, sure: http://php.net/mysql_escape_string [1] and http://php.net/mysql_real_escape_string [2]. The latter also uses the connections charset settings to properly escape, so it is the best (but is not available in old PHP versions). The addslashes page user notes also have more information [3]. [1] http://php.net/mysql_escape_string [2] http://php.net/mysql_real_escape_string [3] http://hu.php.net/addslashes#20013 ------------------------------------------------------------------------ March 12, 2005 - 10:50 : Dries addslashes() has been working for ages (and is simpler code-wise). I guess the mysql_ functions are slightly more secure or not even that? That aside, I'm not really a big fan of version_compare()s. Also note that the patch violates Drupal's coding standards (again) but that's easy enough to fix. ------------------------------------------------------------------------ March 13, 2005 - 21:49 : chx I can only repeat what Goba said: . mysql_real_escape_string uses the connections charset settings to properly escape, so it is the best (but is not available in old PHP versions).

1 0

[drupal-devel] [bug] Menu cache prohibiting access for users
by Steven 01 Jun '05

01 Jun '05

Issue status update for http://drupal.org/node/24034 Project: Drupal Version: 4.6.0 Component: menu system Category: bug reports Priority: critical Assigned to: Anonymous Reported by: nysus Updated by: Steven -Status: active +Status: patch Attachment: http://drupal.org/files/issues/user.menu.cache.patch (1.62 KB) Patch attached. Steven Previous comments: ------------------------------------------------------------------------ June 1, 2005 - 02:05 : nysus See http://drupal.org/node/24019 for description of the problem.

1 0