[Jifty-commit] jifty-dbi branch, master, updated. 0.67-5-g45df75f

Jifty commits jifty-commit at lists.jifty.org
Thu Apr 14 12:11:07 EDT 2011


The branch, master has been updated
       via  45df75f8f5e2caceaffc106d233b5c48a2442b70 (commit)
       via  afa084f1af18d57c8419fa343f5fac9733eb4998 (commit)
       via  9156cf1f0a46baac5b850873d03c1860b2ecc0f0 (commit)
       via  9020ef2c168fbdc899e27e201c9b581087ac7a4c (commit)
       via  c0bdb1fa51864f228f01d2e65ca7ebbf2f2dd901 (commit)
      from  15bb9599026fc528eb8266512f0c1de37fb7cf18 (commit)

Summary of changes:
 lib/Jifty/DBI/Collection.pm    |   93 ++++++++++++++++++++++++++++------------
 lib/Jifty/DBI/Handle/Oracle.pm |   22 +++++++--
 lib/Jifty/DBI/Handle/Pg.pm     |    7 ++-
 3 files changed, 87 insertions(+), 35 deletions(-)

- Log -----------------------------------------------------------------
commit c0bdb1fa51864f228f01d2e65ca7ebbf2f2dd901
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Thu Apr 7 11:51:56 2011 -0400

    There is no need to check $args{column} around our LIKE adjustments

