Bug 1319740
Summary: | Tiering is not resistant to SQL-injection | ||
---|---|---|---|
Product: | [Community] GlusterFS | Reporter: | Niels de Vos <ndevos> |
Component: | tiering | Assignee: | bugs <bugs> |
Status: | CLOSED NOTABUG | QA Contact: | bugs <bugs> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | mainline | CC: | bugs, dlambrig, jdarcy, josferna, nbalacha |
Target Milestone: | --- | Keywords: | Triaged |
Target Release: | --- | ||
Hardware: | Unspecified | ||
OS: | Unspecified | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2016-03-29 12:46:05 UTC | Type: | Bug |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: |
Description
Niels de Vos
2016-03-21 12:07:02 UTC
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. |