8

The problem

I want to write a wrapper around some DBI functions that allows safe execution of parameterized queries. I've found the this resource that explains how to use the glue package to insert parameters into an SQL query. However, there seem to be two distinct ways to use the glue package to insert parameters:

  1. Method 1 involves using ? in the sql query where the parameters need to be inserted, and then subsequently using dbBind to fill them in. Example from the link above:
library(glue)
library(DBI)

airport_sql <- glue_sql("SELECT * FROM airports WHERE faa = ?")
airport <- dbSendQuery(con, airport_sql)

dbBind(airport, list("GPT"))
dbFetch(airport)
  1. Method 2 involves using glue_sql or glue_data_sql to fill in the parameters by itself (no use of dbBind). Again an example from the link above:
airport_sql <- 
  glue_sql(
    "SELECT * FROM airports WHERE faa IN ({airports*})", 
    airports = c("GPT", "MSY"),
    .con = con
  )

airport <- dbSendQuery(con, airport_sql)

dbFetch(airport)

I would prefer using the second method because this has a lot of extra functionality such as collapsing multiple values for an in statement in the where clause of an sql statement. See second example above for how that works (note the * after the parameter which indicates it must be collapsed). The question is: is this safe against SQL injection? (Are there other things I need to worry about?)

My code

This is currently the code I have for my wrapper.

paramQueryWrapper <- function(
  sql,
  params = NULL,
  dsn    = standard_dsn,
  login  = user_login,
  pw     = user_pw
){

  if(missing(sql) || length(sql) != 1 || !is.character(sql)){
    stop("Please provide sql as a character vector of length 1.")
  }

  if(!is.null(params)){
    if(!is.list(params))       stop("params must be a (named) list (or NULL).")
    if(length(params) < 1)     stop("params must be either NULL, or contain at least one element.")
    if(is.null(names(params)) || any(names(params) == "")) stop("All elements in params must be named.")
  }

  con <- DBI::dbConnect(
    odbc::odbc(),
    dsn      = dsn,
    UID      = login,
    PWD      = pw
  )
  on.exit(DBI::dbDisconnect(con), add = TRUE)


  # Replace params with corresponding values and execute query

  sql <- glue::glue_data_sql(.x = params, sql, .con = con)

  query <- DBI::dbSendQuery(conn = con, sql)
  on.exit(DBI::dbClearResult(query), add = TRUE, after = FALSE)

  return(tibble::as_tibble(DBI::dbFetch(query)))

}

My question

Is this safe against SQL injection? Especially since I am not using dbBind.

Epilogue

I know that there already exists a wrapper called dbGetQuery that allows parameters (see this question for more info - look for the answer by @krlmlr for an example with a parameterized query). But this again relies on the first method using ? which is much more basic in terms of functionality.

Willem
  • 976
  • 9
  • 24
  • I see SQL injection mentioned in the `DBI` documentation: https://cran.r-project.org/web/packages/DBI/DBI.pdf – MichaelChirico May 16 '19 at 09:26
  • `glue_sql` documentation also mentions injection (on top of `dbBind`): https://glue.tidyverse.org/reference/glue_sql.html – MichaelChirico May 16 '19 at 09:27
  • @MichaelChirico 1. About the `DBI` documentation. This mentions SQL injection in several functions amongst which `dbBind`. But as I have mentioned in my answer this function does not allow named parameters for an SQL server database. So unless someone can explain to me how to use `dbBind` with named parameters on sql server this is irrelevant. 2. About the documentation on `glue_sql`. This literally only says parameterized queries are safest, implying that use of `dbBind` is better. But it doesn't mention anything about the safety of using `glue_sql` by itself. Again not answering my question. – Willem May 16 '19 at 10:22
  • Looking at [this tweet](https://twitter.com/klmr/status/913742759844040704), it seems that `glue_sql` in principle is not safe against SQL injection – starja Apr 15 '21 at 19:49

0 Answers0