diff --git a/lib/Jifty/DBI/Collection.pm b/lib/Jifty/DBI/Collection.pm
index d6a8f28..f2a777d 100755
--- a/lib/Jifty/DBI/Collection.pm
+++ b/lib/Jifty/DBI/Collection.pm
@@ -1308,27 +1308,24 @@ sub limit {
     #since we're changing the search criteria, we need to redo the search
     $self->redo_search();
 
-    if ( $args{'column'} ) {
-
-        #If it's a like, we supply the %s around the search term
-        if ( $args{'operator'} =~ /MATCHES/i ) {
-            $args{'value'} = "%" . $args{'value'} . "%";
-        } elsif ( $args{'operator'} =~ /STARTS_?WITH/i ) {
-            $args{'value'} = $args{'value'} . "%";
-        } elsif ( $args{'operator'} =~ /ENDS_?WITH/i ) {
-            $args{'value'} = "%" . $args{'value'};
-        }
-        $args{'operator'} =~ s/(?:MATCHES|ENDS_?WITH|STARTS_?WITH)/LIKE/i;
+    #If it's a like, we supply the %s around the search term
+    if ( $args{'operator'} =~ /MATCHES/i ) {
+        $args{'value'} = "%" . $args{'value'} . "%";
+    } elsif ( $args{'operator'} =~ /STARTS_?WITH/i ) {
+        $args{'value'} = $args{'value'} . "%";
+    } elsif ( $args{'operator'} =~ /ENDS_?WITH/i ) {
+        $args{'value'} = "%" . $args{'value'};
+    }
+    $args{'operator'} =~ s/(?:MATCHES|ENDS_?WITH|STARTS_?WITH)/LIKE/i;
 
-        #if we're explicitly told not to to quote the value or
-        # we're doing an IS or IS NOT (null), don't quote the operator.
+    #if we're explicitly told not to to quote the value or
+    # we're doing an IS or IS NOT (null), don't quote the operator.
 
-        if ( $args{'quote_value'} && $args{'operator'} !~ /IS/i ) {
-            if ( $value_ref eq 'ARRAY' ) {
-                map { $_ = $self->_handle->quote_value($_) } @{ $args{'value'} };
-            } else {
-                $args{'value'} = $self->_handle->quote_value( $args{'value'} );
-            }
+    if ( $args{'quote_value'} && $args{'operator'} !~ /IS/i ) {
+        if ( $value_ref eq 'ARRAY' ) {
+            map { $_ = $self->_handle->quote_value($_) } @{ $args{'value'} };
+        } else {
+            $args{'value'} = $self->_handle->quote_value( $args{'value'} );
         }
     }
 

commit 9020ef2c168fbdc899e27e201c9b581087ac7a4c
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Thu Apr 7 18:08:34 2011 -0400

    Prevent SQL injection in column names, operators, order and group by

diff --git a/lib/Jifty/DBI/Collection.pm b/lib/Jifty/DBI/Collection.pm
index f2a777d..9113c0a 100755
--- a/lib/Jifty/DBI/Collection.pm
+++ b/lib/Jifty/DBI/Collection.pm
@@ -1253,15 +1253,6 @@ sub limit {
 
     # }}}
 
-    # Set this to the name of the column and the alias, unless we've been
-    # handed a subclause name
-
-    my $qualified_column
-        = $args{'alias'}
-        ? $args{'alias'} . "." . $args{'column'}
-        : $args{'column'};
-    my $clause_id = $args{'subclause'} || $qualified_column;
-
     # $column_obj is undefined when the table2 argument to the join is a table
     # name and not a collection model class.  In that case, the class key
     # doesn't exist for the join.
@@ -1278,6 +1269,43 @@ sub limit {
         value_ref => \$args{'value'},
     ) if $column_obj && $column_obj->encode_on_select && $args{operator} !~ /IS/;
 
+    # Ensure that the column has nothing fishy going on.  We can't
+    # simply check $column_obj's truth because joins mostly join by
+    # table name, not class, and we don't track table_name -> class.
+    if ($args{column} =~ /\W/) {
+        warn "Possible SQL injection on column '$args{column}' in limit at @{[join(',',(caller)[1,2])]}\n";
+        %args = (
+            %args,
+            column   => 'id',
+            operator => '<',
+            value    => 0,
+        );
+    }
+    if ($args{operator} !~ /^(=|<|>|!=|<>|<=|>=
+                             |(NOT\s*)?LIKE
+                             |(NOT\s*)?(STARTS|ENDS)_?WITH
+                             |(NOT\s*)?MATCHES
+                             |IS(\s*NOT)?
+                             |IN)$/ix) {
+        warn "Unknown operator '$args{operator}' in limit at  @{[join(',',(caller)[1,2])]}\n";
+        %args = (
+            %args,
+            column   => 'id',
+            operator => '<',
+            value    => 0,
+        );
+    }
+
+
+    # Set this to the name of the column and the alias, unless we've been
+    # handed a subclause name
+    my $qualified_column
+        = $args{'alias'}
+        ? $args{'alias'} . "." . $args{'column'}
+        : $args{'column'};
+    my $clause_id = $args{'subclause'} || $qualified_column;
+
+
     # make passing in an object DTRT
     my $value_ref = ref( $args{value} );
     if ($value_ref) {
@@ -1664,6 +1692,10 @@ sub _order_clause {
         } elsif ( ( defined $rowhash{'alias'} )
             and ( $rowhash{'column'} ) )
         {
+            if ($rowhash{'column'} =~ /\W/) {
+                warn "Possible SQL injection in column '$rowhash{column}' in order_by\n";
+                next;
+            }
 
             $clause .= ( $clause ? ", " : " " );
             $clause .= $rowhash{'function'} . "(" if $rowhash{'function'};
@@ -1748,6 +1780,10 @@ sub _group_clause {
         } elsif ( ( $rowhash{'alias'} )
             and ( $rowhash{'column'} ) )
         {
+            if ($rowhash{'column'} =~ /\W/) {
+                warn "Possible SQL injection in column '$rowhash{column}' in group_by\n";
+                next;
+            }
 
             $clause .= ( $clause ? ", " : " " );
             $clause .= $rowhash{'alias'} . ".";

commit 9156cf1f0a46baac5b850873d03c1860b2ecc0f0
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Wed Apr 13 16:09:57 2011 -0400

    Slightly unify nigh-identical codepaths between Pg and Oracle

diff --git a/lib/Jifty/DBI/Handle/Oracle.pm b/lib/Jifty/DBI/Handle/Oracle.pm
index a2dc0ac..a9ad327 100755
--- a/lib/Jifty/DBI/Handle/Oracle.pm
+++ b/lib/Jifty/DBI/Handle/Oracle.pm
@@ -251,18 +251,26 @@ sub distinct_query {
             = [ @{ $collection->{group_by} || [] }, { column => 'id' } ];
         local $collection->{order_by} = [
             map {
-                      ( $_->{alias} and $_->{alias} ne "main" )
-                    ? { %{$_}, column => "min(" . $_->{column} . ")" }
-                    : $_
+                my $alias = $_->{alias} || '';
+                my $column = $_->{column};
+                $alias .= '.' if $alias;
+
+                ( ( !$alias or $alias eq 'main.' ) and $column eq 'id' )
+                    ? $_
+                    : { %{$_}, alias => '', column => "min($alias$column)" }
                 } @{ $collection->{order_by} }
         ];
         my $group = $collection->_group_clause;
         my $order = $collection->_order_clause;
         $$statementref
-            = "SELECT main.* FROM ( SELECT main.id FROM $$statementref $group $order ) distinctquery, $table main WHERE (main.id = distinctquery.id)";
+            = "SELECT "
+            . $collection->query_columns
+            . " FROM ( SELECT main.id FROM $$statementref $group $order ) distinctquery, $table main WHERE (main.id = distinctquery.id)";
     } else {
         $$statementref
-            = "SELECT main.* FROM ( SELECT DISTINCT main.id FROM $$statementref ) distinctquery, $table main WHERE (main.id = distinctquery.id) ";
+            = "SELECT "
+            . $collection->query_columns
+            . " FROM ( SELECT DISTINCT main.id FROM $$statementref ) distinctquery, $table main WHERE (main.id = distinctquery.id) ";
         $$statementref .= $collection->_group_clause;
         $$statementref .= $collection->_order_clause;
     }

commit afa084f1af18d57c8419fa343f5fac9733eb4998
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Wed Apr 13 16:11:05 2011 -0400

    Fix distinct_query to catch injection and correctly rewrite to function =>
    
    Pg and Oracle rewrite the order_by clause in distinct_query; in doing
    so, they not only would trigger the _order_by injection prevention.  The
    naive solution (simply passing 'function' instead of 'column') would
    also walk around the doing such prevention on the user-supplied data.

diff --git a/lib/Jifty/DBI/Handle/Oracle.pm b/lib/Jifty/DBI/Handle/Oracle.pm
index a9ad327..2abba56 100755
--- a/lib/Jifty/DBI/Handle/Oracle.pm
+++ b/lib/Jifty/DBI/Handle/Oracle.pm
@@ -253,11 +253,15 @@ sub distinct_query {
             map {
                 my $alias = $_->{alias} || '';
                 my $column = $_->{column};
+                if ($column =~ /\W/) {
+                    warn "Possible SQL injection in column '$column' in order_by\n";
+                    next;
+                }
                 $alias .= '.' if $alias;
 
                 ( ( !$alias or $alias eq 'main.' ) and $column eq 'id' )
                     ? $_
-                    : { %{$_}, alias => '', column => "min($alias$column)" }
+                    : { %{$_}, column => undef, function => "min($alias$column)" }
                 } @{ $collection->{order_by} }
         ];
         my $group = $collection->_group_clause;
diff --git a/lib/Jifty/DBI/Handle/Pg.pm b/lib/Jifty/DBI/Handle/Pg.pm
index 682bdd8..7b413eb 100755
--- a/lib/Jifty/DBI/Handle/Pg.pm
+++ b/lib/Jifty/DBI/Handle/Pg.pm
@@ -210,12 +210,15 @@ sub distinct_query {
             map {
                 my $alias = $_->{alias} || '';
                 my $column = $_->{column};
+                if ($column =~ /\W/) {
+                    warn "Possible SQL injection in column '$column' in order_by\n";
+                    next;
+                }
                 $alias .= '.' if $alias;
 
-                #warn "alias $alias => column $column\n";
                 ( ( !$alias or $alias eq 'main.' ) and $column eq 'id' )
                     ? $_
-                    : { %{$_}, alias => '', column => "min($alias$column)" }
+                    : { %{$_}, column => undef, function => "min($alias$column)" }
                 } @{ $collection->{order_by} }
         ];
         my $group = $collection->_group_clause;

commit 45df75f8f5e2caceaffc106d233b5c48a2442b70
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Mon Apr 11 14:07:18 2011 -0400

    Prevent SQL injection via IS
    
    Restrict the value on IS or IS NOT to be NULL

diff --git a/lib/Jifty/DBI/Collection.pm b/lib/Jifty/DBI/Collection.pm
index 9113c0a..3b9081e 100755
--- a/lib/Jifty/DBI/Collection.pm
+++ b/lib/Jifty/DBI/Collection.pm
@@ -1346,10 +1346,14 @@ sub limit {
     }
     $args{'operator'} =~ s/(?:MATCHES|ENDS_?WITH|STARTS_?WITH)/LIKE/i;
 
-    #if we're explicitly told not to to quote the value or
-    # we're doing an IS or IS NOT (null), don't quote the operator.
+    # Force the value to NULL (non-quoted) if the operator is IS.
+    if ($args{'operator'} =~ /^IS(\s*NOT)?$/i) {
+        $args{'quote_value'} = 0;
+        $args{'value'} = 'NULL';
+    }
 
-    if ( $args{'quote_value'} && $args{'operator'} !~ /IS/i ) {
+    # Quote the value
+    if ( $args{'quote_value'} ) {
         if ( $value_ref eq 'ARRAY' ) {
             map { $_ = $self->_handle->quote_value($_) } @{ $args{'value'} };
         } else {

-----------------------------------------------------------------------


More information about the Jifty-commit mailing list