Comment by yummyfajitas | Hacker News Reader

Comment by yummyfajitas | Hacker News Reader

Parent Full thread

You are absolutely correct. But by properly locking things down, you can reduce the probability of giving an attacker write privileges.

If you simply split your connections into readonly and read/write pools, you've already drastically reduced your attack surface. (A third DDL role exists for migrations only, and does not exist in your app.) Now SQL injection on your homepage or "display content" page cannot give the attacker INSERT/UPDATE/DELETE.

You can go a little further and split your tables/columns into public ("I could reconstruct these by scraping the site") and private ("I might need to login for these"). Then create a role with access to the public data only, and use it for public views. Now an attack on your homepage/other public views can give the attacker only a limited SELECT.

This is all pretty easy to do and it can drastically reduce your attack surface. It also gives you a useful way to prioritize fixing vulnerabilities. First lock down read/write views. Then lock down read. Lastly lock down readpublic.

When I'm completely in control (writing a totally custom app) I do all my DB access with stored procedures. The application connection cannot see or access any tables directly. It can execute stored procedures, and that's it. I essentially build an API with stored procedures, and that's what the applications use.

Stored procedures don't offer any protection against malicious input. An SQL statement built up of concatenation including user input is just as bad in a stored procedure as it is directly in a program.

Yes, that is why yo never do that, regardless if its in a SP or in the calling code.

Stored procedures can do some more input validation, tough, that can be useful. You can protect (from the integrity standpoint) some of the content with traditional tools like foreign keys, but procedures are more flexible.

> Stored procedures don't offer any protection against malicious input.

Not necessarily true. Parameterized queries generally do and most stored procedure queries should be parameterized. Add good check constraints....

Now, the protection isn't perfect but it is well beyond no protection at all and can be a good level.

> An SQL statement built up of concatenation including user input is just as bad in a stored procedure as it is directly in a program.

Yeah, and sometimes that's the only way to do things in PostgreSQL. So if you find yourself having to do this (I find it most common in CREATE statements), then heavily commend and make sure (via frequent audit) that all values from the input are properly quoted by quote_ident and quote_literal functions.

> Not necessarily true. Parameterized queries generally do and most stored procedure queries should be parameterized.

But that's not anything special about stored procs. Queries called from application code should generally be parameterized, which provides the same protection.

It is different because in SQL you can't do non-parameterized queries, and in PLPGSQL you have to specify you are executing dynamic sql. This means that since it is in the db, it is going to be parameterized by default.

So what is different is the fact that unless you use a non-SQL-derivative stored proc language, you pretty much have to put up big warning signs any time you put in place a possibly exploitable query.

Examples:

1. This is not possibly exploitable:

    CREATE OR REPLACE FUNCTION foo(in_bar int) RETURNS setof foo
    LANGUAGE PLPGSQL AS
    $$
    BEGIN
        RETURN QUERY
            SELECT * FROM foo WHERE bar_id = in_bar;
    END;
    $$;

2. This one is exploitable.

    CREATE OR REPLACE FUNCTION new_user(username text) 
    RETURNS BOOL LANGUAGE PLPGSQL AS
    $$
    BEGIN
       EXECUTE $E$CREATE USER $E$ || username;
       RETURN TRUE;
    END;
    $$;

3. This one is not exploitable.

    CREATE OR REPLACE FUNCTION new_user(username text) 
    RETURNS BOOL LANGUAGE PLPGSQL AS
    $$
    BEGIN
       EXECUTE $E$CREATE USER $E$ || quote_ident(username);
       RETURN TRUE;
    END;
    $$;

The point is that it is crystal clear in these cases whether a query is parameterized internally or not. If you don't see the combination of EXECUTE and ||, there is nothing to worry about.

I don't do this because maintaining stored procs is a nightmare. All of a sudden my migrations folder is a mirror of my git history, except that "merge" takes on a nightmarish new meaning.

