incubator-empire-db-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Rainer Döbele <doeb...@esteam.de>
Subject re: Type checks in DBCommand in order to avoid SQL Injection
Date Thu, 28 Jan 2010 20:40:19 GMT
Hi McKinley,

to be honest: I don't think what you want makes sense and it's not what we're trying to achieve.
Empire-db should be lightweight plus easy and straightforward to use.
Adding various options adds complexity and is not intuitive.
And please understand that we cannot simply add features if someone has one very specific
problem.

The root of your problem is that you using a parameter of a web-request directly as the constraint
of a query.
This is not a good idea.
Anyone can forge the request and even if you have a check for the proper data type, this person
could access any record even if he had no credentials for that record. The check for validity
can only be performed by the application on a higher level at which you would in this case
need to convert the string to an integer (or something else) anyway. At least you should.

So let's try keeping Empire-db slim and simple.
Otherwise we end up as a hibernate clone :-)

Regards
Rainer

BTW: You should really have a look at our Web-examples. For forms they use DBRecord objects
which all kinds of validation checks (not just the data type). But I must admit apart from
that there is not enough security built in either. Security is a issue of its own.

McKinley wrote:
> Re: Type checks in DBCommand in order to avoid SQL Injection
> 
> Would the check go into that the addSQL() ->
> getObjectValue(expr.getDataType()... chain? In the case of my example,
> I have passed in a string so rather than give too much deference to
> the column data type, why not say "rare is the DBMS that will cast
> this correctly, but the developer seems to want [where intCol1 = '0;
> update this set that=\'bla\';']. Of course this requires adding a
> bunch of type checks on that Object value that was passed to determine
> that it is a string and should be quoted. I don't think I want any
> parseInt or regexp for ';' and '--'. Just quote wrapping when the
> value is character based.
> 
> If you do add checks how about changing addSql() to addSql(boolean
> safe)? addSql() would do any checks, but if you have white-listed all
> your inputs you can use addSql(false) to get extra speed.
> 
> I guess another option is that checks could be put into the
> DBCompareColExpr generators like is() and isBetween() so that they
> take specific types instead of Object, but that might be too much
> boilerplate code.
> 
> Thanks,
> 
> McKinley
> 
> 
> On Tue, Jan 26, 2010 at 11:37 PM, Rainer Döbele <doebele@esteam.de>
> wrote:
> > Hi McKinley
> >
> > Just to let you know:
> > In the Where clause of the DBCommand object we're not doing any type
> checks at all.
> > Hence your SQL-Injection example below also works for other numeric
> columns.
> >
> > The question is whether we should at this point or not.
> > We're doing type-checks when working with DBRecord objects.
> > They internally then use a DBCommand as well.
> > Adding a check in DBCommand would mean a double check.
> >
> > I think the DBCommand is the lowest level.
> > All checks should be performed at a higher level in order to avoid
> Overhead.
> >
> > Regards
> > Rainer

Mime
View raw message