from {{Oracle8iDialect}}:374
{code} @Override public String getSelectSequenceNextValString(String sequenceName) { return sequenceName + ".nextval"; } {code}
this means that if you write code like [this|http://stackoverflow.com/a/23267212/206466] if your sequence name comes from user input it's vulnerable to sql injection. Here's a partial proof of concept (from our code) I can write a full one if necessary.
{code :java } int getNextCodeIntegerFromSequence( final TestClassification classification ) { Long next = entityManager.getSession() .doReturningWork( conn -> { DatabaseMetaDataDialectResolutionInfoAdapter info = new DatabaseMetaDataDialectResolutionInfoAdapter( conn.getMetaData() ); Dialect dialect = new StandardDialectResolver().resolveDialect( info ); String seq = classification.getCodeSequence(); /* if ( StringUtils.containsAny( seq, dialect.openQuote(), dialect.closeQuote() )) { String msg = String.format( "classification is being nefarious: '%s'", classification ); throw new IllegalArgumentException( msg ); } */ String quoted = dialect.quote( "`" + seq + "`" ); String sql = dialect.getSequenceNextValString( quoted );
try ( PreparedStatement stmt = conn.prepareStatement( sql ); ResultSet res = stmt.executeQuery() ) { while ( res.next() ) { return res.getLong( 1 ); } } String fmt = "something went wrong, you shouldn't reach this, here's the " + "classifcation: %s"; throw new NoResultException( String.format( fmt, classification ) ); } );
return next.intValue(); } {code} and a test {code :java } @Test( expected = IllegalArgumentException.class ) public void getNextCodeIntegerFromSequenceExploit() { TestClassification classification = testClassificationRepo.findOne( 1L ); classification.setCodeSequence( classification.getCodeSequence() + "\"; drop table site_user; --" );
codeDao.getNextCodeIntegerFromSequence( classification ); // throws, code below never reached but left for // proof of concept testing
Integer siteUserTables = codeDao.getEntityManager().getSession() .doReturningWork( conn -> { String sql = " SELECT COUNT (TABLE_NAME ) from information_schema.tables " + " WHERE table_name = ? ";
try ( PreparedStatement stmt = conn.prepareStatement( sql ) ) { stmt.setString( 1, "SITE_USER" ); try ( ResultSet res = stmt.executeQuery() ) { while ( res.next() ) { return res.getInt( 1 ); } } return null; } } ); assertThat( siteUserTables, greaterThan( 0 ) ); } {code}
it should probably be implemented as {code} @Override public String getSelectSequenceNextValString(String sequenceName) { return quote( sequenceName ) + ".nextval"; } {code}
of course then looking at the implementation of {{Dialect.quote}}, it wouldn't actually do anything. After reviewing, oracle, h2, and postgres docs (pg says this)
{quote}Note that dollar signs are not allowed in identifiers according to the letter of the SQL standard, so their use might render applications less portable. {quote}
I think that the quoting character for an identifier is never allowed in an identifier. So I think this can be added to the beginning of quote. {{StringUtils}} is from apache commons lang3
{code} if ( StringUtils.containsAny( name, this.openQuote(), this.closeQuote() )) { String msg = String.format( "illegal characters in: '%s'", name ); throw new IllegalArgumentException( msg ); } {code}
If this plan is approved I can write a patch, including one that doesn't use commons lang 3 |
|