New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support prepared statements to avoid SQL injection #1500

Closed
xenoterracide opened this Issue Dec 30, 2016 · 6 comments

Comments

Projects
None yet
4 participants
@xenoterracide

xenoterracide commented Dec 30, 2016

So I asked this, http://stackoverflow.com/q/40945277/206466, and never got an answer, big problem of course is that spring boot, mixed with flyway means env variables are easily used in these area's.

What version of Flyway are you using?

http://docs.spring.io/platform/docs/current/reference/htmlsingle/

Which client are you using? (Command-line, Java API, Maven plugin, Gradle plugin, SBT plugin, ANT tasks)

java api, though spring boot hides it.

What database are you using (type & version)?

PostgreSQL 9.6

What operating system are you using?

Windows/Linux/Docker

What did you do?

(Please include the content causing the issue, any relevant configuration settings, and the command you ran)

it appears if I write this code, for example with Spring Boot

CREATE ROLE application WITH LOGIN PASSWORD '${password}';
and then set

FLYWAY_PLACEHOLDERS_PASSWORD="' DROP table -- "
before starting the migration, flyway would execute the injection, as the quoting provided is in the string. Is there a way I can make this a prepared statement, or is there a quoting function I can use to ensure that the value is properly quoted?

What did you expect to see?

I expect this to be quoted properly, and not execute anything in the placeholder

What did you see instead?
Migration V0_1_0_0__initial_schema.sql failed
---------------------------------------------
SQL State  : 42601
Error Code : 0
Message    : ERROR: unrecognized role option "'"
  Position: 47
Location   : db/migration/V0_1_0_0__initial_schema.sql (/pipeline/source/target/classes/db/migration/V0_1_0_0__initial_schema.sql)
Line       : 2
Statement  : CREATE ROLE application WITH LOGIN PASSWORD ''"'"'"' DROP table -- "'';

GRANT SELECT, INSERT, UPDATE, DELETE ON ALL TABLES IN SCHEMA public TO application;

CREATE TABLE IF NOT EXISTS users (
    user_id UUID PRIMARY KEY,
    email   TEXT NOT NULL UNIQUE
);

CREATE TABLE IF NOT EXISTS password_credentials (
    password_credential_id UUID PRIMARY KEY,
    name                   TEXT NOT NULL UNIQUE,
    pass                   TEXT NOT NULL,
    user_id                UUID REFERENCES users ON UPDATE RESTRICT ON DELETE CASCADE
);

- INFO utoConfigurationReportLoggingInitializer : 

Error starting ApplicationContext. To display the auto-configuration report re-run your application with 'debug' enabled.
-ERROR o.s.boot.SpringApplication               : Application startup failed
-
org.springframework.beans.factory.BeanCreationException: Error creating bean with name 'flywayInitializer' defined in class path resource [org/springframework/boot/autoconfigure/flyway/FlywayAutoConfiguration$FlywayConfiguration.class]: Invocation of init method failed; nested exception is org.flywaydb.core.internal.dbsupport.FlywaySqlScriptException: 
Migration V0_1_0_0__initial_schema.sql failed
---------------------------------------------
SQL State  : 42601
Error Code : 0
Message    : ERROR: unrecognized role option "'"
  Position: 47
Location   : db/migration/V0_1_0_0__initial_schema.sql (/pipeline/source/target/classes/db/migration/V0_1_0_0__initial_schema.sql)
Line       : 2
Statement  : CREATE ROLE application WITH LOGIN PASSWORD ''"'"'"' DROP table -- "'';

GRANT SELECT, INSERT, UPDATE, DELETE ON ALL TABLES IN SCHEMA public TO application;

CREATE TABLE IF NOT EXISTS users (
    user_id UUID PRIMARY KEY,
    email   TEXT NOT NULL UNIQUE
);