Nowadays I generally write raw SQL with the Tiramisu library (shameless plug: https://github.com/stucchio/Tiramisu ) I cooked up. It's basically writing straight SQL, but with the ability to safely do (almost) string concatenation to build queries.

Why would you need to store the historical versions of the stored procedures in a migrations folder? Wouldn't it be better to treat the stored procs like any other code, create a .sql file for each and keep them in version control? And then you can have a migration script that re-creates all the procs when you do an update.

Generally your migrations folder is supposed to store a complete history of your DB. Maybe doing it the way you suggest is a better way, it's just a bit outside the standard and to be honest I haven't thought carefully about the best way to structure code this way.

Alex at The Daily WTF wrote a great article on this subject.

http://thedailywtf.com/Articles/Database-Changes-Done-Right....

That's a decent read, but unfortunately even running the same precisely-versioned and ordered change scripts against dev, test, prod dbs can produce divergent results depending on the data in the db at the time. For example, prod could have a null somewhere, or data using a different url encoding flavor.... etc.

(Or if you're really lucky, even the current load on the database could make it fail... i.e. transaction serialization failure due to the random row order you attempted to lock)

So it should have ended with 'you're doomed'.

That's a tooling problem. Sqitch or something similar can help.

http://sqitch.org

Your stored procedures are "M" (Model) in MVC. It is a good abstraction, however I see no advantage (security-wise) in keeping this layer in DB stored procedures. It could just as well reside in some application layer.

In general, with LedgerSMB, we don't usually use stored procedures for security reasons. We use them for abstraction reasons. There are cases, however, where we do add security barriers to them so there are reasons why having this as a layer in the db can be a good thing.

Let's take an example: separation of duties in accounting. You want someone to enter transactions and you want someone else to approve them, and you want to enforce this through db permissions.

You might give insert permissions to user 1 but update permissions to user 2. Except that you don't want to allow updates. You only want to allow some updates, to a few columns.

So now you grant update permissions only on some columns. Except that you don't want to allow all updates on those columns. You only want to allow updating from, say, a null value to a definite value (say, indicating who and when).

You can't do this with standard permissions, so you now create another role with permission to update these columns, and require that mere mortals go through a stored procedure to update them, which will only allow those rows to be updated which had nulls in these values.

Now, I hear you say, you could do this with a join table called 'approval' with these columns so you don't have to worry about NULLs. Great, except that NOT EXISTS() queries tend to suck and you can't use partial indexes to ensure you can always quickly look up those transactions pending approval. So in the end NULLs, stored procedures, and an intermediate role end up winning out in terms of security.

Almost all damage done by SQLi is from reading. So removing write capability might reduce attack surface, it does nothing against the most effective method of total database exfiltration.

Read my next paragraph. A split of public and private data is only marginally more difficult, and is very helpful:

    CREATE TABLE public_user_content (
      body VARCHAR(512) NOT NULL,
      user_id BIGINT REFERENCES users(id)
    )

    CREATE TABLE users (
      id BIGINT PRIMARY KEY,
      username VARCHAR(128) NOT NULL,
      email VARCHAR(256) NOT NULL,
      badly_hashed_password VARCHAR(8),
      social_security_number VARCHAR(9)
    )

    GRANT SELECT ON public_user_content TO public_readers;
    GRANT SELECT ON users (username) TO public_readers;

Then in your view:

    @run_with_role('public_readers')
    def get_public_feed(request):
        ....

SQL injection in `get_public_feed` is no longer part of the attack surface for stealing PII.

This is a good idea.

However, in the context of forms authentication, there comes a delicate moment where the user name and hashed password must be exposed to an unauthenticated user, no?

Of course. And once one user logs in, if you have SQLi vulnerabilities in the `edit_user_pii` page, the attacker has read/write access to everything. Like I said, it's a way to reduce your attack surface, not a way to completely lock everything down.

Reply on news.ycombinator.com