Skip Menu |

This queue is for tickets about the SQL-Abstract CPAN distribution.

Report information
The Basics
Id: 117059
Status: open
Priority: 0/
Queue: SQL-Abstract

People
Owner: Nobody in particular
Requestors: lackas [...] invicro.com
Cc:
AdminCc:

Bug Information
Severity: Important
Broken in: 1.81
Fixed in: 1.81



Subject: SQL Injection in ORDER_BY
During audit of a web application that uses DBIx::Class we found that it is still vulnerable to SQL injection attacks on order_by, which is frequently used for users to sort tables. We assumed that DBIx::Class and ultimately SQL::Abstract would have taken care of dealing with the user input, however, order_by parameters are just passed through literally: use SQL::Abstract; use Data::Dumper::Concise; my $sql = SQL::Abstract->new; my ($stmt, @bind) = $sql->select("users", [ '*' ], { id => 1 }, ['1)']); print Dumper \$stmt, \@bind; $ perl sq.pl \"SELECT * FROM users WHERE ( id = ? ) ORDER BY 1)" [ 1 ] Note the invalid ORDER BY statement causing a syntax error. While these injections are not as easy to exploit as others, you can still use this to fetch whatever field you want from the database, or probe out the server, e.g. in our application you could access salted password hashes: SELECT username FROM users me WHERE id =1 ORDER BY 1,extractvalue(0x0a,concat(0x0a,(SELECT password FROM users LIMIT 1))); ERROR 1105 (HY000): XPATH syntax error: ' {SSHA}Ggq1OB1Y3U5g/67TGSACoYBSo' Is there a reason why bind values are not used here? I see that there is currently another ticket open (https://rt.cpan.org/Ticket/Display.html?id=103219), which seems slightly related and that this is an area that is currently being worked on, so I will not try to come up with my own patch. Since SQL::Abstract is likely used in many web applications, and the manual to neither DBIx::Class nor SQL::Abstract mentions that the application has to deal with order_by parameters themselves I would consider this to be an important issue. Please let me know should you require any further input.
Subject: Re: [rt.cpan.org #117059] SQL Injection in ORDER_BY
Date: Fri, 19 Aug 2016 11:24:41 +0200
To: bug-SQL-Abstract [...] rt.cpan.org
From: Peter Rabbitson <ribasushi [...] cpan.org>
On 08/19/2016 11:11 AM, Christian Lackas via RT wrote: Show quoted text
> Fri Aug 19 05:11:04 2016: Request 117059 was acted upon. > Transaction: Ticket created by DELTA > Queue: SQL-Abstract > Subject: SQL Injection in ORDER_BY > Broken in: 1.81 > Severity: Important > Owner: Nobody > Requestors: lackas@invicro.com > Status: new > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=117059 > > > > During audit of a web application that uses DBIx::Class we found that it is still vulnerable to SQL injection attacks on order_by
Without quoting enabled, SQLA/DBIC are vulnerable to a lot more than that. In DBIC the preferred way for you is to specify https://metacpan.org/pod/DBIx::Class::Storage::DBI#quote_names in your connection info. In Plain SQLA you have to do it yourself, such as: perl -MSQL::Abstract -MData::Dumper -e ' for my $quoted (0, 1) { warn Dumper [ SQL::Abstract->new( $quoted ? ( quote_char => q{"} ) : () ) ->select( "users", [ "*" ], { id => 1 }, ["1)"] ) ] } ' Show quoted text
> Is there a reason why bind values are not used here? > I see that there is currently another ticket open (https://rt.cpan.org/Ticket/Display.html?id=103219), which seems slightly related and that this is an area that is currently being worked on, so I will not try to come up with my own patch.
That particular patch is due to ship any day now, but it does not help you in any way. Bind values can be used for values only. Usually one orders by an identifier (column name and/or expression) which can not be stuffed in a placeholder. Quoting is your only option in this case. Changing the default in SQLA at this point is sadly impractical (besides - what would you change it to? different engines have different quoting rules). Bottom line - as it stands today the user needs to twiddle couple knobs to make things safer. It is as unfortunate as perl5's strictures/warnings being off by default :/ I know this doesn't address your immediate concern, but I am afraid this is it.