Description of problem: It is possible to execute SQL statements in the (server-side) tiering xlator by constructing suitable filenames. Version-Release number of selected component (if applicable): all How reproducible: 100% Steps to Reproduce: 1. create a tiered volume 2. mount the volume 3. create a file with name like 'README; DROP TABLE GF_FILE_TB; COMMIT;' Actual results: The GF_FILE_TB table gets dropped from the tiering database. Expected results: The filename should not get interpreted as SQL, and the file should just be created. Additional info: I do not think this is exploitable more than causing tiering to malfunction.
REVIEW: http://review.gluster.org/13799 (tests: Tiering is not resistant to SQL-injection) posted (#1) for review on master by Niels de Vos (ndevos)
A quick scan found three places where we construct query strings using variants of sprintf instead of sqlite3_bind_*. Of those, by far the most suspicious is gf_sql_update_link_flags, which is the only one where we string-substitute a file name. Furthermore, the file name is at the very end of the query string, which makes it easier to create a syntactically correct but malicious result. Clearly, sqlite_escape_string needs to be used here, but there are might be other changes necessary to handle the resulting filename correctly in other parts of the code.
(In reply to Jeff Darcy from comment #2) > A quick scan found three places where we construct query strings using > variants of sprintf instead of sqlite3_bind_*. Of those, by far the most > suspicious is gf_sql_update_link_flags, which is the only one where we > string-substitute a file name. Furthermore, the file name is at the very > end of the query string, which makes it easier to create a syntactically > correct but malicious result. Clearly, sqlite_escape_string needs to be > used here, but there are might be other changes necessary to handle the > resulting filename correctly in other parts of the code. I meant to say the moral equivalent of sqlite_escape_string, since that's a PHP function.
The more I look at this, the less convinced I am that it's real. For one thing, the test script is manipulating files in the current directory, not the mounted GlusterFS directory. The 'failures' seem more related to bash quoting issues than anything else. Once those are fixed, I see the same behavior for a GlusterFS mount as for a local filesystem, and no evidence that anything is amiss. We are using sqlite3_prepare/sqlite3_bind which should not be subject to injection issues in the first place. Has anyone looked directly at the database files to see if those tables really were dropped?
Right, the test-case is not correct. Dan copy/pasted my test-case and executed the commands manually. He inspected the sqlite database and the tables did not exist any more. Only a file with the (partial) name before the first ";" was created. I'll try to update the test-case later this week.
Per command history, the experiment yesterday did not include the quotes "" around $FILENAME. This seems to have inserted a partial file name, but I do not observe additional SQL commands executed and the table dropped. If you do add the quotes, the entire string is inserted. In the test case you need to cd to $M0. Let us know if tier_sql_injection.t can reproduce a problem. $ FILENAME='filename-before-sql-injection; DROP TABLE GF_FILE_TB; DROP TABLE GF_FLINK_TB; COMMIT;' $ touch /mnt/"${FILENAME}" $ echo "select * from gf_flink_tb;" | sqlite3 /home/t4/.glusterfs/t4.db ad837931-3359-4788-b571-4688471bdf4b|00000000-0000-0000-0000-00000000001|filename-before-sql-injection; DROP TABLE GF_FILE_TB; DROP TABLE GF_FLINK_TB; COMMIT;|0|0 $ stat /mnt/"${FILENAME}" File: ‘/mnt/filename-before-sql-injection; DROP TABLE GF_FILE_TB; DROP TABLE GF_FLINK_TB; COMMIT;’ Size: 0 Blocks: 0 IO Block: 131072 regular empty file Device: 27h/39d Inode: 13074308744355766091 Links: 1 Access: (0644/-rw-r--r--) Uid: ( 0/ root) Gid: ( 0/ root) Context: system_u:object_r:fusefs_t:s0 Access: 2016-03-22 01:39:26.462364000 -0400 Modify: 2016-03-22 01:39:26.462364000 -0400 Change: 2016-03-22 01:39:26.463364174 -0400 Birth: - [root@rhs-cli-01 rhs-glusterfs]# echo $? 0 try without quotes $ touch /mnt/$FILENAME $ echo "select * from gf_flink_tb;" | sqlite3 /home/t3/.glusterfs/t3.db 806f3d74-fb51-43d2-aef1-7c5a84fa5d2a|00000000-0000-0000-0000-000000000001|filename-before-sql-injection;|0|0 [root@rhs-cli-01 rhs-glusterfs]# ls -l /mnt total 0 -rw-r--r--. 1 root root 0 Mar 22 01:45 filename-before-sql-injection; -rw-r--r--. 1 root root 0 Mar 22 01:41 'filename-before-sql-injection; DROP TABLE GF_FILE_TB; DROP TABLE GF_FLINK_TB; COMMIT;'
Per comment #4 and comment #6, code inspection and testing, this appears to not be a bug.