Some bug fixes in censor logic

Author: storojs72

acra-censor/acra-censor_implementation.go

@@ -64,35 +64,40 @@ func (acraCensor *AcraCensor) ReleaseAll() {
 }
 
 // HandleQuery processes every query through each handler.
-func (acraCensor *AcraCensor) HandleQuery(query string) error {
-	if query == "" {
-		return nil
-	}
+func (acraCensor *AcraCensor) HandleQuery(rawQuery string) error {
 	if len(acraCensor.handlers) == 0 {
 		// no handlers, AcraCensor won't work
 		return nil
 	}
-	normalizedQuery, queryWithHiddenValues, err := common.NormalizeAndRedactSQLQuery(query)
-	if err == common.ErrQuerySyntaxError && acraCensor.ignoreParseError {
-		acraCensor.logger.WithError(err).Infof("Parsing error on query (first %v symbols): %s", common.LogQueryLength, common.TrimStringToN(queryWithHiddenValues, common.LogQueryLength))
-
-	}
-	if err != nil {
-		// ignore parsing errors to forward it as is to acra-censor to allow filter it via QueryIgnore handler
-		normalizedQuery = query
+	normalizedQuery, queryWithHiddenValues, parsedQuery, err := common.HandleRawSQLQuery(rawQuery)
+	if err == common.ErrQuerySyntaxError {
+		acraCensor.logger.WithError(err).Warning("Failed to parse input query")
+		if acraCensor.ignoreParseError {
+			acraCensor.logger.Infof("Unparsed query has been allowed")
+			return nil
+		}
+		acraCensor.logger.Errorf("Unparsed query has been forbidden")
+		return err
 	}
+
 	for _, handler := range acraCensor.handlers {
-		// in QueryCapture Handler use only redacted queries
+		// in QueryCapture Handler we use only redacted queries
 		if queryCaptureHandler, ok := handler.(*handlers.QueryCaptureHandler); ok {
-			queryCaptureHandler.CheckQuery(queryWithHiddenValues)
+			queryCaptureHandler.CheckQuery(queryWithHiddenValues, parsedQuery)
 			continue
 		}
-		continueHandling, err := handler.CheckQuery(normalizedQuery)
-		if err != nil {
-			// continue to next handler
-			if err == common.ErrQuerySyntaxError && acraCensor.ignoreParseError {
+		// in QueryIgnore Handler we use only raw queries
+		if queryIgnoreHandler, ok := handler.(*handlers.QueryIgnoreHandler); ok {
+			continueHandling, _ := queryIgnoreHandler.CheckQuery(rawQuery, parsedQuery)
+			if continueHandling {
 				continue
+			} else {
+				break
 			}
+		}
+		// remained handlers operate
+		continueHandling, err := handler.CheckQuery(normalizedQuery, parsedQuery)
+		if err != nil {
 			acraCensor.logger.Errorf("Forbidden query: '%s'", queryWithHiddenValues)
 			return err
 		}

acra-censor/acra-censor_interfaces.go

@@ -22,9 +22,11 @@ limitations under the License.
 // https://github.com/cossacklabs/acra/wiki/AcraCensor
 package acracensor
 
+import "github.com/xwb1989/sqlparser"
+
 // QueryHandlerInterface describes what actions are available for queries.
 type QueryHandlerInterface interface {
-	CheckQuery(sqlQuery string) (bool, error) //1st return arg specifies whether continue verification or not, 2nd specifies whether query is forbidden
+	CheckQuery(sqlQuery string, parsedQuery sqlparser.Statement) (bool, error) //1st return arg specifies whether continue verification or not, 2nd specifies whether query is forbidden
 	Release()
 }
 

acra-censor/acra-censor_test.go

@@ -105,7 +105,7 @@ func TestWhitelistQueries(t *testing.T) {
 		}
 	}
 	//acracensor should block this query because it is not in whitelist
-	err = acraCensor.HandleQuery("SELECT * FROM Schema.views;")
+	err = acraCensor.HandleQuery("SELECT * FROM testDB.testTbl;")
 	if err != common.ErrQueryNotInWhitelist {
 		t.Fatal(err)
 	}
@@ -774,7 +774,6 @@ func TestBlacklistQueries(t *testing.T) {
 		"SELECT Name, Age FROM Patients WHERE Age > 40 GROUP BY Age ORDER BY Name;",
 		"SELECT COUNT(CustomerID), Country FROM Customers GROUP BY Country;",
 		"SELECT SUM(Salary) FROM Employee WHERE Emp_Age < 30;",
-		"SELECT * FROM Schema.views;",
 	}
 	sqlInsertQueries := []string{
 		"INSERT SalesStaff1 VALUES (2, 'Michael', 'Blythe'), (3, 'Linda', 'Mitchell'),(4, 'Jillian', 'Carson'), (5, 'Garrett', 'Vargas');",
@@ -1828,23 +1827,35 @@ func TestDifferentTablesParsing(t *testing.T) {
 
 	blacklist := handlers.NewBlacklistHandler()
 	blacklist.AddTables([]string{"x", "y"})
-	_, err := blacklist.CheckQuery(testQuery)
+
+	acraCensor := NewAcraCensor()
+	defer acraCensor.ReleaseAll()
+	//set our acracensor to use blacklist for query evaluating
+	acraCensor.AddHandler(blacklist)
+
+	err := acraCensor.HandleQuery(testQuery)
 	if err != nil {
 		t.Fatal(err)
 	}
 	blacklist.AddTables([]string{"z", "Shippers"})
-	_, err = blacklist.CheckQuery(testQuery)
+	err = acraCensor.HandleQuery(testQuery)
 	if err != common.ErrAccessToForbiddenTableBlacklist {
 		t.Fatal(err)
 	}
+
+	acraCensor.RemoveHandler(blacklist)
+
 	whitelist := handlers.NewWhitelistHandler()
 	whitelist.AddTables([]string{"Orders", "Customers", "NotShippers"})
-	_, err = whitelist.CheckQuery(testQuery)
+
+	//set our acracensor to use whitelist for query evaluating
+	acraCensor.AddHandler(whitelist)
+	err = acraCensor.HandleQuery(testQuery)
 	if err != common.ErrAccessToForbiddenTableWhitelist {
 		t.Fatal(err)
 	}
 	whitelist.AddTables([]string{"Shippers"})
-	_, err = whitelist.CheckQuery(testQuery)
+	err = acraCensor.HandleQuery(testQuery)
 	if err != nil {
 		t.Fatal(err)
 	}

acra-censor/common/common.go

@@ -185,19 +185,20 @@ func TrimStringToN(query string, n int) string {
 	return query[:n]
 }
 
-// NormalizeAndRedactSQLQuery returns a normalized (lowercases SQL commands) SQL string,
+// HandleRawSQLQuery returns a normalized (lowercases SQL commands) SQL string,
 // and redacted SQL string with the params stripped out for display.
 // Taken from sqlparser package
-func NormalizeAndRedactSQLQuery(sql string) (normalizedQuery string, redactedQuery string, error error) {
+func HandleRawSQLQuery(sql string) (normalizedQuery, redactedQuery string, parsedQuery sqlparser.Statement, err error) {
 	bv := map[string]*querypb.BindVariable{}
 	sqlStripped, _ := sqlparser.SplitMarginComments(sql)
 
 	// sometimes queries might have ; at the end, that should be stripped
 	sqlStripped = strings.TrimSuffix(sqlStripped, ";")
 
 	stmt, err := sqlparser.Parse(sqlStripped)
+	outputStmt, _ := sqlparser.Parse(sqlStripped)
 	if err != nil {
-		return "", "", err
+		return "", "", nil, ErrQuerySyntaxError
 	}
 
 	normalizedQ := sqlparser.String(stmt)
@@ -206,5 +207,114 @@ func NormalizeAndRedactSQLQuery(sql string) (normalizedQuery string, redactedQue
 	sqlparser.Normalize(stmt, bv, ValueMask)
 	redactedQ := sqlparser.String(stmt)
 
-	return normalizedQ, redactedQ, nil
+	return normalizedQ, redactedQ, outputStmt, nil
+}
+
+// CheckPatternsMatching evaluates if parsed query matches specified set of patterns
+func CheckPatternsMatching(patterns []sqlparser.Statement, parsedQuery sqlparser.Statement) bool {
+	for _, pattern := range patterns {
+		if checkSinglePatternMatch(parsedQuery, pattern) {
+			return true
+		}
+	}
+	return false
+}
+
+// CheckExactQueriesMatch evaluates if query presents in set of queries
+func CheckExactQueriesMatch(normalizedQuery string, setOfQueries map[string]bool) bool {
+	if !setOfQueries[normalizedQuery] {
+		return false
+	}
+	return true
+}
+
+// CheckTableNamesMatch evaluates if query contains table presented in specified set of tables
+func CheckTableNamesMatch(parsedQuery sqlparser.Statement, setOfTables map[string]bool) (bool, bool) {
+	atLeastOneTableNameMatch := false
+	allTableNamesMatch := false
+
+	switch parsedQuery.(type) {
+	case *sqlparser.Select:
+		selectQuery := parsedQuery.(*sqlparser.Select)
+		atLeastOneTableNameMatch, allTableNamesMatch = checkTableExprsMatch(selectQuery.From, setOfTables)
+		break
+	case *sqlparser.Insert:
+		insertQuery := parsedQuery.(*sqlparser.Insert)
+		if setOfTables[insertQuery.Table.Name.String()] {
+			atLeastOneTableNameMatch = true
+			allTableNamesMatch = true
+		} else {
+			atLeastOneTableNameMatch = false
+			allTableNamesMatch = false
+		}
+		break
+	default:
+		//TODO other query types
+		return false, false
+	}
+
+	return atLeastOneTableNameMatch, allTableNamesMatch
+}
+
+// Tables matchers
+func checkTableExprsMatch(tables sqlparser.TableExprs, setOfTables map[string]bool) (bool, bool) {
+	oneTableMatch := false
+	allTablesMatch := false
+	counter := 0
+	for _, tableExpr := range tables {
+		oneTableMatchInternal, allTablesMatchInternal := checkTableExprMatch(tableExpr, setOfTables)
+		if oneTableMatchInternal {
+			oneTableMatch = true
+			if allTablesMatchInternal {
+				counter++
+			}
+		}
+	}
+	if counter == len(tables) {
+		allTablesMatch = true
+	}
+	return oneTableMatch, allTablesMatch
+}
+
+func checkTableExprMatch(table sqlparser.TableExpr, setOfTables map[string]bool) (bool, bool) {
+	oneTableMatch := false
+	allTablesMatch := false
+
+	switch table.(type) {
+	case *sqlparser.AliasedTableExpr:
+		oneTableMatch, allTablesMatch = checkAliasedTable(table.(*sqlparser.AliasedTableExpr), setOfTables)
+	case *sqlparser.JoinTableExpr:
+		oneTableMatch, allTablesMatch = checkJoinedTable(table.(*sqlparser.JoinTableExpr), setOfTables)
+	case *sqlparser.ParenTableExpr:
+		oneTableMatch, allTablesMatch = checkParenTable(table.(*sqlparser.ParenTableExpr), setOfTables)
+	}
+	return oneTableMatch, allTablesMatch
+}
+
+func checkAliasedTable(table *sqlparser.AliasedTableExpr, setOfTables map[string]bool) (bool, bool) {
+	if setOfTables[sqlparser.String(table.Expr)] {
+		return true, true
+	}
+	return false, false
+}
+
+func checkJoinedTable(table *sqlparser.JoinTableExpr, setOfTables map[string]bool) (bool, bool) {
+	oneTableMatch := false
+	allTablesMatch := false
+
+	oneLeftTableMatchInternal, allLeftTablesMatchInternal := checkTableExprMatch(table.LeftExpr, setOfTables)
+	oneRightTableMatchInternal, allRightTablesMatchInternal := checkTableExprMatch(table.RightExpr, setOfTables)
+
+	if oneLeftTableMatchInternal || oneRightTableMatchInternal {
+		oneTableMatch = true
+	}
+	if allLeftTablesMatchInternal && allRightTablesMatchInternal {
+		allTablesMatch = true
+	}
+	return oneTableMatch, allTablesMatch
+}
+
+func checkParenTable(table *sqlparser.ParenTableExpr, setOfTables map[string]bool) (bool, bool) {
+	singleTableMatch, allTablesMatch := checkTableExprsMatch(table.Exprs, setOfTables)
+	return singleTableMatch, allTablesMatch
 }

acra-censor/common/matching_logic.go

@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-// Package handlers contains all query handlers for AcraCensor:
+// Package common contains all query handlers for AcraCensor:
 // blacklist handler, which allows everything and forbids specific query/pattern/table;
 // whitelist handler, which allows query/pattern/table and restricts/forbids everything else;
 // ignore handler, which allows to ignore any query;
@@ -25,27 +25,11 @@ package common
 
 import (
 	"bytes"
-	log "github.com/sirupsen/logrus"
 	"github.com/xwb1989/sqlparser"
 	"reflect"
 	"strings"
 )
 
-func CheckPatternsMatching(patterns []sqlparser.Statement, query string) (bool, error) {
-	parsedQuery, err := sqlparser.Parse(query)
-	if err != nil {
-		log.WithError(err).Errorln("Can't parse query")
-		return false, ErrQuerySyntaxError
-	}
-
-	for _, pattern := range patterns {
-		if checkSinglePatternMatch(parsedQuery, pattern) {
-			return true, nil
-		}
-	}
-	return false, nil
-}
-
 func checkSinglePatternMatch(query, pattern sqlparser.Statement) bool {
 	switch pattern.(type) {
 	case *sqlparser.Union:

acra-censor/common/matching_logic_test.go

@@ -71,7 +71,13 @@ func testSingleTopLevelPlaceholder(t *testing.T, pattern string, indexOfMatchedQ
 	}
 	match := false
 	for index := range testQueries {
-		match, err = CheckPatternsMatching(parsedPatterns, testQueries[index])
+
+		stmt, err := sqlparser.Parse(testQueries[index])
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		match, err = CheckPatternsMatching(parsedPatterns, stmt)
 		if err != nil {
 			t.Fatal(err)
 		}