[Rt-commit] rt branch, 3.8.10-releng, updated. rt-3.8.9-91-gd2055eb
Kevin Falcone
falcone at bestpractical.com
Thu Apr 14 10:12:55 EDT 2011
The branch, 3.8.10-releng has been updated
via d2055ebe2f27a38ea34dcd269978851e1a5d4ddd (commit)
via 572c5725e4c760a69a9848bacd598ca351a2f205 (commit)
via 992045b7fb51635c734a739d1f7cabe7c4f4c614 (commit)
via a251bce4a9760789c15db290b70c5d5de87912c1 (commit)
via c59d8f3dcb1c3c436de8981fddbb53f102f2e55e (commit)
via 34d6eaac2eb3df17296c0a9a6c303d82d61f6e7f (commit)
via 20b23eaa127744e5d99897724d6ae9bdac835c31 (commit)
via ef5c2c3c1eb8ef77e232f99cb5d9022af778a67c (commit)
via 01cea1c7bb1f6fdc4e11d3a8d851ef3bd23229f6 (commit)
via 3e0d834c14647b7cc4161fecc66112e6c1be5322 (commit)
via dc839582da7f3f2789bc375f02f01e618275c1c8 (commit)
via 18f490f5206f3189131c1f1aa8e1c5c1bf613660 (commit)
via f6acc6cf043408116a622acd315b59563c7631d6 (commit)
via f43d0452fb16dd444f36de1f56b619a78124f662 (commit)
via 4692fcbdcbb219ab72cd146bef140f445ad17367 (commit)
via 9c30ac738ea5cce86176d8036ee40a0aa74a4209 (commit)
via c2291f59683484d54e0a342ca920ca305040bce5 (commit)
via 06e708c6954e5a808310f2d9adeeddcf94133060 (commit)
via 7824737c6e3a43f79f374f3f310c77ba36421f0a (commit)
via 528ad20ced2a457c2989590b1e223bfe5e60e38a (commit)
via 55679544e461d916af8a2b3c458375fc5782a607 (commit)
via a9ea4c1be2206e95605e784d12f3ebed3abe0cbf (commit)
via dc5c15d849c1ef06cd7fd9341147e0b2abae222f (commit)
via 0d5d5d887dc90b50e18d9cf1ca515aa41ff6fd68 (commit)
via e31944c036e3bdb445eea20bc99fa545c879c1e7 (commit)
via 093d627de579d8175f22f9d2f1412b822af4e257 (commit)
via cf01c196b3d9a89a92988044ea1009be770da128 (commit)
via 6d19b89c55021834a7491158976df7f43f1ba726 (commit)
via 44724a010f30303f9e0592aff28587ba7b1c7f8a (commit)
via 862c1471aba2a8d55f739ae92f191973b272f91e (commit)
via d825e6a2d55200508d02fb427a12affd428c9c39 (commit)
via 103d7e0a1d9369dd8f20588da30f6dc0a93c9d15 (commit)
via a19c0edd65fdb8d7181c18a14fded91fd2baeef7 (commit)
via e43c99db83b0300a665f59b7fc8cc9fbf4fc8959 (commit)
via 78830a59954371e5c8d7d229d0171915b2b39f75 (commit)
via 236c3de77467f4498ac2fa57cb7df392e6960e7f (commit)
via 97127fb9551fb3b71243a203b904e31322f64e61 (commit)
via d7dcbef684eac6240b110fd34be2d60907d56232 (commit)
via e5e4675d9578637b259cd0aef66d75e93ca44243 (commit)
via 3ec0091143b32234ffa0faac4379eb043e8bbfac (commit)
via c65f17cb6edbbf740568fac731e0fc1e28619f8e (commit)
via c9546687690fca3dfcc5c25f352d455c4db6c783 (commit)
via f1f39bb646a68b88412fe22a15dc0ed605d9faf4 (commit)
via 61ecfd19350ec99dc0d0702f783cf300a87d1010 (commit)
via a50dd7649985d656557c94cfb46570d16ffcdb4d (commit)
from bd0ee2574eba5983275033092d278f949673c7c7 (commit)
Summary of changes:
bin/fastcgi_server.in | 12 +++
bin/mason_handler.fcgi.in | 11 +++
bin/mason_handler.scgi.in | 12 +++
bin/mason_handler.svc.in | 11 +++
bin/webmux.pl.in | 14 ++++
configure.ac | 2 +-
lib/RT/CustomFieldValues/External.pm | 84 +++++++++-----------
lib/RT/Interface/Web.pm | 33 ++++++++
lib/RT/Interface/Web/Standalone.pm | 9 ++
lib/RT/SearchBuilder.pm | 50 ++++++++++++-
lib/RT/Tickets_Overlay.pm | 31 +++++++-
share/html/Elements/Header | 3 +-
share/html/NoAuth/Logout.html | 3 +-
share/html/{Admin => NoAuth/RichText}/autohandler | 13 ++--
share/html/Search/Chart | 3 +-
share/html/Search/Elements/Chart | 2 +
share/html/Search/Elements/SelectPersonType | 2 +-
share/html/Tools/Reports/ResolvedByDates.html | 2 +-
share/html/Tools/Reports/ResolvedByOwner.html | 2 +-
t/api/tickets_overlay_sql.t | 27 ++++++-
t/web/charting.t | 69 +++++++++++++++++
t/web/compilation_errors.t | 2 +-
t/web/path-traversal.t | 40 ++++++++++
t/web/private-components.t | 40 ++++++++++
t/web/query_builder.t | 29 +++++++-
t/web/richtext-autohandler.t | 13 +++
26 files changed, 450 insertions(+), 69 deletions(-)
copy share/html/{Admin => NoAuth/RichText}/autohandler (90%)
create mode 100644 t/web/charting.t
create mode 100644 t/web/path-traversal.t
create mode 100644 t/web/private-components.t
create mode 100644 t/web/richtext-autohandler.t
- Log -----------------------------------------------------------------
commit a50dd7649985d656557c94cfb46570d16ffcdb4d
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Fri Mar 25 16:16:22 2011 -0400
Tests for exposing private componets
diff --git a/t/web/private-components.t b/t/web/private-components.t
new file mode 100644
index 0000000..0d5f000
--- /dev/null
+++ b/t/web/private-components.t
@@ -0,0 +1,35 @@
+use strict;
+
+use RT::Test tests => 24;
+my ($baseurl, $agent) = RT::Test->started_ok;
+
+ok $agent->login, 'logged in';
+
+$agent->get_ok("/Elements/Refresh?Name=private");
+$agent->content_lacks("private");
+$agent->content_lacks("Refresh this page every");
+
+$agent->get_ok("/Ticket/Elements/ShowTime?minutes=42");
+$agent->content_lacks("42 min");
+
+$agent->get_ok("/Widgets/TitleBox?title=private");
+$agent->content_lacks("private");
+
+$agent->get_ok("/m/_elements/header?title=private");
+$agent->content_lacks("private");
+
+$agent->get_ok("/autohandler");
+$agent->content_lacks("comp called without component");
+
+$agent->get_ok("/NoAuth/js/autohandler");
+$agent->content_lacks("no next component");
+
+$agent->get_ok("/l");
+$agent->content_lacks("No handle/phrase");
+
+$agent->get_ok("/%61utohandler");
+$agent->content_lacks("comp called without component");
+
+$agent->get_ok("/%45lements/Refresh?Name=private");
+$agent->content_lacks("private");
+$agent->content_lacks("Refresh this page every");
commit 61ecfd19350ec99dc0d0702f783cf300a87d1010
Author: Shawn M Moore <sartak at bestpractical.com>
Date: Mon Mar 28 10:05:09 2011 -0400
All of these requests oughta result in an error code
diff --git a/t/web/private-components.t b/t/web/private-components.t
index 0d5f000..ceb2b34 100644
--- a/t/web/private-components.t
+++ b/t/web/private-components.t
@@ -5,31 +5,40 @@ my ($baseurl, $agent) = RT::Test->started_ok;
ok $agent->login, 'logged in';
-$agent->get_ok("/Elements/Refresh?Name=private");
+$agent->get("/Elements/Refresh?Name=private");
+is($agent->status, 403);
$agent->content_lacks("private");
$agent->content_lacks("Refresh this page every");
-$agent->get_ok("/Ticket/Elements/ShowTime?minutes=42");
+$agent->get("/Ticket/Elements/ShowTime?minutes=42");
+is($agent->status, 403);
$agent->content_lacks("42 min");
-$agent->get_ok("/Widgets/TitleBox?title=private");
+$agent->get("/Widgets/TitleBox?title=private");
+is($agent->status, 403);
$agent->content_lacks("private");
-$agent->get_ok("/m/_elements/header?title=private");
+$agent->get("/m/_elements/header?title=private");
+is($agent->status, 403);
$agent->content_lacks("private");
-$agent->get_ok("/autohandler");
+$agent->get("/autohandler");
+is($agent->status, 403);
$agent->content_lacks("comp called without component");
-$agent->get_ok("/NoAuth/js/autohandler");
+$agent->get("/NoAuth/js/autohandler");
+is($agent->status, 403);
$agent->content_lacks("no next component");
-$agent->get_ok("/l");
+$agent->get("/l");
+is($agent->status, 403);
$agent->content_lacks("No handle/phrase");
-$agent->get_ok("/%61utohandler");
+$agent->get("/%61utohandler");
+is($agent->status, 403);
$agent->content_lacks("comp called without component");
-$agent->get_ok("/%45lements/Refresh?Name=private");
+$agent->get("/%45lements/Refresh?Name=private");
+is($agent->status, 403);
$agent->content_lacks("private");
$agent->content_lacks("Refresh this page every");
commit f1f39bb646a68b88412fe22a15dc0ed605d9faf4
Author: Shawn M Moore <sartak at bestpractical.com>
Date: Mon Mar 28 10:05:45 2011 -0400
First pass at MaybeRejectPrivateComponentRequest
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 90a699f..ed05f45 100755
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -195,6 +195,8 @@ sub HandleRequest {
# Process session-related callbacks before any auth attempts
$HTML::Mason::Commands::m->callback( %$ARGS, CallbackName => 'Session', CallbackPage => '/autohandler' );
+ MaybeRejectPrivateComponentRequest();
+
MaybeShowNoAuthPage($ARGS);
AttemptExternalAuth($ARGS) if RT->Config->Get('WebExternalAuthContinuous') or not _UserLoggedIn();
@@ -412,6 +414,34 @@ sub MaybeShowNoAuthPage {
$m->abort;
}
+=head2 MaybeRejectPrivateComponentRequest
+
+This function will reject calls to private components, like those under
+C</Elements>. If the requested path is a private component then we will
+abort with a C<403> error.
+
+=cut
+
+sub MaybeRejectPrivateComponentRequest {
+ my $m = $HTML::Mason::Commands::m;
+ my $path = $m->base_comp->path;
+
+ if ($path =~ m{
+ / # leading slash
+ ( Elements |
+ _elements | # mobile UI
+ Widgets |
+ dhandler |
+ autohandler |
+ l ) # loc component
+ ( $ | / ) # trailing slash or end of path
+ }xi) {
+ $m->abort(403);
+ }
+
+ return;
+}
+
=head2 ShowRequestedPage \%ARGS
This function, called exclusively by RT's autohandler, dispatches
commit c9546687690fca3dfcc5c25f352d455c4db6c783
Author: Shawn M Moore <sartak at bestpractical.com>
Date: Mon Mar 28 10:35:50 2011 -0400
Use the requested path directly for private component checking
Otherwise Mason resolves the requested url into a file, and so we
see that the user "tried" to request a dhandler even if they did the
right thing and requested (say) squished CSS.
We have tests to ensure that PATH_INFO is properly sanitized and unescaped.
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index ed05f45..60515f8 100755
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -423,20 +423,19 @@ abort with a C<403> error.
=cut
sub MaybeRejectPrivateComponentRequest {
- my $m = $HTML::Mason::Commands::m;
- my $path = $m->base_comp->path;
+ my $path = $ENV{PATH_INFO};
if ($path =~ m{
/ # leading slash
( Elements |
_elements | # mobile UI
Widgets |
- dhandler |
- autohandler |
+ dhandler | # requesting this directly is suspicious
+ autohandler | # ditto
l ) # loc component
( $ | / ) # trailing slash or end of path
}xi) {
- $m->abort(403);
+ $HTML::Mason::Commands::m->abort(403);
}
return;
commit c65f17cb6edbbf740568fac731e0fc1e28619f8e
Author: Shawn M Moore <sartak at bestpractical.com>
Date: Mon Mar 28 11:32:00 2011 -0400
Explain why we're using PATH_INFO instead of request_comp
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 60515f8..cca9b4b 100755
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -423,6 +423,14 @@ abort with a C<403> error.
=cut
sub MaybeRejectPrivateComponentRequest {
+ # We must use PATH_INFO directly because $m->base_comp and $m->request_comp
+ # are resolved to a file on disk, which is too late. This means that
+ # requesting squished CSS looks like the user requested the dhandler
+ # directly. What we really want is the URL directly requested by the user.
+ # Unfortunately we don't keep the Plack::Request around. If we did, it'd be
+ # cleaner to use $request->path_info here. Either way, PATH_INFO is
+ # unescaped for us, so requesting /%61utohandler doesn't circumvent this
+ # check. See t/web/private-components.t
my $path = $ENV{PATH_INFO};
if ($path =~ m{
commit 3ec0091143b32234ffa0faac4379eb043e8bbfac
Author: Shawn M Moore <sartak at bestpractical.com>
Date: Mon Mar 28 11:35:22 2011 -0400
More explanation
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index cca9b4b..cae6ce3 100755
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -424,13 +424,14 @@ abort with a C<403> error.
sub MaybeRejectPrivateComponentRequest {
# We must use PATH_INFO directly because $m->base_comp and $m->request_comp
- # are resolved to a file on disk, which is too late. This means that
- # requesting squished CSS looks like the user requested the dhandler
- # directly. What we really want is the URL directly requested by the user.
- # Unfortunately we don't keep the Plack::Request around. If we did, it'd be
+ # are resolved to a file on disk, which is too late, because when the user
+ # requests squished CSS, it looks like they requested the dhandler
+ # directly. What we really want is more or less the URL from the HTTP
+ # request. Unfortunately we don't keep the Plack::Request around that we
+ # construct in RT::Interface::Web::Handler::PSGIApp. If we did, it'd be
# cleaner to use $request->path_info here. Either way, PATH_INFO is
# unescaped for us, so requesting /%61utohandler doesn't circumvent this
- # check. See t/web/private-components.t
+ # check. See t/web/private-components.t.
my $path = $ENV{PATH_INFO};
if ($path =~ m{
commit e5e4675d9578637b259cd0aef66d75e93ca44243
Author: Shawn M Moore <sartak at bestpractical.com>
Date: Mon Mar 28 12:08:10 2011 -0400
Use request_comp but don't check for dhandler
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index cae6ce3..f56abb3 100755
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -423,28 +423,23 @@ abort with a C<403> error.
=cut
sub MaybeRejectPrivateComponentRequest {
- # We must use PATH_INFO directly because $m->base_comp and $m->request_comp
- # are resolved to a file on disk, which is too late, because when the user
- # requests squished CSS, it looks like they requested the dhandler
- # directly. What we really want is more or less the URL from the HTTP
- # request. Unfortunately we don't keep the Plack::Request around that we
- # construct in RT::Interface::Web::Handler::PSGIApp. If we did, it'd be
- # cleaner to use $request->path_info here. Either way, PATH_INFO is
- # unescaped for us, so requesting /%61utohandler doesn't circumvent this
- # check. See t/web/private-components.t.
- my $path = $ENV{PATH_INFO};
+ my $m = $HTML::Mason::Commands::m;
+ my $path = $m->request_comp->path;
+
+ # We do not check for dhandler here, because requesting our dhandlers
+ # directly is okay. Mason will invoke the dhandler with a dhandler_arg of
+ # 'dhandler'.
if ($path =~ m{
/ # leading slash
( Elements |
_elements | # mobile UI
Widgets |
- dhandler | # requesting this directly is suspicious
- autohandler | # ditto
+ autohandler | # requesting this directly is suspicious
l ) # loc component
( $ | / ) # trailing slash or end of path
}xi) {
- $HTML::Mason::Commands::m->abort(403);
+ $m->abort(403);
}
return;
commit d7dcbef684eac6240b110fd34be2d60907d56232
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Fri Apr 1 15:29:34 2011 -0400
Stop direct access to richtext editor files
We have a dhandler that rewrites access, but nothing that stopped you
from directly invoking one of the files under the RichText editor.
This means you could ask mason to run a .js file or a .png.
You shouldn't be able to do anything interesting with this, but it seems
wrong so we're stopping it by providing an autohandler.
However, the .js or .png is still compiled before mason consults the
autohandler. Please don't add BEGIN blocks to your .js or .png files
or to the GPL
We should really have static content outside the mason component root
diff --git a/share/html/NoAuth/RichText/autohandler b/share/html/NoAuth/RichText/autohandler
new file mode 100644
index 0000000..fd48b59
--- /dev/null
+++ b/share/html/NoAuth/RichText/autohandler
@@ -0,0 +1,56 @@
+%# BEGIN BPS TAGGED BLOCK {{{
+%#
+%# COPYRIGHT:
+%#
+%# This software is Copyright (c) 1996-2011 Best Practical Solutions, LLC
+%# <sales at bestpractical.com>
+%#
+%# (Except where explicitly superseded by other copyright notices)
+%#
+%#
+%# LICENSE:
+%#
+%# This work is made available to you under the terms of Version 2 of
+%# the GNU General Public License. A copy of that license should have
+%# been provided with this software, but in any event can be snarfed
+%# from www.gnu.org.
+%#
+%# This work is distributed in the hope that it will be useful, but
+%# WITHOUT ANY WARRANTY; without even the implied warranty of
+%# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+%# General Public License for more details.
+%#
+%# You should have received a copy of the GNU General Public License
+%# along with this program; if not, write to the Free Software
+%# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
+%# 02110-1301 or visit their web page on the internet at
+%# http://www.gnu.org/licenses/old-licenses/gpl-2.0.html.
+%#
+%#
+%# CONTRIBUTION SUBMISSION POLICY:
+%#
+%# (The following paragraph is not intended to limit the rights granted
+%# to you to modify and distribute this software under the terms of
+%# the GNU General Public License and is only of importance to you if
+%# you choose to contribute your changes and enhancements to the
+%# community by submitting them to Best Practical Solutions, LLC.)
+%#
+%# By intentionally submitting any modifications, corrections or
+%# derivatives to this work, or any other work intended for use with
+%# Request Tracker, to Best Practical Solutions, LLC, you confirm that
+%# you are the copyright holder for those contributions and you grant
+%# Best Practical Solutions, LLC a nonexclusive, worldwide, irrevocable,
+%# royalty-free, perpetual, license to use, copy, create derivative
+%# works based on those contributions, and sublicense and distribute
+%# those contributions and any derivatives thereof.
+%#
+%# END BPS TAGGED BLOCK }}}
+<%init>
+my $file = $m->base_comp->source_file;
+if ($file =~ m{RichText/+FCKeditor}) {
+ $RT::Logger->crit("Invalid request directly to the rich text editor: $file");
+ $m->abort(403);
+} else {
+ $m->call_next();
+}
+</%init>
diff --git a/t/web/richtext-autohandler.t b/t/web/richtext-autohandler.t
new file mode 100644
index 0000000..56617b2
--- /dev/null
+++ b/t/web/richtext-autohandler.t
@@ -0,0 +1,13 @@
+use strict;
+
+use RT::Test tests => 7;
+my ($baseurl, $agent) = RT::Test->started_ok;
+
+$agent->get("$baseurl/NoAuth/RichText/FCKeditor/license.txt");
+is($agent->status, 403);
+$agent->content_lacks("It is not the purpose of this section to induce");
+
+$agent->get_ok("/NoAuth/RichText/license.txt");
+$agent->content_contains("It is not the purpose of this section to induce");
+
+$agent->warning_like(qr/Invalid request directly to the rich text editor/,);
commit 97127fb9551fb3b71243a203b904e31322f64e61
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Mon Mar 28 18:33:47 2011 -0400
Restrict PrimaryGroupBy to only the explicit options that we offer
Not limiting the PrimaryGroupBy allows:
* SQL injection; the "Group By" clause is not escaped when inserted
into the generated SQL. Use of DBI's prepared statements disallows
the attacker from running more than one SQL statement, thus
precluding destructive queries such as "DELETE FROM" or "DROP TABLE",
but complex data extraction is still feasable.
* Information leakage; we blindly allow grouping even by valid columns
such as 'Subject', which allows an attacker to see the subjects of
tickets they do not otherwise have permission to view.
$UseSQLForACLChecks protects against this by pre-limiting the
resultset to tickets the current user has access to.
diff --git a/share/html/Search/Chart b/share/html/Search/Chart
index 429fea5..7bcb8d0 100644
--- a/share/html/Search/Chart
+++ b/share/html/Search/Chart
@@ -66,7 +66,8 @@ if ($ChartStyle eq 'pie') {
use RT::Report::Tickets;
my $tix = RT::Report::Tickets->new( $session{'CurrentUser'} );
-
+my %AllowedGroupings = reverse $tix->Groupings( Query => $Query );
+$PrimaryGroupBy = 'Queue' unless exists $AllowedGroupings{$PrimaryGroupBy};
my ($count_name, $value_name) = $tix->SetupGroupings(
Query => $Query, GroupBy => $PrimaryGroupBy,
);
diff --git a/share/html/Search/Elements/Chart b/share/html/Search/Elements/Chart
index b74aba4..e25a9ef 100644
--- a/share/html/Search/Elements/Chart
+++ b/share/html/Search/Elements/Chart
@@ -56,6 +56,8 @@ use RT::Report::Tickets;
$PrimaryGroupBy ||= 'Queue'; # make sure PrimaryGroupBy is not undef
my $tix = RT::Report::Tickets->new( $session{'CurrentUser'} );
+my %AllowedGroupings = reverse $tix->Groupings( Query => $Query );
+$PrimaryGroupBy = 'Queue' unless exists $AllowedGroupings{$PrimaryGroupBy};
my ($count_name, $value_name) = $tix->SetupGroupings(
Query => $Query, GroupBy => $PrimaryGroupBy,
);
diff --git a/t/web/charting.t b/t/web/charting.t
new file mode 100644
index 0000000..5618762
--- /dev/null
+++ b/t/web/charting.t
@@ -0,0 +1,69 @@
+use strict;
+use warnings;
+
+use RT::Test no_plan => 1;
+
+for my $n (1..7) {
+ my $ticket = RT::Ticket->new( RT->SystemUser );
+ my $req = 'root' . ($n % 2) . '@localhost';
+ my ( $ret, $msg ) = $ticket->Create(
+ Subject => "base ticket $_",
+ Queue => "General",
+ Owner => "root",
+ Requestor => $req,
+ MIMEObj => MIME::Entity->build(
+ From => $req,
+ To => 'rt at localhost',
+ Subject => "base ticket $_",
+ Data => "Content $_",
+ ),
+ );
+ ok( $ret, "ticket $n created: $msg" );
+}
+
+my ($url, $m) = RT::Test->started_ok;
+ok( $m->login, "Logged in" );
+
+# Test that defaults work
+$m->get_ok( "/Search/Chart.html?Query=id>0" );
+$m->content_like(qr{<th[^>]*>Queue\s*</th>\s*<th[^>]*>Tickets\s*</th>}, "Grouped by queue");
+$m->content_like(qr{General</a>\s*</td>\s*<td[^>]*>\s*<a[^>]*>7</a>}, "Found results in table");
+$m->content_like(qr{<img src="/Search/Chart\?}, "Found image");
+
+$m->get_ok( "/Search/Chart?Query=id>0" );
+is( $m->content_type, "image/png" );
+ok( length($m->content), "Has content" );
+
+
+# Group by Queue
+$m->get_ok( "/Search/Chart.html?Query=id>0&PrimaryGroupBy=Queue" );
+$m->content_like(qr{<th[^>]*>Queue\s*</th>\s*<th[^>]*>Tickets\s*</th>}, "Grouped by queue");
+$m->content_like(qr{General</a>\s*</td>\s*<td[^>]*>\s*<a[^>]*>7</a>}, "Found results in table");
+$m->content_like(qr{<img src="/Search/Chart\?}, "Found image");
+
+$m->get_ok( "/Search/Chart?Query=id>0&PrimaryGroupBy=Queue" );
+is( $m->content_type, "image/png" );
+ok( length($m->content), "Has content" );
+
+
+# Group by Requestor email
+$m->get_ok( "/Search/Chart.html?Query=id>0&PrimaryGroupBy=Requestor.EmailAddress" );
+$m->content_like(qr{<th[^>]*>Requestor\.EmailAddress\s*</th>\s*<th[^>]*>Tickets\s*</th>},
+ "Grouped by requestor");
+$m->content_like(qr{root0\@localhost</a>\s*</td>\s*<td[^>]*>\s*<a[^>]*>3</a>}, "Found results in table");
+$m->content_like(qr{<img src="/Search/Chart\?}, "Found image");
+
+$m->get_ok( "/Search/Chart?Query=id>0&PrimaryGroupBy=Requestor.Email" );
+is( $m->content_type, "image/png" );
+ok( length($m->content), "Has content" );
+
+
+# Group by Requestor phone -- which is bogus, and falls back to queue
+$m->get_ok( "/Search/Chart.html?Query=id>0&PrimaryGroupBy=Requestor.Phone" );
+$m->content_like(qr{General</a>\s*</td>\s*<td[^>]*>\s*<a[^>]*>7</a>},
+ "Found queue results in table, as a default");
+$m->content_like(qr{<img src="/Search/Chart\?}, "Found image");
+
+$m->get_ok( "/Search/Chart?Query=id>0&PrimaryGroupBy=Requestor.Phone" );
+is( $m->content_type, "image/png" );
+ok( length($m->content), "Has content" );
commit 236c3de77467f4498ac2fa57cb7df392e6960e7f
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Fri Apr 1 11:46:26 2011 -0400
Prevent FIELD- and OPERATOR- based SQL injection at the RT::SB level
Provide a whitelist of possible operators to use, and restrict field to
being /^\w*$/. Any arguments not matching this will instead log a
critical message and insert a clause which should never match.
In the two places we attempt to be clever, and generate the SQL
"FUNCTION( alias.field ) = 42" by putting "FUNCTION( alias" as the
ALIAS, and "field )" as the FIELD -- which violates this constraint.
Provide a new FUNCTION parameter, which walks around the new
restriction, and does the split across the '.' for you. This should
only be used if you are certain that you trust the data that the
FUNCTION argument is built up from.
diff --git a/lib/RT/SearchBuilder.pm b/lib/RT/SearchBuilder.pm
index be64a2c..e38047e 100755
--- a/lib/RT/SearchBuilder.pm
+++ b/lib/RT/SearchBuilder.pm
@@ -278,10 +278,32 @@ match lower(colname) agaist lc($val);
sub Limit {
my $self = shift;
- my %args = ( CASESENSITIVE => 1,
- @_ );
+ my %ARGS = (
+ CASESENSITIVE => 1,
+ OPERATOR => '=',
+ @_,
+ );
- return $self->SUPER::Limit(%args);
+ if ($ARGS{FUNCTION}) {
+ ($ARGS{ALIAS}, $ARGS{FIELD}) = split /\./, delete $ARGS{FUNCTION}, 2;
+ $self->SUPER::Limit(%ARGS);
+ } elsif ($ARGS{FIELD} =~ /\W/
+ or $ARGS{OPERATOR} !~ /^(=|<|>|!=|<>|<=|>=
+ |(NOT\s*)?LIKE
+ |(NOT\s*)?(STARTS|ENDS)WITH
+ |(NOT\s*)?MATCHES
+ |IS(\s*NOT)?
+ |IN)$/ix) {
+ $RT::Logger->crit("Possible SQL injection attack: $ARGS{FIELD} $ARGS{OPERATOR}");
+ $self->SUPER::Limit(
+ %ARGS,
+ FIELD => 'id',
+ OPERATOR => '<',
+ VALUE => '0',
+ );
+ } else {
+ $self->SUPER::Limit(%ARGS);
+ }
}
=head2 ItemsOrderBy
commit 78830a59954371e5c8d7d229d0171915b2b39f75
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed Mar 30 16:53:30 2011 -0400
Lock down possible OCFV columns to the two that we use
The only codepath which uses the CF.{foo}.Content form is
_CustomFieldLimit itself, which calls into itself for IP address ranges.
However, this provides a SQL injection, since it does not check the
column given, nor does SearchBuilder escape it. Limit it to the only
two valid column types.
diff --git a/lib/RT/Tickets_Overlay.pm b/lib/RT/Tickets_Overlay.pm
index e4020f6..220b4ee 100755
--- a/lib/RT/Tickets_Overlay.pm
+++ b/lib/RT/Tickets_Overlay.pm
@@ -1202,7 +1202,7 @@ Try and turn a CF descriptor into (cfid, cfname) object pair.
sub _CustomFieldDecipher {
my ($self, $string) = @_;
- my ($queue, $field, $column) = ($string =~ /^(?:(.+?)\.)?{(.+)}(?:\.(.+))?$/);
+ my ($queue, $field, $column) = ($string =~ /^(?:(.+?)\.)?{(.+)}(?:\.(Content|LargeContent))?$/);
$field ||= ($string =~ /^{(.*?)}$/)[0] || $string;
my $cf;
commit e43c99db83b0300a665f59b7fc8cc9fbf4fc8959
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Tue Mar 29 15:42:14 2011 -0400
Use closures instead of eval to construct external CF limits
This prevents an arbitrary code execution vulnerability in
__BuildLimitCheck, wherein the $quoted_value scalar would fail to escape
\'s, allowing the user-provided VALUE to escape the quoted string.
Simply replace the eval with a closure, which does not have the same
vulnerabilities.
The other use of eval, in __BuildAggregatorsCheck, does not interpolate
user-provided data, but has also been removed in the interests of
clarity and bullet-proofing. In doing so, the interpretation of
mixed-entryaggregators has changed slightly; association is now
left-to-right, not the previous defualt of && binding tighter than ||.
diff --git a/lib/RT/CustomFieldValues/External.pm b/lib/RT/CustomFieldValues/External.pm
index 997108d..81aaa2b 100644
--- a/lib/RT/CustomFieldValues/External.pm
+++ b/lib/RT/CustomFieldValues/External.pm
@@ -123,57 +123,49 @@ sub __BuildLimitCheck {
my ($self, %args) = (@_);
return undef unless $args{'FIELD'} =~ /^(?:Name|Description)$/;
- $args{'OPERATOR'} ||= '=';
- my $quoted_value = $args{'VALUE'};
- if ( $quoted_value ) {
- $quoted_value =~ s/'/\\'/g;
- $quoted_value = "'$quoted_value'";
- }
-
- my $code = <<END;
-my \$record = shift;
-my \$value = \$record->$args{'FIELD'};
-my \$condition = $quoted_value;
-END
-
- if ( $args{'OPERATOR'} =~ /^(?:=|!=|<>)$/ ) {
- $code .= 'return 0 unless defined $value;';
- my %h = ( '=' => ' eq ', '!=' => ' ne ', '<>' => ' ne ' );
- $code .= 'return 0 unless $value'. $h{ $args{'OPERATOR'} } .'$condition;';
- $code .= 'return 1;'
- }
- elsif ( $args{'OPERATOR'} =~ /^(?:LIKE|NOT LIKE)$/i ) {
- $code .= 'return 0 unless defined $value;';
- my %h = ( 'LIKE' => ' =~ ', 'NOT LIKE' => ' !~ ' );
- $code .= 'return 0 unless $value'. $h{ uc $args{'OPERATOR'} } .'/\Q$condition/i;';
- $code .= 'return 1;'
- }
- else {
- $code .= 'return 0;'
- }
- $code = "sub {$code}";
- my $cb = eval "$code";
- $RT::Logger->error( "Couldn't build callback '$code': $@" ) if $@;
- return $cb;
+ my $condition = $args{VALUE};
+ my $op = $args{'OPERATOR'} || '=';
+ my $field = $args{FIELD};
+
+ return sub {
+ my $record = shift;
+ my $value = $record->$field;
+ return 0 unless defined $value;
+ if ($op eq "=") {
+ return 0 unless $value eq $condition;
+ } elsif ($op eq "!=" or $op eq "<>") {
+ return 0 unless $value ne $condition;
+ } elsif (uc($op) eq "LIKE") {
+ return 0 unless $value =~ /\Q$condition\E/i;
+ } elsif (rc($op) eq "NOT LIKE") {
+ return 0 unless $value !~ /\Q$condition\E/i;
+ } else {
+ return 0;
+ }
+ return 1;
+ };
}
sub __BuildAggregatorsCheck {
my $self = shift;
+ my @cbs = grep {$_->{CALLBACK}} @{ $self->{'__external_cf_limits'} };
+ return undef unless @cbs;
- my %h = ( OR => ' || ', AND => ' && ' );
-
- my $code = '';
- for( my $i = 0; $i < @{ $self->{'__external_cf_limits'} }; $i++ ) {
- next unless $self->{'__external_cf_limits'}->[$i]->{'CALLBACK'};
- $code .= $h{ uc($self->{'__external_cf_limits'}->[$i]->{'ENTRYAGGREGATOR'} || 'OR') } if $code;
- $code .= '$sb->{\'__external_cf_limits\'}->['. $i .']->{\'CALLBACK\'}->($record)';
- }
- return unless $code;
+ my %h = (
+ OR => sub { defined $_[0] ? ($_[0] || $_[1]) : $_[1] },
+ AND => sub { defined $_[0] ? ($_[0] && $_[1]) : $_[1] },
+ );
- $code = "sub { my (\$sb,\$record) = (\@_); return $code }";
- my $cb = eval "$code";
- $RT::Logger->error( "Couldn't build callback '$code': $@" ) if $@;
- return $cb;
+ return sub {
+ my ($sb, $record) = @_;
+ my $ok;
+ for my $limit ( @cbs ) {
+ $ok = $h{$limit->{ENTRYAGGREGATOR} || 'OR'}->(
+ $ok, $limit->{CALLBACK}->($record),
+ );
+ }
+ return $ok;
+ };
}
sub _DoSearch {
commit a19c0edd65fdb8d7181c18a14fded91fd2baeef7
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Tue Mar 29 10:52:56 2011 -0400
Disallow SQL injection in FIELD argument to OrderBy
A number of locations blindly pass OrderBy clauses from the user into
the FIELD of OrderByCols, which provides no escaping or sanity checking.
As a short-term fix, limit the FIELD to /^[a-zA-Z0-9_]*$/ at the
RT::SearchBuilder level.
There is one place which injects logic into the ORderByCols FIELD;
namely, the "PAW sort" feature, which sorts the current user's tickets,
followed by unowned tickets, followed by all other tickets. Allow this
codepath by adding a FUNCTION field to OrderByCols which steps around
the limit mentioned above. This particular use is safe because no
user-specified data is used in the function which is built.
The severity of this SQL injection is limited by the ability to create a
single dangerous statement by appending to a "SELECT ... FROM ... ORDER
BY main." statement. The lack of parens around the SELECT statement
precludes a UNION attack, and the use of DBI prepared statements
disallows the attacker from running more than one SQL statement, thus
precluding destructive queries such as "DELETE FROM" or "DROP TABLE".
diff --git a/lib/RT/SearchBuilder.pm b/lib/RT/SearchBuilder.pm
index be64a2c..43acbdd 100755
--- a/lib/RT/SearchBuilder.pm
+++ b/lib/RT/SearchBuilder.pm
@@ -85,6 +85,17 @@ sub _Init {
$self->SUPER::_Init( 'Handle' => $RT::Handle);
}
+sub OrderByCols {
+ my $self = shift;
+ my @sort;
+ for my $s (@_) {
+ next if defined $s->{FIELD} and $s->{FIELD} =~ /\W/;
+ $s->{FIELD} = $s->{FUNCTION} if $s->{FUNCTION};
+ push @sort, $s;
+ }
+ return $self->SUPER::OrderByCols( @sort );
+}
+
=head2 LimitToEnabled
Only find items that haven't been disabled
diff --git a/lib/RT/Tickets_Overlay.pm b/lib/RT/Tickets_Overlay.pm
index e4020f6..abe810c 100755
--- a/lib/RT/Tickets_Overlay.pm
+++ b/lib/RT/Tickets_Overlay.pm
@@ -1726,9 +1726,20 @@ sub OrderByCols {
foreach my $uid ( $self->CurrentUser->Id, $RT::Nobody->Id ) {
if ( RT->Config->Get('DatabaseType') eq 'Oracle' ) {
my $f = ($row->{'ALIAS'} || 'main') .'.Owner';
- push @res, { %$row, ALIAS => '', FIELD => "CASE WHEN $f=$uid THEN 1 ELSE 0 END", ORDER => $order } ;
+ push @res, {
+ %$row,
+ FIELD => undef,
+ ALIAS => '',
+ FUNCTION => "CASE WHEN $f=$uid THEN 1 ELSE 0 END",
+ ORDER => $order
+ };
} else {
- push @res, { %$row, FIELD => "Owner=$uid", ORDER => $order } ;
+ push @res, {
+ %$row,
+ FIELD => undef,
+ FUNCTION => "Owner=$uid",
+ ORDER => $order
+ };
}
}
commit 103d7e0a1d9369dd8f20588da30f6dc0a93c9d15
Author: Shawn M Moore <sartak at bestpractical.com>
Date: Mon Apr 4 12:48:13 2011 -0400
Copy 4.0's path-traversal.t and tweak it for 3.8
diff --git a/t/web/path-traversal.t b/t/web/path-traversal.t
new file mode 100644
index 0000000..81fa2d7
--- /dev/null
+++ b/t/web/path-traversal.t
@@ -0,0 +1,15 @@
+use strict;
+use warnings;
+
+use RT::Test tests => 7;
+
+my ($baseurl, $agent) = RT::Test->started_ok;
+
+$agent->get("$baseurl/NoAuth/../Elements/HeaderJavascript");
+is($agent->status, 400);
+$agent->warning_like(qr/Invalid request.*aborting/,);
+
+$agent->get("$baseurl/NoAuth/../../../etc/RT_Config.pm");
+is($agent->status, 400);
+$agent->warning_like(qr/Invalid request.*aborting/,);
+
commit d825e6a2d55200508d02fb427a12affd428c9c39
Author: Shawn M Moore <sartak at bestpractical.com>
Date: Mon Apr 4 14:25:21 2011 -0400
Forbid /. in Standalone
This affects tests as well but not FCGI, mod_perl, etc
diff --git a/lib/RT/Interface/Web/Standalone.pm b/lib/RT/Interface/Web/Standalone.pm
index 91dbac3..3157e31 100755
--- a/lib/RT/Interface/Web/Standalone.pm
+++ b/lib/RT/Interface/Web/Standalone.pm
@@ -77,6 +77,15 @@ sub handle_request {
Module::Refresh->refresh if RT->Config->Get('DevelMode');
RT::ConnectToDatabase() unless RT->InstallMode;
+
+ # Each environment has its own way of handling .. and so on in paths,
+ # so RT consistently forbids such paths.
+ if ( $cgi->path_info =~ m{/\.} ) {
+ $RT::Logger->crit("Invalid request for ".$cgi->path_info." aborting");
+ print STDOUT "HTTP/1.0 400\r\n\r\n";
+ return RT::Interface::Web::Handler->CleanupRequest();
+ }
+
$self->SUPER::handle_request($cgi);
$RT::Logger->crit($@) if $@ && $RT::Logger;
warn $@ if $@ && !$RT::Logger;
commit 862c1471aba2a8d55f739ae92f191973b272f91e
Author: Shawn M Moore <sartak at bestpractical.com>
Date: Mon Apr 4 14:41:57 2011 -0400
Traversal protection for fastcgi_server and mason_handler.fcgi.in
diff --git a/bin/fastcgi_server.in b/bin/fastcgi_server.in
index 2aa9531..d6df63c 100644
--- a/bin/fastcgi_server.in
+++ b/bin/fastcgi_server.in
@@ -230,6 +230,18 @@ while ( my $cgi = CGI::Fast->new ) {
Module::Refresh->refresh if RT->Config->Get('DevelMode');
RT::ConnectToDatabase();
+ # Each environment has its own way of handling .. and so on in paths,
+ # so RT consistently forbids such paths.
+ if ( $cgi->path_info =~ m{/\.} ) {
+ $RT::Logger->crit("Invalid request for ".$cgi->path_info." aborting");
+ print STDOUT "HTTP/1.0 400\r\n\r\n";
+
+ RT::Interface::Web::Handler->CleanupRequest();
+ $proc_manager->pm_post_dispatch;
+
+ next;
+ }
+
my $interp = $RT::Mason::Handler->interp;
if (
!$interp->comp_exists( $cgi->path_info )
diff --git a/bin/mason_handler.fcgi.in b/bin/mason_handler.fcgi.in
index b980bc1..4682abf 100755
--- a/bin/mason_handler.fcgi.in
+++ b/bin/mason_handler.fcgi.in
@@ -68,6 +68,17 @@ while ( my $cgi = CGI::Fast->new ) {
Module::Refresh->refresh if RT->Config->Get('DevelMode');
RT::ConnectToDatabase();
+ # Each environment has its own way of handling .. and so on in paths,
+ # so RT consistently forbids such paths.
+ if ( $cgi->path_info =~ m{/\.} ) {
+ $RT::Logger->crit("Invalid request for ".$cgi->path_info." aborting");
+ print STDOUT "HTTP/1.0 400\r\n\r\n";
+
+ RT::Interface::Web::Handler->CleanupRequest();
+
+ next;
+ }
+
my $interp = $RT::Mason::Handler->interp;
if (
!$interp->comp_exists( $cgi->path_info )
commit 44724a010f30303f9e0592aff28587ba7b1c7f8a
Author: Shawn M Moore <sartak at bestpractical.com>
Date: Mon Apr 4 14:42:11 2011 -0400
Traversal protection for webmux.pl (mod_perl)
diff --git a/bin/webmux.pl.in b/bin/webmux.pl.in
index f1ab591..fe14765 100755
--- a/bin/webmux.pl.in
+++ b/bin/webmux.pl.in
@@ -94,6 +94,14 @@ sub handler {
RT::ConnectToDatabase();
+ # Each environment has its own way of handling .. and so on in paths,
+ # so RT consistently forbids such paths.
+ if ( $r->filename =~ m{/\.} ) {
+ $RT::Logger->crit("Invalid request for ".$r->filename." aborting");
+ RT::Interface::Web::Handler->CleanupRequest();
+ return 400;
+ }
+
my (%session, $status);
{
local $@;
commit 6d19b89c55021834a7491158976df7f43f1ba726
Author: Shawn M Moore <sartak at bestpractical.com>
Date: Mon Apr 4 14:42:42 2011 -0400
Traversal protection for speedycgi and svc
diff --git a/bin/mason_handler.scgi.in b/bin/mason_handler.scgi.in
index 5b6a7e2..fa771b7 100755
--- a/bin/mason_handler.scgi.in
+++ b/bin/mason_handler.scgi.in
@@ -57,6 +57,18 @@ require (dirname(__FILE__) . '/webmux.pl');
require CGI;
my $cgi = CGI->new;
+
+# Each environment has its own way of handling .. and so on in paths,
+# so RT consistently forbids such paths.
+if ( $cgi->path_info =~ m{/\.} ) {
+ $RT::Logger->crit("Invalid request for ".$cgi->path_info." aborting");
+ print STDOUT "HTTP/1.0 400\r\n\r\n";
+
+ RT::Interface::Web::Handler->CleanupRequest();
+
+ return 0;
+}
+
if ( ( !$Handler->interp->comp_exists( $cgi->path_info ) )
&& ( $Handler->interp->comp_exists( $cgi->path_info . "/index.html" ) ) ) {
$cgi->path_info( $cgi->path_info . "/index.html" );
diff --git a/bin/mason_handler.svc.in b/bin/mason_handler.svc.in
index 2cbf435..119b110 100755
--- a/bin/mason_handler.svc.in
+++ b/bin/mason_handler.svc.in
@@ -234,6 +234,17 @@ $Handler ||= RT::Interface::Web::Handler->new(
while( my $cgi = CGI::Fast->new ) {
my $comp = $ENV{'PATH_INFO'};
+ # Each environment has its own way of handling .. and so on in paths,
+ # so RT consistently forbids such paths.
+ if ( $cgi->path_info =~ m{/\.} ) {
+ $RT::Logger->crit("Invalid request for ".$cgi->path_info." aborting");
+ print STDOUT "HTTP/1.0 400\r\n\r\n";
+
+ RT::Interface::Web::Handler->CleanupRequest();
+
+ next;
+ }
+
$comp = $1 if ($comp =~ /^(.*)$/);
my $web_path = RT->Config->Get('WebPath');
$comp =~ s|^\Q$web_path\E\b||i;
commit cf01c196b3d9a89a92988044ea1009be770da128
Author: Shawn M Moore <sartak at bestpractical.com>
Date: Mon Apr 4 14:58:28 2011 -0400
path-traversal test for a SendStaticFile dhandler
diff --git a/t/web/path-traversal.t b/t/web/path-traversal.t
index 81fa2d7..53287b3 100644
--- a/t/web/path-traversal.t
+++ b/t/web/path-traversal.t
@@ -1,7 +1,7 @@
use strict;
use warnings;
-use RT::Test tests => 7;
+use RT::Test tests => 10;
my ($baseurl, $agent) = RT::Test->started_ok;
@@ -13,3 +13,7 @@ $agent->get("$baseurl/NoAuth/../../../etc/RT_Config.pm");
is($agent->status, 400);
$agent->warning_like(qr/Invalid request.*aborting/,);
+$agent->get("$baseurl/NoAuth/css/web2/images/../../../../../../etc/RT_Config.pm");
+is($agent->status, 400);
+$agent->warning_like(qr/Invalid request.*aborting/,);
+
commit 093d627de579d8175f22f9d2f1412b822af4e257
Author: Shawn M Moore <sartak at bestpractical.com>
Date: Tue Apr 5 12:26:22 2011 -0400
Construct a path we can usefully test for /. in webmux.pl
When requesting /NoAuth/../%45lements/HeaderJavascript?foo=1 these are the
responses from the various methods in Apache2::RequestRec:
filename: /opt/rt3/share/html/Elements/HeaderJavascript
path_info: (empty)
uri: /Elements/HeaderJavascript
unparsed_uri: /NoAuth/../%45lements/HeaderJavascript?foo=1
None of these are what we need. Ideally what we would get is:
ideal: /NoAuth/../Elements/HeaderJavascript
..but we don't get that, so we have to construct it ourselves from
unparsed_uri by parsing (and escaping) it. We need to make sure to ignore
query parameters in the /. check since those are perfectly valid.
diff --git a/bin/webmux.pl.in b/bin/webmux.pl.in
index fe14765..06321c0 100755
--- a/bin/webmux.pl.in
+++ b/bin/webmux.pl.in
@@ -94,10 +94,15 @@ sub handler {
RT::ConnectToDatabase();
- # Each environment has its own way of handling .. and so on in paths,
- # so RT consistently forbids such paths.
- if ( $r->filename =~ m{/\.} ) {
- $RT::Logger->crit("Invalid request for ".$r->filename." aborting");
+ # none of the methods in $r gives us the information we want (most
+ # canonicalize /foo/../bar to /bar which is exactly what we want to avoid)
+ my $uri = URI->new("http://".$r->hostname.$r->unparsed_uri);
+ my $path = URI::Escape::uri_unescape($uri->path);
+
+ ## Each environment has its own way of handling .. and so on in paths,
+ ## so RT consistently forbids such paths.
+ if ( $path =~ m{/\.} ) {
+ $RT::Logger->crit("Invalid request for ".$path." aborting");
RT::Interface::Web::Handler->CleanupRequest();
return 400;
}
commit e31944c036e3bdb445eea20bc99fa545c879c1e7
Author: Shawn M Moore <sartak at bestpractical.com>
Date: Tue Apr 5 12:36:45 2011 -0400
More tests for unsafe and safe URLs
diff --git a/t/web/path-traversal.t b/t/web/path-traversal.t
index 53287b3..8d2f5cc 100644
--- a/t/web/path-traversal.t
+++ b/t/web/path-traversal.t
@@ -1,7 +1,7 @@
use strict;
use warnings;
-use RT::Test tests => 10;
+use RT::Test tests => 20;
my ($baseurl, $agent) = RT::Test->started_ok;
@@ -9,6 +9,14 @@ $agent->get("$baseurl/NoAuth/../Elements/HeaderJavascript");
is($agent->status, 400);
$agent->warning_like(qr/Invalid request.*aborting/,);
+$agent->get("$baseurl/NoAuth/../%45lements/HeaderJavascript");
+is($agent->status, 400);
+$agent->warning_like(qr/Invalid request.*aborting/,);
+
+$agent->get("$baseurl/NoAuth/%2E%2E/Elements/HeaderJavascript");
+is($agent->status, 400);
+$agent->warning_like(qr/Invalid request.*aborting/,);
+
$agent->get("$baseurl/NoAuth/../../../etc/RT_Config.pm");
is($agent->status, 400);
$agent->warning_like(qr/Invalid request.*aborting/,);
@@ -17,3 +25,16 @@ $agent->get("$baseurl/NoAuth/css/web2/images/../../../../../../etc/RT_Config.pm"
is($agent->status, 400);
$agent->warning_like(qr/Invalid request.*aborting/,);
+# do not reject these URLs, even though they contain /. outside the path
+$agent->get("$baseurl/index.html?ignored=%2F%2E");
+is($agent->status, 200);
+
+$agent->get("$baseurl/index.html?ignored=/.");
+is($agent->status, 200);
+
+$agent->get("$baseurl/index.html#%2F%2E");
+is($agent->status, 200);
+
+$agent->get("$baseurl/index.html#/.");
+is($agent->status, 200);
+
commit 0d5d5d887dc90b50e18d9cf1ca515aa41ff6fd68
Author: Shawn M Moore <sartak at bestpractical.com>
Date: Tue Mar 29 15:42:43 2011 -0400
Use only the integer number of seconds in the Refresh header
Otherwise attackers could inject additional content into the header to
convince the browser to, say, redirect to a different site
diff --git a/share/html/Elements/Header b/share/html/Elements/Header
index ac9683d..5b7abe1 100755
--- a/share/html/Elements/Header
+++ b/share/html/Elements/Header
@@ -54,7 +54,7 @@
% if ($Refresh && $Refresh =~ /^(\d+)/ && $1 > 0) {
- <meta http-equiv="refresh" content="<% $Refresh %>" />
+ <meta http-equiv="refresh" content="<% $1 %>" />
% }
<link rel="shortcut icon" href="<%RT->Config->Get('WebImagesURL')%>/favicon.png" type="image/png" />
commit dc5c15d849c1ef06cd7fd9341147e0b2abae222f
Author: Thomas Sibley <trs at bestpractical.com>
Date: Mon Mar 28 19:27:48 2011 -0400
Test that values for IS and IS NOT are forced to NULL
This is a SQL injection vulnerability since DBIx::SearchBuilder doesn't
quote values when the IS and IS NOT operators are used. Our TicketSQL
parsing doesn't enforce NULL either, so arbitrary strings make it into
the generated SQL. At best this results in broken queries; at worst,
attacks allowing complex data extraction. The use of DBI's prepared
statements disallows an attacker from running more than one SQL
statement, thus precluding destructive queries such as "DELETE FROM" or
"DROP TABLE".
Right now the added tests fail, but a fix is forthcoming.
diff --git a/t/api/tickets_overlay_sql.t b/t/api/tickets_overlay_sql.t
index 7980886..ab4dc66 100644
--- a/t/api/tickets_overlay_sql.t
+++ b/t/api/tickets_overlay_sql.t
@@ -1,6 +1,6 @@
use RT;
-use RT::Test tests => 7;
+use RT::Test tests => 15;
{
@@ -66,6 +66,20 @@ my $string = 'subject/content SQL test';
is ($count, scalar @created, "number of returned tickets same as entered");
}
+diag "Make sure we don't barf on invalid input for IS / IS NOT";
+{
+ my ($status, $msg) = $tix->FromSQL("Subject IS 'foobar'");
+ ok ($status, "valid query") or diag("error: $msg");
+ is $tix->Count, 0, "found no tickets";
+ unlike $tix->BuildSelectQuery, qr/foobar/, "didn't find foobar in the select";
+ like $tix->BuildSelectQuery, qr/Subject IS NULL/, "found right clause";
+
+ my ($status, $msg) = $tix->FromSQL("Subject IS NOT 'foobar'");
+ ok ($status, "valid query") or diag("error: $msg");
+ is $tix->Count, 2, "found two tickets";
+ unlike $tix->BuildSelectQuery, qr/foobar/, "didn't find foobar in the select";
+ like $tix->BuildSelectQuery, qr/Subject IS NOT NULL/, "found right clause";
+}
}
commit a9ea4c1be2206e95605e784d12f3ebed3abe0cbf
Author: Thomas Sibley <trs at bestpractical.com>
Date: Mon Mar 28 19:57:51 2011 -0400
Override Limit further to force values to NULL for IS and IS NOT
Values for IS and IS NOT aren't quoted by DBIx::SearchBuilder's Limit.
This leads to a SQL injection vulnerability when user provided values
are passed into Limit, such as through TicketSQL. Forcing to NULL
closes this vulnerability and prevents accidental invalid SQL across RT.
diff --git a/lib/RT/SearchBuilder.pm b/lib/RT/SearchBuilder.pm
index be64a2c..dceba25 100755
--- a/lib/RT/SearchBuilder.pm
+++ b/lib/RT/SearchBuilder.pm
@@ -274,14 +274,27 @@ This Limit sub calls SUPER::Limit, but defaults "CASESENSITIVE" to 1, thus
making sure that by default lots of things don't do extra work trying to
match lower(colname) agaist lc($val);
+We also force VALUE to C<NULL> when the OPERATOR is C<IS> or C<IS NOT>.
+This ensures that we don't pass invalid SQL to the database or allow SQL
+injection attacks when we pass through user specified values.
+
=cut
sub Limit {
my $self = shift;
- my %args = ( CASESENSITIVE => 1,
- @_ );
+ my %args = (
+ CASESENSITIVE => 1,
+ @_
+ );
+
+ # We use the same regex here that DBIx::SearchBuilder uses to exclude
+ # values from quoting
+ if ( ($args{'OPERATOR'} || '') =~ /IS/i ) {
+ # Don't pass anything but NULL for IS and IS NOT
+ $args{'VALUE'} = 'NULL';
+ }
- return $self->SUPER::Limit(%args);
+ $self->SUPER::Limit(%args);
}
=head2 ItemsOrderBy
commit 55679544e461d916af8a2b3c458375fc5782a607
Author: Thomas Sibley <trs at bestpractical.com>
Date: Mon Mar 28 20:04:20 2011 -0400
Test that our UI canonicalizes values to NULL for IS/IS NOT
RT::Interface::Web::QueryBuilder::Tree already does this. When we
refactor that crazy in RT::SQL, however, we need to ensure we don't
break the canonicalization in the UI.
diff --git a/t/web/query_builder.t b/t/web/query_builder.t
index fa2c56d..25d6ec5 100644
--- a/t/web/query_builder.t
+++ b/t/web/query_builder.t
@@ -5,7 +5,7 @@ use HTTP::Request::Common;
use HTTP::Cookies;
use LWP;
use Encode;
-use RT::Test tests => 44;
+use RT::Test tests => 50;
my $cookie_jar = HTTP::Cookies->new;
my ($baseurl, $agent) = RT::Test->started_ok;
@@ -256,3 +256,30 @@ diag "send query with not quoted negative number";
"query is the same"
);
}
+
+diag "click advanced, enter an invalid SQL IS restriction, apply and check that we corrected it";
+{
+ my $response = $agent->get($url."Search/Edit.html");
+ ok( $response->is_success, "Fetched /Search/Edit.html" );
+ ok($agent->form_number(3), "found the form");
+ $agent->field("Query", "Requestor.EmailAddress IS 'FOOBAR'");
+ $agent->submit;
+ is( getQueryFromForm($agent),
+ "Requestor.EmailAddress IS NULL",
+ "foobar is replaced by NULL"
+ );
+}
+
+diag "click advanced, enter an invalid SQL IS NOT restriction, apply and check that we corrected it";
+{
+ my $response = $agent->get($url."Search/Edit.html");
+ ok( $response->is_success, "Fetched /Search/Edit.html" );
+ ok($agent->form_number(3), "found the form");
+ $agent->field("Query", "Requestor.EmailAddress IS NOT 'FOOBAR'");
+ $agent->submit;
+ is( getQueryFromForm($agent),
+ "Requestor.EmailAddress IS NOT NULL",
+ "foobar is replaced by NULL"
+ );
+}
+
commit 528ad20ced2a457c2989590b1e223bfe5e60e38a
Author: Thomas Sibley <trs at bestpractical.com>
Date: Wed Mar 30 11:50:28 2011 -0400
A failing test that searches by invalid watcher subfields in TicketSQL
There's also a basic test to ensure valid subfields are allowed.
diff --git a/t/api/tickets_overlay_sql.t b/t/api/tickets_overlay_sql.t
index 7980886..71da432 100644
--- a/t/api/tickets_overlay_sql.t
+++ b/t/api/tickets_overlay_sql.t
@@ -1,6 +1,6 @@
use RT;
-use RT::Test tests => 7;
+use RT::Test tests => 11;
{
@@ -33,6 +33,7 @@ my $string = 'subject/content SQL test';
my $t = RT::Ticket->new($RT::SystemUser);
ok( $t->Create( Queue => 'General',
+ Requestor => 'jesse at example.com',
Subject => 'another ticket',
MIMEObj => $Message,
MemberOf => $created[0]
@@ -66,6 +67,16 @@ my $string = 'subject/content SQL test';
is ($count, scalar @created, "number of returned tickets same as entered");
}
+{
+ my ($status, $msg) = $tix->FromSQL("Requestor.Signature LIKE 'foo'");
+ ok (!$status, "invalid query - Signature not valid") or diag("error: $msg");
+
+ my ($status, $msg) = $tix->FromSQL("Requestor.EmailAddress LIKE 'jesse'");
+ ok ($status, "valid query") or diag("error: $msg");
+ is $tix->Count, 1, "found one ticket";
+ like $tix->First->Subject, qr/another ticket/, "found the right ticket";
+}
+
}
commit 7824737c6e3a43f79f374f3f310c77ba36421f0a
Author: Thomas Sibley <trs at bestpractical.com>
Date: Wed Mar 30 12:10:35 2011 -0400
Limit watcher subfields to a valid subset
Previously you could search on user fields which shouldn't be
accessible, such as Comments and Password. Unfortunately we can't build
the valid list of fields from RT::User's accessible hashes since search
historically lets you limit on fields which aren't publically
accessible.
This also closes a SQL injection vulnerability which allowed you access
to the WHERE clause, potentially returning data not normally available.
The vulnerability exploited that everything after the first dot (.) in a
watcher TicketSQL clause was passed in to Limit as FIELD.
Like other TicketSQL methods, we die to throw an error on invalid
subfields.
diff --git a/lib/RT/Tickets_Overlay.pm b/lib/RT/Tickets_Overlay.pm
index e4020f6..9674feb 100755
--- a/lib/RT/Tickets_Overlay.pm
+++ b/lib/RT/Tickets_Overlay.pm
@@ -147,6 +147,13 @@ our %FIELD_METADATA = (
HasNoAttribute => [ 'HASATTRIBUTE', 0 ],
);
+our %SEARCHABLE_SUBFIELDS = (
+ User => [qw(
+ EmailAddress Name RealName Nickname Organization Address1 Address2
+ WorkPhone HomePhone MobilePhone PagerPhone id
+ )],
+);
+
# Mapping of Field Type to Function
our %dispatch = (
ENUM => \&_EnumLimit,
@@ -804,6 +811,13 @@ sub _WatcherLimit {
my $type = $meta->[1] || '';
my $class = $meta->[2] || 'Ticket';
+ # Bail if the subfield is not allowed
+ if ( $rest{SUBKEY}
+ and not grep { $_ eq $rest{SUBKEY} } @{$SEARCHABLE_SUBFIELDS{'User'}})
+ {
+ die "Invalid watcher subfield: '$rest{SUBKEY}'";
+ }
+
# Owner was ENUM field, so "Owner = 'xxx'" allowed user to
# search by id and Name at the same time, this is workaround
# to preserve backward compatibility
diff --git a/share/html/Search/Elements/SelectPersonType b/share/html/Search/Elements/SelectPersonType
index 2d20973..6454acf 100644
--- a/share/html/Search/Elements/SelectPersonType
+++ b/share/html/Search/Elements/SelectPersonType
@@ -72,7 +72,7 @@ else {
@types = qw(Requestor Cc AdminCc Watcher Owner QueueCc QueueAdminCc QueueWatcher);
}
-my @subtypes = qw(EmailAddress Name RealName Nickname Organization Address1 Address2 WorkPhone HomePhone MobilePhone PagerPhone id);
+my @subtypes = @{ $RT::Tickets::SEARCHABLE_SUBFIELDS{'User'} };
</%INIT>
<%ARGS>
commit 06e708c6954e5a808310f2d9adeeddcf94133060
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Fri Apr 8 11:14:27 2011 -0400
Allow the logout page to specify a URL to redirect to
We pass in $m->notes because %ARGS is passed in wholesale in too many
places, and there is no way to validate the url obtained via %ARGS, due
to the callback.
diff --git a/share/html/Elements/Header b/share/html/Elements/Header
index 5b7abe1..f9bd27f 100755
--- a/share/html/Elements/Header
+++ b/share/html/Elements/Header
@@ -54,7 +54,8 @@
% if ($Refresh && $Refresh =~ /^(\d+)/ && $1 > 0) {
- <meta http-equiv="refresh" content="<% $1 %>" />
+% my $URL = $m->notes->{LogoutURL}; $URL = $URL ? ";URL=$URL" : "";
+ <meta http-equiv="refresh" content="<% "$1$URL" %>" />
% }
<link rel="shortcut icon" href="<%RT->Config->Get('WebImagesURL')%>/favicon.png" type="image/png" />
diff --git a/share/html/NoAuth/Logout.html b/share/html/NoAuth/Logout.html
index 103ae4f..fa21100 100755
--- a/share/html/NoAuth/Logout.html
+++ b/share/html/NoAuth/Logout.html
@@ -45,7 +45,7 @@
%# those contributions and any derivatives thereof.
%#
%# END BPS TAGGED BLOCK }}}
-<& /Elements/Header, Title => loc('Logout'), Refresh => RT->Config->Get('LogoutRefresh').";URL=$URL" &>
+<& /Elements/Header, Title => loc('Logout'), Refresh => RT->Config->Get('LogoutRefresh') &>
</div>
<div id="body" class="login-body">
@@ -81,4 +81,5 @@ if (keys %session) {
}
$m->callback( %ARGS, CallbackName => 'AfterSessionDelete' );
+$m->notes->{LogoutURL} = $URL;
</%INIT>
commit c2291f59683484d54e0a342ca920ca305040bce5
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Tue Apr 12 12:24:55 2011 -0400
Update the two reports which used the short form of User in charting
diff --git a/share/html/Tools/Reports/ResolvedByDates.html b/share/html/Tools/Reports/ResolvedByDates.html
index 4f833ac..c23e238 100644
--- a/share/html/Tools/Reports/ResolvedByDates.html
+++ b/share/html/Tools/Reports/ResolvedByDates.html
@@ -79,7 +79,7 @@ $q->LoadByCols(Name => $Queue);
% if ($Queue) { $query .= " AND Queue = '$Queue'"}
% if ($ResolvedBefore) { $query .= " AND Resolved < '".$before->ISO(Timezone => 'user')."'"; }
% if ($ResolvedAfter) { $query .= " AND Resolved > '".$after->ISO(Timezone => 'user')."'"}
-% my $groupby = 'Owner';
+% my $groupby = 'Owner.Name';
<& /Search/Elements/Chart, Query => $query, PrimaryGroupBy => $groupby &>
% }
diff --git a/share/html/Tools/Reports/ResolvedByOwner.html b/share/html/Tools/Reports/ResolvedByOwner.html
index 6638388..d41f503 100644
--- a/share/html/Tools/Reports/ResolvedByOwner.html
+++ b/share/html/Tools/Reports/ResolvedByOwner.html
@@ -59,7 +59,7 @@ $q->LoadByCols(Name => $Queue);
% if ($Queue) {
% # if we have a queue, do the search
% my $query = "Status = 'resolved' AND Queue = '$Queue'";
-% my $groupby = 'Owner';
+% my $groupby = 'Owner.Name';
<& /Search/Elements/Chart, Query => $query, PrimaryGroupBy => $groupby &>
% }
commit 9c30ac738ea5cce86176d8036ee40a0aa74a4209
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Tue Apr 12 16:10:27 2011 -0400
Use Apache->the_request for mod_perl1 compat, instead of ->unparsed_uri
diff --git a/bin/webmux.pl.in b/bin/webmux.pl.in
index 06321c0..7aae041 100755
--- a/bin/webmux.pl.in
+++ b/bin/webmux.pl.in
@@ -96,7 +96,8 @@ sub handler {
# none of the methods in $r gives us the information we want (most
# canonicalize /foo/../bar to /bar which is exactly what we want to avoid)
- my $uri = URI->new("http://".$r->hostname.$r->unparsed_uri);
+ my (undef, $requested) = split ' ', $r->the_request, 3;
+ my $uri = URI->new("http://".$r->hostname.$requested);
my $path = URI::Escape::uri_unescape($uri->path);
## Each environment has its own way of handling .. and so on in paths,
commit 4692fcbdcbb219ab72cd146bef140f445ad17367
Author: Shawn M Moore <sartak at bestpractical.com>
Date: Wed Apr 6 11:27:36 2011 -0400
Avoid testing files out of RichText
diff --git a/t/web/compilation_errors.t b/t/web/compilation_errors.t
index 4fd9c40..36a0068 100644
--- a/t/web/compilation_errors.t
+++ b/t/web/compilation_errors.t
@@ -5,7 +5,7 @@ use Test::More;
use File::Find;
BEGIN {
sub wanted {
- -f && /\.html$/ && $_ !~ /Logout.html$/;
+ -f && /\.html$/ && $_ !~ /Logout.html$/ && $File::Find::dir !~ /RichText/;
}
my $tests = 4;
find( sub { wanted() and $tests += 4 }, 'share/html/' );
commit f43d0452fb16dd444f36de1f56b619a78124f662
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed Apr 13 11:53:32 2011 -0400
We do not link the results in the table in this version
diff --git a/t/web/charting.t b/t/web/charting.t
index 5618762..7c11f9c 100644
--- a/t/web/charting.t
+++ b/t/web/charting.t
@@ -26,8 +26,8 @@ ok( $m->login, "Logged in" );
# Test that defaults work
$m->get_ok( "/Search/Chart.html?Query=id>0" );
-$m->content_like(qr{<th[^>]*>Queue\s*</th>\s*<th[^>]*>Tickets\s*</th>}, "Grouped by queue");
-$m->content_like(qr{General</a>\s*</td>\s*<td[^>]*>\s*<a[^>]*>7</a>}, "Found results in table");
+$m->content_like(qr{<th[^>]*>\s*Queue\s*</th>\s*<th[^>]*>\s*Tickets\s*</th>}, "Grouped by queue");
+$m->content_like(qr{General</a>\s*</td>\s*<td[^>]*>\s*7}, "Found results in table");
$m->content_like(qr{<img src="/Search/Chart\?}, "Found image");
$m->get_ok( "/Search/Chart?Query=id>0" );
@@ -37,8 +37,8 @@ ok( length($m->content), "Has content" );
# Group by Queue
$m->get_ok( "/Search/Chart.html?Query=id>0&PrimaryGroupBy=Queue" );
-$m->content_like(qr{<th[^>]*>Queue\s*</th>\s*<th[^>]*>Tickets\s*</th>}, "Grouped by queue");
-$m->content_like(qr{General</a>\s*</td>\s*<td[^>]*>\s*<a[^>]*>7</a>}, "Found results in table");
+$m->content_like(qr{<th[^>]*>\s*Queue\s*</th>\s*<th[^>]*>\s*Tickets\s*</th>}, "Grouped by queue");
+$m->content_like(qr{General</a>\s*</td>\s*<td[^>]*>\s*7}, "Found results in table");
$m->content_like(qr{<img src="/Search/Chart\?}, "Found image");
$m->get_ok( "/Search/Chart?Query=id>0&PrimaryGroupBy=Queue" );
@@ -48,9 +48,9 @@ ok( length($m->content), "Has content" );
# Group by Requestor email
$m->get_ok( "/Search/Chart.html?Query=id>0&PrimaryGroupBy=Requestor.EmailAddress" );
-$m->content_like(qr{<th[^>]*>Requestor\.EmailAddress\s*</th>\s*<th[^>]*>Tickets\s*</th>},
+$m->content_like(qr{<th[^>]*>\s*Requestor\.EmailAddress\s*</th>\s*<th[^>]*>\s*Tickets\s*</th>},
"Grouped by requestor");
-$m->content_like(qr{root0\@localhost</a>\s*</td>\s*<td[^>]*>\s*<a[^>]*>3</a>}, "Found results in table");
+$m->content_like(qr{root0\@localhost</a>\s*</td>\s*<td[^>]*>\s*3}, "Found results in table");
$m->content_like(qr{<img src="/Search/Chart\?}, "Found image");
$m->get_ok( "/Search/Chart?Query=id>0&PrimaryGroupBy=Requestor.Email" );
@@ -60,7 +60,7 @@ ok( length($m->content), "Has content" );
# Group by Requestor phone -- which is bogus, and falls back to queue
$m->get_ok( "/Search/Chart.html?Query=id>0&PrimaryGroupBy=Requestor.Phone" );
-$m->content_like(qr{General</a>\s*</td>\s*<td[^>]*>\s*<a[^>]*>7</a>},
+$m->content_like(qr{General</a>\s*</td>\s*<td[^>]*>\s*7},
"Found queue results in table, as a default");
$m->content_like(qr{<img src="/Search/Chart\?}, "Found image");
commit f6acc6cf043408116a622acd315b59563c7631d6
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Wed Apr 13 14:44:30 2011 -0400
Tests - now with more passing
diff --git a/t/web/private-components.t b/t/web/private-components.t
index ceb2b34..30e145f 100644
--- a/t/web/private-components.t
+++ b/t/web/private-components.t
@@ -1,6 +1,6 @@
use strict;
-use RT::Test tests => 24;
+use RT::Test tests => 20;
my ($baseurl, $agent) = RT::Test->started_ok;
ok $agent->login, 'logged in';
@@ -18,10 +18,6 @@ $agent->get("/Widgets/TitleBox?title=private");
is($agent->status, 403);
$agent->content_lacks("private");
-$agent->get("/m/_elements/header?title=private");
-is($agent->status, 403);
-$agent->content_lacks("private");
-
$agent->get("/autohandler");
is($agent->status, 403);
$agent->content_lacks("comp called without component");
commit 18f490f5206f3189131c1f1aa8e1c5c1bf613660
Merge: bd0ee25 78830a5
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Wed Apr 13 20:17:32 2011 -0400
Merge branch 'security/3.8/customfield-column-injection' into 3.8.10-releng
Fixes CVE-2011-1686 CVE-2011-1687
commit dc839582da7f3f2789bc375f02f01e618275c1c8
Merge: 18f490f e43c99d
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Wed Apr 13 20:18:20 2011 -0400
Merge branch 'security/3.8/external-cf-eval' into 3.8.10-releng
Fixes CVE-2011-1685
commit 3e0d834c14647b7cc4161fecc66112e6c1be5322
Merge: dc83958 5567954
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Wed Apr 13 20:18:48 2011 -0400
Merge branch 'security/3.8/force-null' into 3.8.10-releng
Fixes CVE-2011-1686 CVE-2011-1687
commit 01cea1c7bb1f6fdc4e11d3a8d851ef3bd23229f6
Merge: 3e0d834 236c3de
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Wed Apr 13 20:19:29 2011 -0400
Merge branch 'security/3.8/limit-security-restriction' into 3.8.10-releng
Fixes CVE-2011-1686
Conflicts:
lib/RT/SearchBuilder.pm
diff --cc lib/RT/SearchBuilder.pm
index dceba25,e38047e..e58a00c
--- a/lib/RT/SearchBuilder.pm
+++ b/lib/RT/SearchBuilder.pm
@@@ -282,19 -278,32 +282,39 @@@ injection attacks when we pass through
sub Limit {
my $self = shift;
- my %args = (
+ my %ARGS = (
CASESENSITIVE => 1,
- @_
+ OPERATOR => '=',
+ @_,
);
+ # We use the same regex here that DBIx::SearchBuilder uses to exclude
+ # values from quoting
- if ( ($args{'OPERATOR'} || '') =~ /IS/i ) {
++ if ( $ARGS{'OPERATOR'} =~ /IS/i ) {
+ # Don't pass anything but NULL for IS and IS NOT
- $args{'VALUE'} = 'NULL';
++ $ARGS{'VALUE'} = 'NULL';
+ }
+
- $self->SUPER::Limit(%args);
+ if ($ARGS{FUNCTION}) {
+ ($ARGS{ALIAS}, $ARGS{FIELD}) = split /\./, delete $ARGS{FUNCTION}, 2;
+ $self->SUPER::Limit(%ARGS);
+ } elsif ($ARGS{FIELD} =~ /\W/
+ or $ARGS{OPERATOR} !~ /^(=|<|>|!=|<>|<=|>=
+ |(NOT\s*)?LIKE
+ |(NOT\s*)?(STARTS|ENDS)WITH
+ |(NOT\s*)?MATCHES
+ |IS(\s*NOT)?
+ |IN)$/ix) {
+ $RT::Logger->crit("Possible SQL injection attack: $ARGS{FIELD} $ARGS{OPERATOR}");
+ $self->SUPER::Limit(
+ %ARGS,
+ FIELD => 'id',
+ OPERATOR => '<',
+ VALUE => '0',
+ );
+ } else {
+ $self->SUPER::Limit(%ARGS);
+ }
}
=head2 ItemsOrderBy
commit ef5c2c3c1eb8ef77e232f99cb5d9022af778a67c
Merge: 01cea1c a19c0ed
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Wed Apr 13 20:23:40 2011 -0400
Merge branch 'security/3.8/orderby-injection' into 3.8.10-releng
Fixes CVE-2011-1686 CVE-2011-1687
commit 20b23eaa127744e5d99897724d6ae9bdac835c31
Merge: ef5c2c3 9c30ac7
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Wed Apr 13 20:24:41 2011 -0400
Merge branch 'security/3.8/path-traversal' into 3.8.10-releng
Fixes CVE-2011-1688
commit 34d6eaac2eb3df17296c0a9a6c303d82d61f6e7f
Merge: 20b23ea f6acc6c
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Wed Apr 13 20:25:14 2011 -0400
Merge branch 'security/3.8/private-components' into 3.8.10-releng
Fixes CVE-2011-1689
commit c59d8f3dcb1c3c436de8981fddbb53f102f2e55e
Merge: 34d6eaa f43d045
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Wed Apr 13 20:25:30 2011 -0400
Merge branch 'security/3.8/restrict-charting' into 3.8.10-releng
Fixes CVE-2011-1686 CVE-2011-1687
commit a251bce4a9760789c15db290b70c5d5de87912c1
Merge: c59d8f3 4692fcb
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Wed Apr 13 20:26:06 2011 -0400
Merge branch 'security/3.8/richtext-autohandler' into 3.8.10-releng
commit 992045b7fb51635c734a739d1f7cabe7c4f4c614
Merge: a251bce 7824737
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Wed Apr 13 20:26:25 2011 -0400
Merge branch 'security/3.8/ticketsql-private-fields' into 3.8.10-releng
Fixes CVE-2011-1686 CVE-2011-1687
Conflicts:
t/api/tickets_overlay_sql.t
diff --cc t/api/tickets_overlay_sql.t
index ab4dc66,71da432..5bc6140
--- a/t/api/tickets_overlay_sql.t
+++ b/t/api/tickets_overlay_sql.t
@@@ -1,7 -1,7 +1,6 @@@
use RT;
- use RT::Test tests => 15;
-
-use RT::Test tests => 11;
-
++use RT::Test tests => 19;
{
@@@ -66,22 -67,18 +66,31 @@@ my $string = 'subject/content SQL test'
is ($count, scalar @created, "number of returned tickets same as entered");
}
+diag "Make sure we don't barf on invalid input for IS / IS NOT";
+{
+ my ($status, $msg) = $tix->FromSQL("Subject IS 'foobar'");
+ ok ($status, "valid query") or diag("error: $msg");
+ is $tix->Count, 0, "found no tickets";
+ unlike $tix->BuildSelectQuery, qr/foobar/, "didn't find foobar in the select";
+ like $tix->BuildSelectQuery, qr/Subject IS NULL/, "found right clause";
+
+ my ($status, $msg) = $tix->FromSQL("Subject IS NOT 'foobar'");
+ ok ($status, "valid query") or diag("error: $msg");
+ is $tix->Count, 2, "found two tickets";
+ unlike $tix->BuildSelectQuery, qr/foobar/, "didn't find foobar in the select";
+ like $tix->BuildSelectQuery, qr/Subject IS NOT NULL/, "found right clause";
+}
+
+ {
+ my ($status, $msg) = $tix->FromSQL("Requestor.Signature LIKE 'foo'");
+ ok (!$status, "invalid query - Signature not valid") or diag("error: $msg");
+
+ my ($status, $msg) = $tix->FromSQL("Requestor.EmailAddress LIKE 'jesse'");
+ ok ($status, "valid query") or diag("error: $msg");
+ is $tix->Count, 1, "found one ticket";
+ like $tix->First->Subject, qr/another ticket/, "found the right ticket";
+ }
-
-
}
1;
commit 572c5725e4c760a69a9848bacd598ca351a2f205
Merge: 992045b 06e708c
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Wed Apr 13 20:26:42 2011 -0400
Merge branch 'security/3.8/validate-refresh' into 3.8.10-releng
Fixes CVE-2011-1689
commit d2055ebe2f27a38ea34dcd269978851e1a5d4ddd
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Wed Apr 13 20:32:21 2011 -0400
Bump version for 3.8.10
diff --git a/configure.ac b/configure.ac
index e6a0e20..0affb7d 100755
--- a/configure.ac
+++ b/configure.ac
@@ -7,7 +7,7 @@ AC_REVISION($Revision$)dnl
dnl Setup autoconf
AC_PREREQ([2.53])
-AC_INIT(RT, 3.8.10rc1, [rt-bugs at bestpractical.com])
+AC_INIT(RT, 3.8.10, [rt-bugs at bestpractical.com])
AC_CONFIG_SRCDIR([lib/RT.pm.in])
dnl Extract RT version number components
-----------------------------------------------------------------------
More information about the Rt-commit
mailing list