Bug 33549 – Disallow some sqlite functions that are not safe

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dumitru Daniliuc]

Some sqlite functions are not safe, and we should disallow them. The best way to do it is to have a list of whitelisted functions in the authorizer. That would guard us against new functions that might not be safe in future sqlite releases.

[Dumitru Daniliuc]

Created attachment 46406 [details]
patch

[Chris Evans]

Patch looks good to me.
Invoking Adam for 2nd look :)

[Adam Barth]

Looks reasonable to me.  I don't understand this change:

-              return auth->allowFunction(parameter1);
+             return auth->allowFunction(parameter2);

Also, I'm not an expert on these functions.

[Dumitru Daniliuc]

(In reply to comment #3)
> Looks reasonable to me.  I don't understand this change:
> 
> -              return auth->allowFunction(parameter1);
> +             return auth->allowFunction(parameter2);
> 
> Also, I'm not an expert on these functions.

it was a bug. according to http://www.sqlite.org/c3ref/c_alter_table.html the function name is passed in the 4th parameter, not the 3rd.

[Adam Barth]

Comment on attachment 46406 [details]
patch

I'm marking this r+ because this was discussed on webkit-dev.  If folks have additional feedback, please me me or Dumitru know.

[Dumitru Daniliuc]

Created attachment 46420 [details]
patch

Same patch, with 3 new functions whitelisted:

sqlite_rename_table: used by the ALTER TABLE command
sqlite_rename_trigger: used by the ALTER TRIGGER command
glob: used by the GLOB operator

I've grep'd the sqlite source code for calls to sqlite3CreateFunc() and these were the only 3 "helper functions" that seem to be registered by the sqlite code.

[Adam Barth]

Comment on attachment 46420 [details]
patch

ok

[Dumitru Daniliuc]

Forgot to add that if sqlite_rename_table is not whitelisted, then a command like 'ALTER TABLE Blah RENAME to BlahBlah;' will fail. Not whitelisting the other 2 functions has similar consequences.

[Dumitru Daniliuc]

Landed as r53177.

[Darin Adler]

Comment on attachment 46420 [details]
patch

Code like this should use a case folding HashSet rather than calling lower each time it calls contains on the HashSet. See examples in DOMImplementation.cpp, ScriptElement.cpp, CrossOriginAccessControl.cpp, RenderEmbeddedObject.cpp, and XMLHttpRequest.cpp.

I also see incorrect indenting, 2 spaces instead of 4, in the patch.

> +  if (m_securityEnabled && !m_whitelistedFunctions.contains(functionName.lower()))
> +    return SQLAuthDeny;
> +
> +  return SQLAuthAllow;

Incorrect indenting here. Two spaces instead of 4.

[Dumitru Daniliuc]

(In reply to comment #10)
> (From update of attachment 46420 [details])
> Code like this should use a case folding HashSet rather than calling lower each
> time it calls contains on the HashSet. See examples in DOMImplementation.cpp,
> ScriptElement.cpp, CrossOriginAccessControl.cpp, RenderEmbeddedObject.cpp, and
> XMLHttpRequest.cpp.
> 
> I also see incorrect indenting, 2 spaces instead of 4, in the patch.
> 
> > +  if (m_securityEnabled && !m_whitelistedFunctions.contains(functionName.lower()))
> > +    return SQLAuthDeny;
> > +
> > +  return SQLAuthAllow;
> 
> Incorrect indenting here. Two spaces instead of 4.

Opened bug 33612 to address Darin's comments.