CREATE TABLE IF NOT EXISTS password_credentials (
    password_credential_id UUID PRIMARY KEY,
    name                   TEXT NOT NULL UNIQUE,
    pass                   TEXT NOT NULL,
    user_id                UUID REFERENCES users ON UPDATE RESTRICT ON DELETE CASCADE
);

	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.initializeBean(AbstractAutowireCapableBeanFactory.java:1583) ~[spring-beans-4.3.4.RELEASE.jar:4.3.4.RELEASE]
	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.doCreateBean(AbstractAutowireCapableBeanFactory.java:553) ~[spring-beans-4.3.4.RELEASE.jar:4.3.4.RELEASE]
	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.createBean(AbstractAutowireCapableBeanFactory.java:482) ~[spring-beans-4.3.4.RELEASE.jar:4.3.4.RELEASE]
	at org.springframework.beans.factory.support.AbstractBeanFactory$1.getObject(AbstractBeanFactory.java:306) ~[spring-beans-4.3.4.RELEASE.jar:4.3.4.RELEASE]
	at org.springframework.beans.factory.support.DefaultSingletonBeanRegistry.getSingleton(DefaultSingletonBeanRegistry.java:230) ~[spring-beans-4.3.4.RELEASE.jar:4.3.4.RELEASE]
	at org.springframework.beans.factory.support.AbstractBeanFactory.doGetBean(AbstractBeanFactory.java:302) ~[spring-beans-4.3.4.RELEASE.jar:4.3.4.RELEASE]
	at org.springframework.beans.factory.support.AbstractBeanFactory.getBean(AbstractBeanFactory.java:197) ~[spring-beans-4.3.4.RELEASE.jar:4.3.4.RELEASE]
	at org.springframework.beans.factory.support.DefaultListableBeanFactory.preInstantiateSingletons(DefaultListableBeanFactory.java:754) ~[spring-beans-4.3.4.RELEASE.jar:4.3.4.RELEASE]
	at org.springframework.context.support.AbstractApplicationContext.finishBeanFactoryInitialization(AbstractApplicationContext.java:866) ~[spring-context-4.3.4.RELEASE.jar:4.3.4.RELEASE]
	at org.springframework.context.support.AbstractApplicationContext.refresh(AbstractApplicationContext.java:542) ~[spring-context-4.3.4.RELEASE.jar:4.3.4.RELEASE]
	at org.springframework.boot.SpringApplication.refresh(SpringApplication.java:761) ~[spring-boot-1.4.2.RELEASE.jar:1.4.2.RELEASE]
	at org.springframework.boot.SpringApplication.refreshContext(SpringApplication.java:371) ~[spring-boot-1.4.2.RELEASE.jar:1.4.2.RELEASE]
	at org.springframework.boot.SpringApplication.run(SpringApplication.java:315) ~[spring-boot-1.4.2.RELEASE.jar:1.4.2.RELEASE]
	at org.springframework.boot.test.context.SpringBootContextLoader.loadContext(SpringBootContextLoader.java:111) [spring-boot-test-1.4.2.RELEASE.jar:1.4.2.RELEASE]
	at org.springframework.test.context.cache.DefaultCacheAwareContextLoaderDelegate.loadContextInternal(DefaultCacheAwareContextLoaderDelegate.java:98) [spring-test-4.3.4.RELEASE.jar:4.3.4.RELEASE]
	at org.springframework.test.context.cache.DefaultCacheAwareContextLoaderDelegate.loadContext(DefaultCacheAwareContextLoaderDelegate.java:116) [spring-test-4.3.4.RELEASE.jar:4.3.4.RELEASE]
	at org.springframework.test.context.support.DefaultTestContext.getApplicationContext(DefaultTestContext.java:83) [spring-test-4.3.4.RELEASE.jar:4.3.4.RELEASE]
	at org.springframework.test.context.support.DependencyInjectionTestExecutionListener.injectDependencies(DependencyInjectionTestExecutionListener.java:117) [spring-test-4.3.4.RELEASE.jar:4.3.4.RELEASE]
	at org.springframework.test.context.support.DependencyInjectionTestExecutionListener.prepareTestInstance(DependencyInjectionTestExecutionListener.java:83) [spring-test-4.3.4.RELEASE.jar:4.3.4.RELEASE]
	at org.springframework.boot.test.autoconfigure.SpringBootDependencyInjectionTestExecutionListener.prepareTestInstance(SpringBootDependencyInjectionTestExecutionListener.java:44) [spring-boot-test-autoconfigure-1.4.2.RELEASE.jar:1.4.2.RELEASE]
	at org.springframework.test.context.TestContextManager.prepareTestInstance(TestContextManager.java:230) [spring-test-4.3.4.RELEASE.jar:4.3.4.RELEASE]
	at org.springframework.test.context.junit4.SpringJUnit4ClassRunner.createTest(SpringJUnit4ClassRunner.java:228) [spring-test-4.3.4.RELEASE.jar:4.3.4.RELEASE]
	at org.springframework.test.context.junit4.SpringJUnit4ClassRunner$1.runReflectiveCall(SpringJUnit4ClassRunner.java:287) [spring-test-4.3.4.RELEASE.jar:4.3.4.RELEASE]
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12) [junit-4.12.jar:4.12]
	at org.springframework.test.context.junit4.SpringJUnit4ClassRunner.methodBlock(SpringJUnit4ClassRunner.java:289) [spring-test-4.3.4.RELEASE.jar:4.3.4.RELEASE]
	at org.springframework.test.context.junit4.SpringJUnit4ClassRunner.runChild(SpringJUnit4ClassRunner.java:247) [spring-test-4.3.4.RELEASE.jar:4.3.4.RELEASE]
	at org.springframework.test.context.junit4.SpringJUnit4ClassRunner.runChild(SpringJUnit4ClassRunner.java:94) [spring-test-4.3.4.RELEASE.jar:4.3.4.RELEASE]
	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290) [junit-4.12.jar:4.12]
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71) [junit-4.12.jar:4.12]
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288) [junit-4.12.jar:4.12]
	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58) [junit-4.12.jar:4.12]
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268) [junit-4.12.jar:4.12]
	at org.springframework.test.context.junit4.statements.RunBeforeTestClassCallbacks.evaluate(RunBeforeTestClassCallbacks.java:61) [spring-test-4.3.4.RELEASE.jar:4.3.4.RELEASE]
	at org.springframework.test.context.junit4.statements.RunAfterTestClassCallbacks.evaluate(RunAfterTestClassCallbacks.java:70) [spring-test-4.3.4.RELEASE.jar:4.3.4.RELEASE]
	at org.junit.runners.ParentRunner.run(ParentRunner.java:363) [junit-4.12.jar:4.12]
	at org.springframework.test.context.junit4.SpringJUnit4ClassRunner.run(SpringJUnit4ClassRunner.java:191) [spring-test-4.3.4.RELEASE.jar:4.3.4.RELEASE]
	at org.apache.maven.surefire.junit4.JUnit4Provider.execute(JUnit4Provider.java:283) [surefire-junit4-2.18.1.jar:2.18.1]
	at org.apache.maven.surefire.junit4.JUnit4Provider.executeWithRerun(JUnit4Provider.java:173) [surefire-junit4-2.18.1.jar:2.18.1]
	at org.apache.maven.surefire.junit4.JUnit4Provider.executeTestSet(JUnit4Provider.java:153) [surefire-junit4-2.18.1.jar:2.18.1]
	at org.apache.maven.surefire.junit4.JUnit4Provider.invoke(JUnit4Provider.java:128) [surefire-junit4-2.18.1.jar:2.18.1]
	at org.apache.maven.surefire.booter.ForkedBooter.invokeProviderInSameClassLoader(ForkedBooter.java:203) [surefire-booter-2.18.1.jar:2.18.1]
	at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:155) [surefire-booter-2.18.1.jar:2.18.1]
	at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:103) [surefire-booter-2.18.1.jar:2.18.1]
