Changeset 28841

Timestamp:

2014-02-21T18:46:01+13:00 (10 years ago)

Author:

ak19

Message:

Fixing up URL encoding of cgi args so that phrase searching works again. Tested MGPP, Lucene and SQLite searching. Tested simple search, fielded search, advanced single field and multi-field as well as running a query.

Location:

main/trunk/greenstone2/runtime-src/src/recpt

Files:

• cgiutils.cpp (modified) (5 diffs)
• cgiutils.h (modified) (1 diff)
• queryaction.cpp (modified) (2 diffs)
• querytools.cpp (modified) (16 diffs)
• sqlqueryaction.cpp (modified) (2 diffs)

main/trunk/greenstone2/runtime-src/src/recpt/cgiutils.cpp

@@ -43 +43 @@
 #endif
 
+// set to false to undo security changes (url-encoding arguments)
+static bool do_safe_cgi_args = true;
 
 static unsigned short hexdigit (unsigned short c) {
@@ -336 +338 @@
 // This function encodes <>, &, ", ', / which are scripting chars or chars which can be used to
 // break out of an html/XML/javascript context.
-void safe_cgi_arg (text_t &argstr) {
+void safe_cgi_arg (const text_t &key, text_t &argstr) {
+  if(!do_safe_cgi_args) {
+    return;
+  }
+
   text_t::iterator in = argstr.begin();
   text_t out = "";
@@ -350 +356 @@
     else { // append whatever char is in *in, but as a char, not int
             //out += *in; // appends as int
-      out += " "; // append placeholder character
-      out[out.size()-1] = *in; // now set location containing placeholder to what's in *in
+      out.push_back(*in);
     }
     ++in;
@@ -359 +364 @@
   argstr += out;  
 }
+
+
+// given a list of characters (or "all") to decode, and given the string, str, where those 
+// characters are to be decoded, this method replaces any occurrences of the url-encoded 
+// variants of those characters with their actual characters in the given string str.
+void unsafe_cgi_arg(const text_t &chars, text_t &str) {
+  if(!do_safe_cgi_args) {
+    return;
+  }
+
+  text_t allchars = "<>&\"\'/";
+
+  text_t chars_to_decode = (chars == "all" || chars == "ALL") ? allchars : chars;
+
+  text_t::iterator in = chars_to_decode.begin();
+  text_t::iterator end = chars_to_decode.end();
+
+  char hex_char[4];
+
+  // using sprint to urlencode a character. See http://www.programmingforums.org/thread15443.html
+
+  while (in != end) { 
+    
+    // *in is a character from the accepted list of chars_to_decode list
+    
+    // 1. create the url-encoded value of the char *in in variable hex_char
+    // sprintf adds in a null byte at the end
+    sprintf(hex_char,"%%%02X",*in);
+    
+    // 2. Need the actual char to be decoded as a text_t string, so we can do a string replace with it
+    text_t tmp = "";
+    tmp.push_back(*in);
+    
+    // 3. replaces occurrences of hex_char (the url_encoded version of the char *in) in str with its decoded version    
+    str.replace(hex_char, tmp);
+
+    ++in;
+  }  
+}
+
 
 // split up the cgi arguments
@@ -378 +423 @@
     decode_cgi_arg (value);
 
-    safe_cgi_arg(value); // mitigate obvious cross-site scripting hacks in URL cgi-params
+    safe_cgi_arg(key, value); // mitigate obvious cross-site scripting hacks in URL cgi-params
 
     value.setencoding(1); // other encoding

main/trunk/greenstone2/runtime-src/src/recpt/cgiutils.h

@@ -42 +42 @@
 void split_cgi_args (const cgiargsinfoclass &argsinfo, text_t argstr, 
              cgiargsclass &args);
+
+// url-decode selected chars of a given string
+void unsafe_cgi_arg(const text_t &chars_to_decode, text_t &str);
 
 text_t encode_commas (const text_t &intext);

main/trunk/greenstone2/runtime-src/src/recpt/queryaction.cpp

@@ -1342 +1342 @@
     formattedstring = args["q"];
     // remove & | ! for simple search,do segmentation if necessary
+    // To url-decode the '&', format_querystring() will call unsafe_cgi_arg() first
     format_querystring (formattedstring, args.getintarg("b"), segment);
     if (args["ct"]!=0) { // mgpp and lucene - need to add in tag info if appropriate
@@ -1358 +1359 @@
     if (args["b"]=="1" && args["fqa"]=="1") { // explicit query
       formattedstring = args["q"];
+
+      // Replace %22 and %26 with " and & respectively, since these characters have meaning 
+      // in queries: " are used in phrases and & is used in boolean advanced searches.
+      // For form searches below, unsafe_cgi_arg is called in the parse_..._form() functions
+
+      unsafe_cgi_arg("ALL", formattedstring);
     }
     else { // form search
       if (args["b"]=="0") { // regular form
-    parse_reg_query_form(formattedstring, args, segment);
+    parse_reg_query_form(formattedstring, args, segment); // will call unsafe_cgi_arg to decode url encoding
       }
       else  { // advanced form
-    parse_adv_query_form(formattedstring, args, segment);
+    parse_adv_query_form(formattedstring, args, segment); // will call unsafe_cgi_arg to decode url encoding
       }
       args["q"] = formattedstring;

main/trunk/greenstone2/runtime-src/src/recpt/querytools.cpp

@@ -25 +25 @@
 
 #include "querytools.h"
+#include "cgiutils.h"
 #include <ctype.h>
 #include "unitool.h" // for is_unicode_letdig
@@ -343 +344 @@
 // This function removes boolean operators from simple searches, and segments
 // chinese characters if segment=true
+// Called by several parse_..._form methods here, this function decodes &
+// to undo the URL encoding done in cgiutils.cpp for security purposes
 void format_querystring (text_t &querystring, int querymode, bool segment) {
   text_t formattedstring;
+
+  // & has meaning in boolean searches and can be %26 encoded at this point, need to decode them now. 
+  // Also decode any " here, so that the entire search phrase is highlighted and not just the final word
+  unsafe_cgi_arg("ALL", querystring);
 
   // advanced search, no segmenting, don't need to do anything
@@ -449 +456 @@
   }
   
-  
   if (arg_ct == "2") { // lucene
     // look for AND OR NOT and remove
@@ -579 +585 @@
 
 
+// The following parse_..._form functions first decode various fields for 
+// both simple and advanced searches to undo the URL encoding. 
+// E.g. quotes have meaning in phrase searches and these have to be decoded 
+// before sending the search off to the index.
+
 // some query form parsing functions for use with mgpp & lucene
 
@@ -599 +610 @@
   text_t field = args["fqf"];
   if (field.empty()) return; // no query
+  unsafe_cgi_arg("ALL", field);
   text_tarray fields;
   splitchar(field.begin(), field.end(), ',', fields); 
@@ -604 +616 @@
   text_t value = args["fqv"];
   if (value.empty()) return; // somethings wrong
+  unsafe_cgi_arg("ALL", value);
   text_tarray values;
   splitchar(value.begin(), value.end(), ',', values);
@@ -651 +664 @@
   text_t field = args["fqf"];
   if (field.empty()) return; // no query
+  unsafe_cgi_arg("ALL", field);
   text_tarray fields;
   splitchar(field.begin(), field.end(), ',', fields); 
@@ -656 +670 @@
   text_t value = args["fqv"];
   if (value.empty()) return; // somethings wrong
+  unsafe_cgi_arg("ALL", value);
   text_tarray values;
   splitchar(value.begin(), value.end(), ',', values);
@@ -661 +676 @@
   text_t comb = args["fqc"];
   if (comb.empty()) return; //somethings wrong
+  //unsafe_cgi_arg("ALL", comb);
   text_tarray combs;
   splitchar(comb.begin(), comb.end(), ',', combs);
@@ -734 +750 @@
   text_t field = args["sqlfqf"];
   if (field.empty()) return; // no query
+  unsafe_cgi_arg("ALL", field); // for the slash. //unsafe_cgi_arg("/", field);
   text_tarray fields;
   splitchar(field.begin(), field.end(), ',', fields); 
@@ -739 +756 @@
   text_t sqlcomb = args["sqlfqc"];
   if (sqlcomb.empty()) return; //somethings wrong
+  //unsafe_cgi_arg("ALL", sqlcomb);
   text_tarray sqlcombs;
   splitchar(sqlcomb.begin(), sqlcomb.end(), ',', sqlcombs);
@@ -744 +762 @@
   text_t value = args["fqv"];
   if (value.empty()) return; // somethings wrong
+  unsafe_cgi_arg("ALL", value);
   text_tarray values;
   splitchar(value.begin(), value.end(), ',', values);
@@ -808 +827 @@
 
   if (field.empty()) return; // no query
+  // need to decode %2F to / in the URL, e.g. to get dc.Title/Title/ex.Title again in the fields to search in
+  unsafe_cgi_arg("ALL", field); //unsafe_cgi_arg("/", field);
   text_tarray fields;
   splitchar(field.begin(), field.end(), ',', fields); 
@@ -813 +834 @@
   text_t sqlcomb = args["sqlfqc"];
   if (sqlcomb.empty()) return; //somethings wrong
+  //unsafe_cgi_arg("ALL", sqlcomb);
   text_tarray sqlcombs;
   splitchar(sqlcomb.begin(), sqlcomb.end(), ',', sqlcombs);
@@ -818 +840 @@
   text_t value = args["fqv"];
   if (value.empty()) return; // somethings wrong
+  unsafe_cgi_arg("ALL", value); // decode all url-encoded parts of the values to search in
   text_tarray values;
   splitchar(value.begin(), value.end(), ',', values);
@@ -823 +846 @@
   text_t comb = args["fqc"];
   if (comb.empty()) return; //somethings wrong
+  //unsafe_cgi_arg("ALL", comb);
   text_tarray combs;
   splitchar(comb.begin(), comb.end(), ',', combs);

main/trunk/greenstone2/runtime-src/src/recpt/sqlqueryaction.cpp

@@ -260 +260 @@
                           ostream& logout) 
 {
+  // A great many characters have meanings in SQL queries, including > and %, 
+  // where % stands for a multi-char wildcard 
+  // http://docs.oracle.com/cd/B10501_01/text.920/a96518/cqspcl.htm
+  // Further, Greenstone's Advanced SQLite Search allows <, >, %, ' (rounded brackets and more) 
+  // So it's best to url-decode all encoded cgi-args 
+  // We do so here if normal text search or explicit query, and in the
+  // parse_sql_query_form functions if dealing with forms.
+
   if (args["qt"]=="0" && args["sqlqto"] != "1") { // normal text search
+    unsafe_cgi_arg("ALL", args["q"]);
     formattedstring = "SELECT DISTINCT docOID FROM document_metadata WHERE " + args["q"];    
   }
@@ -267 +276 @@
     if (args["b"]=="1" && args["fqa"]=="1") { // explicit query
       formattedstring = args["q"];
+      unsafe_cgi_arg("ALL", formattedstring);
     }
     else { // form search