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.
Created attachment 46406 [details] patch
Patch looks good to me. Invoking Adam for 2nd look :)
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.
(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.
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.
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.
Comment on attachment 46420 [details] patch ok
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.
Landed as r53177.
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.
(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.