Caused by: org.flywaydb.core.internal.dbsupport.FlywaySqlScriptException: 

My suggestion would be to allow the following sytnax

CREATE ROLE application WITH LOGIN PASSWORD ?{password};

or you could go with jpql named parameter style

CREATE ROLE application WITH LOGIN PASSWORD :password;
@alkamo

This comment has been minimized.

alkamo commented Jan 6, 2017

Most databases don't support bind variables for DDL. I couldn't find any documentation for Postgres one way or another, but I get the impression that it does not.

Are you sure the functionality you're asking for is possible?

@xenoterracide

This comment has been minimized.

xenoterracide commented Jan 6, 2017

I'm certain that liquibase provides these on liquibase.database.Database and can quote them in its xml.

    String table = database.escapeTableName( null, null, this.table );
    String col = database.escapeColumnName( null, null, this.table, this.field );

I'm also certain that ${...} is not sql, I'm not particularly certain about a bind parameter in the case of PASSWORD, I can probably check later.

I don't think it's impossible to provide ways to quote, or bind properties before they are passed to the sql system.

even if you had to write${quoteString(prop)} (or some alternative syntax I'm not currently thinking of) that'd be better than the possibility of an injection attack from a property source.

Don't focus on the example of CREATE USER focus on the fact that I could inject any sql statement that's using a "property placeholder" from the env in spring boot.

@alkamo

This comment has been minimized.

alkamo commented Jan 11, 2017

Escaping table (or object) names is different than using bind variables. In the original password example neither is typically allowed. That's a limitation of the databases, not Flyway.

Liquibase isn't a terribly good comparison as it attempts to fully describe the database, whereas Flyway, by definition, executes arbitrary SQL.

I understand the concern related to SQL injection, but I'd suggest that someone that has the privileges necessary to deploy your application probably doesn't need to use SQL injection to compromise it. In your example above, why wouldn't the person that has access to the environment running the release simply add a release script that drops the table?

I can see some utility in providing some sort of escaping functionality, but I think it would be pretty limited. In my experience, replacement values are more often used for partial identifiers (such as prefixes) rather than full object names. In that use case, escaping would be useless.

@xenoterracide

This comment has been minimized.

xenoterracide commented Jan 11, 2017

well as was pointed out this also helps prevent "stupid user" as much as "exploit" what if someone puts a ' in a password, that's valid for a password, but it needs escaping.

Yes bind parameters, and quote functionality are different. Even just having the bind parameters (where valid) would be nice, or perhaps some way of executing a quoting function before it's passed to sql (like SpEL). I'm not sure how you would do something like SpEL without a lot of work though, unless there's a standard way to execute someone else interpreter. Maybe an EL interpreter can be passed to (whatever factory flyway needs). Then you could have ?{foo} for bind parameters (where valid), and #{ evaluation } for other arbitrary things, and people bring their own "quoter".

I can see some utility in providing some sort of escaping functionality, but I think it would be pretty limited. In my experience, replacement values are more often used for partial identifiers (such as prefixes) rather than full object names. In that use case, escaping would be useless.

I don't have that experience

I understand the concern related to SQL injection, but I'd suggest that someone that has the privileges necessary to deploy your application probably doesn't need to use SQL injection to compromise it. In your example above, why wouldn't the person that has access to the environment running the release simply add a release script that drops the table?

in a Docker environment, it's possible to pass any env as --env to runtime, and spring boot allows you to override any property that is defined in an application.properties with an env variable. If someone hacks the "swarm manager" (kubernetes/docker swarm/etc), which may be on another server, they might be able to provide said env vars. What you wouldn't be able to do is add an arbitrary file to the container. In the same token as earlier, you might give someone access to the swarm manager to stop/start/change env vars (like IPs and connection credentials) and they might not know not to make the password h'ello$ (or really anything with a parameter ${...}).

Also worth calling out with these sorts of cloud paradigmns, it's possible to give an admin start/stop/configure, without ever giving them actual network access to the server, meaning that individual couldn't actually use the db credentials to log in themselves.

Lastly this example is, as I've said, the first parameter I tried, I tested, but in the future my plan is to only have changesets run as a user other than the "root" user for the database, thus restricting their access.

@lpetit-yseop

This comment has been minimized.

lpetit-yseop commented Jan 23, 2017

Just started with flyway and have the same concern.

My usecase is to create a migration for creating the first user admin user in the database, based on an email address to be provided at application startup (in my case via an env variable ADMIN_EMAIL).

I decided to circumvent the problem by avoiding placeholders and writing a java based migration instead. I'll then be able to use a prepared statement, and also to add some more logic (as in not trying to create the admin user if no ADMIN_EMAIL has been set. Or provide a sensible default value, etc.)

@axelfontaine

This comment has been minimized.

Contributor

axelfontaine commented Jan 30, 2017

I stand by @alkamo's point. Furthermore a good number of cases covered by placeholders simply cannot be served by bind variables (think replacing part of the SQL statement itself).

Won't fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment