Forum Discussion

Sam_Hall's avatar
Sam_Hall
Icon for Nimbostratus rankNimbostratus
Dec 04, 2013

Serving any iFiles: sanitising user input

Here I'm developing a global iRule to be applied to all my websites. During an outage, the iRule will serve up an outage page along with any additional resources that may have been included and stored as ifiles.

So the URI to access any ifile will be of the format /ifile?filename.ext. I'm currently just using HTTP::query virtually as is without any sanity checks and passing it to "ifile get". Is this safe?

If not, any recommendations on how best to confirm it's not tainted?

when HTTP_REQUEST priority 100 {
  if { [active_members [LB::server pool]] == 0 } {

     Allow for additional resources to be served
    if { [HTTP::path] eq "/ifile" } {

       iFiles served in this way must be stored lowercase
      set qrystr [string tolower [HTTP::query]]

       determine mimetype based on file extension
      switch -glob $qrystr {
        "*.jpg" { set mimetype "image/jpeg" }
        "*.png" { set mimetype "image/png" }
        "*.js"  { set mimetype "text/javascript" }
        "*.css" { set mimetype "text/css" }
        "*.txt" { set mimetype "text/plain" }
        default { set mimetype "text/html" }
      }

       Attempt to serve up the iFile if it exists
      if { [catch { HTTP::respond 200 content [ifile get $qrystr] "Content-Type" $mimetype "Connection" "Close" } ] } {
        HTTP::respond 404 content "404: iFile Not Found" "Content-Type" "text/plain" "Connection" "Close"
      }
    } else {
       Extract lowercase pool name removing path details
      set pool_shortname [string tolower [lindex [split [LB::server pool] "/"] end]]
      set pool_outage_iFile ${pool_shortname}_outage.html

       Serve up pool specific outage page if it exists
      if { [catch { HTTP::respond 200 content [ifile get $pool_outage_iFile] "Content-Type" "text/html" "Connection" "Close" } ] } {
         Otherwise use the default outage page
        HTTP::respond 200 content [ifile get system_outage.html] "Content-Type" "text/html" "Connection" "Close"
      }
    }

     Don't fire any more HTTP_REQUEST iRule events
     This takes effect for the duration of the connection, so we need to ensure the connection is closed after the response
    event disable
  }
}

when HTTP_RESPONSE priority 100 {
  if { [active_members [LB::server pool]] == 0 } {
     During outage, close connection to prevent unhandled HTTP_REQUEST events
    HTTP::close
  }
}

4 Replies

  • Maybe check the file name represents an ifile early. Then, as long as your ifile names are "sane", you'll have no issues.

    if { [lsearch -exact [ifile listall] "$qrystr"] }
       ...
    
    • Sam_Hall's avatar
      Sam_Hall
      Icon for Nimbostratus rankNimbostratus
      Thanks, certainly looks safer and it's a cleaner option than using the catch command. I'm happy with this solution since we have full control over the ifile names, and I assume there's not much risk of TCL interpreting them as anything other than strings anyway. I'm relatively new to TCL and started to worry that TCL injection might be a possibility. A quick search turned up only a couple options for sanity checking, either using regex (which is apparently inefficient) or using scan (which seemed pretty limited).
  • uni's avatar
    uni
    Icon for Altostratus rankAltostratus

    Maybe check the file name represents an ifile early. Then, as long as your ifile names are "sane", you'll have no issues.

    if { [lsearch -exact [ifile listall] "$qrystr"] }
       ...
    
    • Sam_Hall's avatar
      Sam_Hall
      Icon for Nimbostratus rankNimbostratus
      Thanks, certainly looks safer and it's a cleaner option than using the catch command. I'm happy with this solution since we have full control over the ifile names, and I assume there's not much risk of TCL interpreting them as anything other than strings anyway. I'm relatively new to TCL and started to worry that TCL injection might be a possibility. A quick search turned up only a couple options for sanity checking, either using regex (which is apparently inefficient) or using scan (which seemed pretty limited).