Summary: | Disallow some sqlite functions that are not safe | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dumitru Daniliuc <dumi> | ||||||
Component: | New Bugs | Assignee: | Dumitru Daniliuc <dumi> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | abarth, beidson, dglazkov, michaeln, scarybeasts | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
Dumitru Daniliuc
2010-01-12 13:09:20 PST
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. 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. |