Add some patterns support to blacklist [T660] #226

storojs72 · 2018-07-28T17:30:40Z

Add support of %%SELECT%%
Add support of SELECT %%COLUMN%% .. %%COLUMN%%
Add support of SELECT A,B FROM T %%WHERE%%

I added some stubs (commented some places in code) to whitelist workflow - would like to know from reviewers if blacklist now will work as expected

…pport

…ensor_refactoring

storojs72 · 2018-07-29T15:49:38Z

Add support of SELECT a, b FROM t1 WHERE userID=%%VALUE%%

Lagovas · 2018-07-30T09:22:15Z

acra-censor/acra-censor_test.go

+
+func TestBlacklistPatterns(t *testing.T) {
+	//test %%SELECT%% pattern
+	testBlacklistPattern0(t)


better to use meaningful names for functions:

testBlacklistSelectPattern

testBlacklistColumnPattern

testBlacklistWherePattern

testBlacklistValuePattern

Lagovas · 2018-07-30T09:24:18Z

acra-censor/acra-censor_test.go

+	//Queries that should be blocked by first pattern have indexes: [0 .. 12] (all select queries)
+	for i, query := range queries {
+		err := censor.HandleQuery(query)
+		if i <= 12 {


const SelectQueryCount = 12 for .... if i < SelectQueryCount {} ....

It is less readable when we need to find comment some line above that explain what the number mean in if clause

Lagovas · 2018-07-30T09:26:53Z

acra-censor/acra-censor_test.go

+	testBlacklistPattern3(t)
+
+}
+func testBlacklistPattern0(t *testing.T) {


would be good to add some general comment about whole test case like "test that all 'select' queries will be blocked by blacklist handler". after that future reader will understand whole logic and know what expect
or use verbose function names :)

Lagovas · 2018-07-30T09:34:21Z

acra-censor/acra-censor_test.go

+	queries := []string{
+		"SELECT A",
+		"SELECT A, B",
+		"SELECT A, B, C",


better to use two arrays of positive and negative values to check them in a loop.
in a future when someone will need to add some special value to check he will need just to add new query to correct array
or copy-paste one more if block and increase code size without need
plus it's more readable when you see

for _, query := range AcceptableQueries { // do something if err != nil { t.Log("unexpected error") } } for _, blockableQuery := range BlockableQueries { // do something if err == nil { t.Log("we expect error") } }

Lagovas · 2018-07-30T09:37:04Z

acra-censor/acra-censor_test.go

-			t.Fatal(err)
-		}
+	queries := []string{
+		"SELECT a, b, c FROM x WHERE a = 'someValue'",


add cases with two/three conditions: a='some value' and b=2 or c between 20 and 30

Lagovas · 2018-07-30T09:47:44Z

acra-censor/handlers/blacklist_handler.go

 		if err != nil {
-			log.WithField(logging.FieldKeyEventCode, logging.EventCodeErrorCensorQueryParseError).WithError(err).Errorln("Can't parse query to add rule to blacklist handler")
-			return ErrQuerySyntaxError


this type of error specially handled on acra-censor level to ignore errors when we can't parse sql queries

Lagovas · 2018-07-30T09:49:34Z

acra-censor/handlers/blacklist_handler.go

+	patternDetected := false
+	matchOccurred := false
+	patternDetected, matchOccurred = handleSelectPattern(queryNodes, patternNodes)
+	if patternDetected {


why not if patternDetected && matchOccurred?

and for what we use patternDetected? why not just check that matchOccurred?

Lagovas · 2018-07-30T09:56:34Z

acra-censor/handlers/blacklist_handler.go

+func handleSelectColumnPattern(queryNodes, patternNodes []sqlparser.SQLNode) (patternDetected bool, matchOccurred bool) {
+	var emptySelect *sqlparser.Select
+	if reflect.TypeOf(queryNodes[0]) == reflect.TypeOf(emptySelect) && reflect.TypeOf(patternNodes[0]) == reflect.TypeOf(emptySelect) {
+		if strings.Contains(sqlparser.String(patternNodes[0].(*sqlparser.Select).SelectExprs), ColumnConfigPlaceholderReplacer) {


can we somehow to remember result of it and don't check it always? for example on config load and remembering patterns that contains such pattern and check only them with this filter?

Lagovas · 2018-07-30T09:59:38Z

acra-censor/handlers/blacklist_handler.go

+	var emptySelect *sqlparser.Select
+	if reflect.TypeOf(queryNodes[0]) == reflect.TypeOf(emptySelect) && reflect.TypeOf(patternNodes[0]) == reflect.TypeOf(emptySelect) {
+		if strings.Contains(sqlparser.String(patternNodes[0].(*sqlparser.Select).SelectExprs), ColumnConfigPlaceholderReplacer) {
+			if len(patternNodes[0].(*sqlparser.Select).SelectExprs) == len(queryNodes[0].(*sqlparser.Select).SelectExprs) {


how we check that places of pattern %%COLUMN%% the same as in query? how it handle cases like:
pattern: select a, %%COLUMN%% from table
query: select b, blabla from table ?
We doesn't calculate count of %%column%% pattern to compare it with columns count in query

Lagovas · 2018-07-30T10:01:34Z

acra-censor/handlers/blacklist_handler.go

+//handle SELECT a, b from t %%WHERE%% pattern
+func handleSelectWherePattern(queryNodes, patternNodes []sqlparser.SQLNode) (patternDetected bool, matchOccurred bool) {
+	var emptySelect *sqlparser.Select
+	if reflect.TypeOf(queryNodes[0]) == reflect.TypeOf(emptySelect) && reflect.TypeOf(patternNodes[0]) == reflect.TypeOf(emptySelect) {


we use this check > 1 times. what about to add some helper like func isSelectQuery(node sqlparser.SQLNode) and then use:
if isSelectQuery(queryNodes[0]) && isSelectQuery(patternNodes[0]) ? it's more readable and we encapsulate logic of detecting query type

Lagovas · 2018-07-30T10:15:08Z

select %%COLUMN%%, %%COLUMN%% great pattern support but we need select %%SKIP_ALL_COLUMNS%% too. It will provide easy way to skip any permutations of columns in select clause and different column count. Actually, we planned it at start in T660

vixentael · 2018-07-30T11:16:28Z

acra-censor/handlers/blacklist_handler.go

@@ -160,153 +157,208 @@ func (handler *BlacklistHandler) handleParenTables(statement *sqlparser.ParenTab
 	return nil
 }

-// Reset blacklist rules
 func (handler *BlacklistHandler) Reset() {


removed comment :(

vixentael · 2018-07-30T11:16:32Z

acra-censor/handlers/blacklist_handler.go

 }

-// Release / reset blacklist rules


removed comment :(

vixentael · 2018-07-30T11:16:37Z

acra-censor/handlers/blacklist_handler.go

 func (handler *BlacklistHandler) Release() {
 	handler.Reset()
 }

-// AddQueries adds queries to the list that should be blacklisted


removed comment :(

vixentael · 2018-07-30T11:17:36Z

Please don't remove golint comments, it took me some time to add them.

vixentael · 2018-07-30T12:21:48Z

acra-censor/handlers/blacklist_handler.go

 }

-// NewBlacklistHandler creates new blacklist handler
 func NewBlacklistHandler() *BlacklistHandler {


still removed

… + some refactoring

Lagovas · 2018-07-31T09:41:27Z

acra-censor/handlers/blacklist_handler.go

+		if index == 0 || reflect.DeepEqual(patternNode, queryNodes[index]) {
+			continue
+		}
+		if patternNodeColName, ok := patternNode.(*sqlparser.ColName); ok && patternNodeColName != nil {


what if type of node in query != type of node in pattern and it's not ColName? Can we check that they both has same type and then check value of .Name?

Lagovas · 2018-07-31T09:47:09Z

acra-censor/handlers/blacklist_handler.go

+		if queryWhereNode, ok := queryNodes[index].(*sqlparser.Where); ok && queryWhereNode != nil {
+			queryWhereDetected = true
+		}
+		if patternWhereDetected == queryWhereDetected && patternWhereDetected {


true == true && true ?)
better to leave only patternWhereDetected && queryWhereDetected, because it's true only when both are true

Lagovas · 2018-07-31T09:48:29Z

acra-censor/handlers/blacklist_handler.go

@@ -88,9 +89,10 @@ func (handler *BlacklistHandler) CheckQuery(query string) (bool, error) {
 	}
 	//Check patterns
 	if len(handler.patterns) != 0 {
+		//fmt.Println("here")


git add -p acra-censor/handlers/blacklist_handler.go and you can not include this :)

…ing'

Lagovas · 2018-07-31T12:31:19Z

acra-censor/acra-censor_test.go

+		"SELECT * FROM company WHERE AGE NOT IN ( 25, 27 )",
+		"SELECT * FROM company WHERE AGE > (SELECT AGE FROM COMPANY WHERE SALARY > 65000)",
+		"SELECT AGE FROM company WHERE EXISTS (SELECT AGE FROM company WHERE SALARY > 65000)",
+		"SELECT age FROM company11 WHERE EXISTS (SELECT AGE FROM company WHERE SALARY > 65000)",


why this query blockable too? company != company11

Because internal query inside EXISTS block try to access to 'company'

Lagovas · 2018-07-31T12:52:14Z

acra-censor/handlers/blacklist_handler.go

+			queryTables = querySelectStatement.From
+		}
+	}
+	if patternStarDetected {


can we move this check after walking through patternNodes and write if !patternStarDetected{ return false} to avoid processing second loop?

Lagovas · 2018-08-01T14:47:31Z

acra-censor/handlers/blacklist_handler.go

-		if querySelectStatement, ok := queryNode.(*sqlparser.Select); ok && querySelectStatement != nil {
-			queryTables = querySelectStatement.From
+		if queryWhereNode, ok := queryNodes[index+queryNodeOffset].(*sqlparser.Where); ok && queryWhereNode != nil {
+			queryWhereDetected = true


can we return true here and false in else block and don't check queryWhereDetected && partternWhereDetected?

what if nodes are not both *sqlparser.Where?

Lagovas · 2018-08-01T14:48:52Z

acra-censor/handlers/blacklist_handler.go

+	starDetected := false
+	sqlparser.Walk(func(node sqlparser.SQLNode) (bool, error) {
+		if _, ok := node.(*sqlparser.StarExpr); ok {
+			starDetected = true


can we stop walking here somehow? return error or false maybe

vixentael · 2018-08-01T16:09:51Z

acra-censor/handlers/whitelist_handler.go

-		}
-	}
-}
+//func (handler *WhitelistHandler) RemoveRules(rules []string) {


if it's commented, remove it?

vixentael · 2018-08-01T16:10:11Z

acra-censor/handlers/whitelist_handler.go

@@ -10,15 +10,13 @@ import (
 	log "github.com/sirupsen/logrus"
 )

-// WhitelistHandler shows handler structure
 type WhitelistHandler struct {


comment missing

vixentael · 2018-08-01T16:10:16Z

acra-censor/handlers/whitelist_handler.go

 type WhitelistHandler struct {
 	queries map[string]bool
 	tables  map[string]bool
 	rules   []string
 	logger  *log.Entry
 }

-// NewWhitelistHandler creates new white handler
 func NewWhitelistHandler() *WhitelistHandler {


comment missing

vixentael · 2018-08-01T16:10:19Z

acra-censor/handlers/whitelist_handler.go

@@ -27,8 +25,6 @@ func NewWhitelistHandler() *WhitelistHandler {
 	return handler
 }

-// CheckQuery checks each query, returns false and error if query is not in a white list or
-// if query tries to access to non-whitelisted table
 func (handler *WhitelistHandler) CheckQuery(query string) (bool, error) {


comment missing

vixentael · 2018-08-01T16:10:24Z

acra-censor/handlers/whitelist_handler.go

@@ -191,48 +187,41 @@ func (handler *WhitelistHandler) handleParenTables(statement *sqlparser.ParenTab
 	return nil
 }

-// Reset whitelist rules
 func (handler *WhitelistHandler) Reset() {


comment missing

vixentael · 2018-08-01T16:10:29Z

acra-censor/handlers/whitelist_handler.go

 func (handler *WhitelistHandler) Reset() {
 	handler.queries = make(map[string]bool)
 	handler.tables = make(map[string]bool)
 	handler.rules = nil
 }

-// Release / reset whitelist rules
 func (handler *WhitelistHandler) Release() {


comment missing

vixentael · 2018-08-01T16:10:32Z

acra-censor/handlers/whitelist_handler.go

 func (handler *WhitelistHandler) Release() {
 	handler.Reset()
 }

-// AddQueries adds queries to the list that should be whitelisted
 func (handler *WhitelistHandler) AddQueries(queries []string) {


comment missing

vixentael · 2018-08-01T16:10:35Z

acra-censor/handlers/whitelist_handler.go

 func (handler *WhitelistHandler) AddQueries(queries []string) {
 	for _, query := range queries {
 		handler.queries[query] = true
 	}
 }

-// RemoveQueries removes queries from the list that should be whitelisted
 func (handler *WhitelistHandler) RemoveQueries(queries []string) {


comment missing

vixentael · 2018-08-01T16:10:39Z

acra-censor/handlers/whitelist_handler.go

 func (handler *WhitelistHandler) RemoveQueries(queries []string) {
 	for _, query := range queries {
 		delete(handler.queries, query)
 	}
 }

-// AddTables adds tables that should be whitelisted
 func (handler *WhitelistHandler) AddTables(tableNames []string) {


comment missing

vixentael · 2018-08-01T16:10:42Z

acra-censor/handlers/whitelist_handler.go

 func (handler *WhitelistHandler) AddTables(tableNames []string) {
 	for _, tableName := range tableNames {
 		handler.tables[tableName] = true
 	}
 }

-// RemoveTables removes whitelisted tables
 func (handler *WhitelistHandler) RemoveTables(tableNames []string) {


comment missing

vixentael · 2018-08-01T16:10:47Z

acra-censor/handlers/whitelist_handler.go

 func (handler *WhitelistHandler) RemoveTables(tableNames []string) {
 	for _, tableName := range tableNames {
 		delete(handler.tables, tableName)
 	}
 }

-// AddRules adds rules that should be whitelisted
-func (handler *WhitelistHandler) AddRules(rules []string) error {
+func (handler *WhitelistHandler) AddPatterns(rules []string) error {


comment missing

vixentael · 2018-08-01T16:11:21Z

acra-censor/acra-censor_test.go

-			t.Fatal(err)
-		}
-	}
+	//whitelistHandler.RemovePatterns(testSecurityRules)


why is it commented?

vixentael · 2018-08-01T16:11:53Z

acra-censor/handlers/blacklist_handler.go

 )

-// BlacklistHandler shows handler structure
 type BlacklistHandler struct {


comment missing

vixentael · 2018-08-01T16:12:40Z

acra-censor/handlers/blacklist_handler.go

 	handler.logger = log.WithField("handler", "blacklist")
 	return handler
 }

-// CheckQuery checks each query, returns false and error if query is blacklisted or
-// if query tries to access to forbidden table
+//CheckQuery checks input query


new comment doesn't look very helpful, however old one was.

maybe update old comment?

vixentael · 2018-08-01T16:14:01Z

acra-censor/handlers/blacklist_handler.go

@@ -105,8 +103,9 @@ func (handler *BlacklistHandler) CheckQuery(query string) (bool, error) {
 func (handler *BlacklistHandler) handleAliasedTables(statement *sqlparser.AliasedTableExpr) error {
 	if handler.tables[sqlparser.String(statement.Expr)] {
 		return ErrAccessToForbiddenTableBlacklist
+	} else {


golint has a rule: if method ends on

if return else return

it should be changed to (remove else)

if return return

storojs72 · 2018-08-01T16:37:15Z

I think that we can now merge (into acracensor branch), and more carefully look for a golint when will merge into master

vixentael · 2018-08-01T16:47:16Z

Merge it!

…

On Wed, Aug 1, 2018 at 19:37 Artem Storozhuk ***@***.***> wrote: I think that we can now merge (into acracensor branch), and more carefully look for a golint when will merge into master — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#226 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACvp4Do_H0dPt-QeBhFH343o2w2k0sEbks5uMdk7gaJpZM4VlHtG> .

storojs72 added 4 commits July 28, 2018 14:47

Rename everywhere [rules] -> [patterns] and add %%SELECT%% pattern su…

14a02a5

…pport

Add SELECT %%COLUMN%% .. %%COLUMN%% and %%WHERE%% patterns support

588f9a0

Making some stubs in whitelist (would like to check blacklist first)

3440ba2

Some changes in configs to match patterns instead of obsolete rules

5e4f81a

storojs72 requested review from vixentael, Lagovas and shadinua July 28, 2018 17:30

storojs72 added 4 commits July 28, 2018 20:34

Merge branch 'master' into censor_refactoring

9701ae6

Add support of 'SELECT a, b FROM t1 WHERE userID=%%VALUE%%' pattern

1368b2d

Some refactoring

4735c8e

Merge branch 'censor_refactoring' of github.com:storojs72/acra into c…

bea3192

…ensor_refactoring

Lagovas requested changes Jul 30, 2018

View reviewed changes

storojs72 changed the base branch from master to acracensor July 30, 2018 11:11

vixentael reviewed Jul 30, 2018

View reviewed changes

acra-censor/handlers/blacklist_handler.go

}

// Release / reset blacklist rules

Copy link

Collaborator

vixentael Jul 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed comment :(

vixentael reviewed Jul 30, 2018

View reviewed changes

storojs72 added 2 commits July 30, 2018 14:18

Requested changes (meaningful names)

0f26772

Requested changes (Add SelectQueryCount const for readability)

c7989a7

vixentael added the acra-censor label Jul 30, 2018

storojs72 added 3 commits July 30, 2018 14:41

Requested changes (add acceptable/blockable queries for readability)

6f26b95

Requested changes (add cases with some conditions)

f1504c3

Requested changes (add comments for golint)

43a756c

vixentael reviewed Jul 30, 2018

View reviewed changes

storojs72 added 2 commits July 31, 2018 11:45

Requested changes (handle SELECT A, %%COLUMN%% FROM testTable' cases)…

4a86aa5

… + some refactoring

Some refactoring

3dd7873

Lagovas requested changes Jul 31, 2018

View reviewed changes

Add support of 'SELECT * FROM table %%WHERE%% pattern + some refactor…

349fd27

…ing'

storojs72 added 2 commits July 31, 2018 14:34

Requested changes

af28384

Requested changes (ColName check)

a475643

Lagovas reviewed Jul 31, 2018

View reviewed changes

storojs72 added 2 commits July 31, 2018 16:10

Requested changes (avoid processing loop if possible)

c2b542a

Rewrite '*' pattern

4a4d8e8

Lagovas reviewed Aug 1, 2018

View reviewed changes

Requested changes (skip all nodes walking if star found)

7137fb9

Lagovas approved these changes Aug 1, 2018

View reviewed changes

vixentael reviewed Aug 1, 2018

View reviewed changes

acra-censor/acra-censor_test.go

t.Fatal(err)

}

}

//whitelistHandler.RemovePatterns(testSecurityRules)

Copy link

Collaborator

vixentael Aug 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is it commented?

vixentael reviewed Aug 1, 2018

View reviewed changes

Requested changes (golint)

2433966

More golint fixes

56e0e5d

storojs72 merged commit 9632850 into cossacklabs:acracensor Aug 1, 2018

Add some patterns support to blacklist [T660] #226

Add some patterns support to blacklist [T660] #226

Conversation

storojs72 commented Jul 28, 2018

storojs72 commented Jul 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lagovas commented Jul 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vixentael commented Jul 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

storojs72 commented Aug 1, 2018

vixentael commented Aug 1, 2018 via email