Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Flag more code patterns in asyncpg-sqli ruleset #3024

Merged
merged 1 commit into from
Aug 3, 2023

Conversation

kholia
Copy link
Contributor

@kholia kholia commented Aug 2, 2023

Related: #3022.

This PR adds another pattern in asyncpg-sqli ruleset to catch more bad code patterns.

Before this PR

$ semgrep --config asyncpg-sqli.yaml asyncpg-sqli.py
               
               
┌─────────────┐
│ Scan Status │
└─────────────┘
  Scanning 1 file tracked by git with 1 Code rule:
  Scanning 1 file.
  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 100% 0:00:00                                                                                                                        
                    
                    
┌──────────────────┐
│ 10 Code Findings │
└──────────────────┘
                                   
    asyncpg-sqli.py 
       asyncpg-sqli                                                                         
          Detected string concatenation with a non-literal variable in a asyncpg Python SQL statement.
          This could lead to SQL injection if the variable is user-controlled and not properly        
          sanitized. In order to prevent SQL injection, use parameterized queries or prepared         
          statements instead. You can create parameterized queries like so: 'conn.fetch("SELECT $1    
          FROM table", value)'. You can also create prepared statements with 'Connection.prepare':    
          'stmt = conn.prepare("SELECT $1 FROM table"); await stmt.fetch(user_value)'                 
                                                                                                      
           10┆ values = await conn.fetch(query)
            ⋮┆----------------------------------------
           17┆ cur = await conn.cursor(sql_query)
            ⋮┆----------------------------------------
           23┆ await connection.execute(sql_query)
            ⋮┆----------------------------------------
           30┆ await pool.fetch(sql_query)
            ⋮┆----------------------------------------
           37┆ await con.execute("SELECT name FROM users WHERE age=" + req.FormValue("age"))
            ⋮┆----------------------------------------
           44┆ await con.execute('SELECT * FROM {}'.format(user_input))
            ⋮┆----------------------------------------
           50┆ conn.execute('SELECT * FROM %s'%(user_input))
            ⋮┆----------------------------------------
           54┆ conn.fetchrow(f'SELECT * FROM {user_input}')
            ⋮┆----------------------------------------
           58┆ conn.execute(
           59┆ "insert into %s values (%%s, %%s)" % ext.quote_ident(table_name),[10, 20])
            ⋮┆----------------------------------------
           65┆ cur = await conn.cursor(sql_query)

Note: It does NOT detect the following (two) SQLi problems (aka false negatives):

def bad11(conn: Connection):
    import common
    sql_query = common.bad_query_1.format(user_input)
    # ruleid: asyncpg-sqli
    cur = conn.fetch(sql_query)
    # ruleid: asyncpg-sqli
    cur = conn.fetch(common.bad_query_1.format(user_input))

After this PR

$ semgrep --config asyncpg-sqli.yaml asyncpg-sqli.py
               
               
┌─────────────┐
│ Scan Status │
└─────────────┘
  Scanning 1 file tracked by git with 1 Code rule:
  Scanning 1 file.
  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 100% 0:00:00                                                                                                                        
                    
                    
┌──────────────────┐
│ 11 Code Findings │
└──────────────────┘
                                   
    asyncpg-sqli.py 
       asyncpg-sqli                                                                         
          Detected string concatenation with a non-literal variable in a asyncpg Python SQL statement.
          This could lead to SQL injection if the variable is user-controlled and not properly        
          sanitized. In order to prevent SQL injection, use parameterized queries or prepared         
          statements instead. You can create parameterized queries like so: 'conn.fetch("SELECT $1    
          FROM table", value)'. You can also create prepared statements with 'Connection.prepare':    
          'stmt = conn.prepare("SELECT $1 FROM table"); await stmt.fetch(user_value)'                 
                                                                                                      
           10┆ values = await conn.fetch(query)
            ⋮┆----------------------------------------
           17┆ cur = await conn.cursor(sql_query)
            ⋮┆----------------------------------------
           23┆ await connection.execute(sql_query)
            ⋮┆----------------------------------------
           30┆ await pool.fetch(sql_query)
            ⋮┆----------------------------------------
           37┆ await con.execute("SELECT name FROM users WHERE age=" + req.FormValue("age"))
            ⋮┆----------------------------------------
           44┆ await con.execute('SELECT * FROM {}'.format(user_input))
            ⋮┆----------------------------------------
           50┆ conn.execute('SELECT * FROM %s'%(user_input))
            ⋮┆----------------------------------------
           54┆ conn.fetchrow(f'SELECT * FROM {user_input}')
            ⋮┆----------------------------------------
           58┆ conn.execute(
           59┆ "insert into %s values (%%s, %%s)" % ext.quote_ident(table_name),[10, 20])
            ⋮┆----------------------------------------
           65┆ cur = await conn.cursor(sql_query)
            ⋮┆----------------------------------------
           73┆ cur = conn.fetch(common.bad_query_1.format(user_input))

While it catches one more problem, it fails to flag the following code construct:

sql_query = common.bad_query_1.format(user_input)
# ruleid: asyncpg-sqli
cur = conn.fetch(sql_query)

It would be awesome to extend this PR to catch this problematic code block as well - thanks for the help.

@kholia kholia force-pushed the asyncpg-sqli-updates branch from 6a9caae to 41661c6 Compare August 3, 2023 03:31
@kurt-r2c kurt-r2c merged commit 57fcb9c into semgrep:develop Aug 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants