Fix a critical security vulnerability where unsanitized user-modifiab…

…le values could be included in a command line template.
blankenberg · Jan 13, 2015 · 50d65f4 · 50d65f4
1 parent 65ef16f
commit 50d65f4
Show file tree

Hide file tree

Showing 6 changed files with 515 additions and 23 deletions.
diff --git a/lib/galaxy/datatypes/metadata.py b/lib/galaxy/datatypes/metadata.py
@@ -20,6 +20,7 @@
 
 import galaxy.model
 from galaxy.util import listify
+from galaxy.util.object_wrapper import sanitize_lists_to_string
 from galaxy.util import stringify_dictionary_keys
 from galaxy.util import string_as_bool
 from galaxy.util import in_directory
@@ -232,6 +233,9 @@ def get_html( self, value, context=None, other_values=None, **kwd ):
     def to_string( self, value ):
         return str( value )
 
+    def to_safe_string( self, value ):
+        return sanitize_lists_to_string( self.to_string( value ) )
+
     def make_copy( self, value, target_context = None, source_context = None ):
         return copy.deepcopy( value )
 
@@ -480,6 +484,10 @@ class DictParameter( MetadataParameter ):
     def to_string( self, value ):
         return  json.dumps( value )
 
+    def to_safe_string( self, value ):
+        # We do not sanitize json dicts
+        return json.safe_dumps( value )
+
 
 class PythonObjectParameter( MetadataParameter ):
 
@@ -510,6 +518,10 @@ def to_string( self, value ):
             return str( self.spec.no_value )
         return value.file_name
 
+    def to_safe_string( self, value ):
+        # We do not sanitize file names
+        return self.to_string( value )
+
     def get_html_field( self, value=None, context=None, other_values=None, **kwd ):
         context = context or {}
         other_values = other_values or {}

diff --git a/lib/galaxy/tools/evaluation.py b/lib/galaxy/tools/evaluation.py
@@ -2,10 +2,12 @@
 import tempfile
 
 from galaxy import model
+from galaxy.util.object_wrapper import wrap_with_safe_string
 from galaxy.util.bunch import Bunch
 from galaxy.util.none_like import NoneDataset
 from galaxy.util.template import fill_template
 from galaxy.tools.wrappers import (
+    ToolParameterValueWrapper,
     DatasetFilenameWrapper,
     DatasetListWrapper,
     DatasetCollectionWrapper,
@@ -114,6 +116,9 @@ def build_param_dict( self, incoming, input_datasets, output_datasets, output_pa
         self.__populate_input_dataset_wrappers(param_dict, input_datasets, input_dataset_paths)
         self.__populate_output_dataset_wrappers(param_dict, output_datasets, output_paths, job_working_directory)
         self.__populate_unstructured_path_rewrites(param_dict)
+        # Call param dict sanitizer, before non-job params are added, as we don't want to sanitize filenames.
+        self.__sanitize_param_dict( param_dict )
+        # Parameters added after this line are not sanitized
         self.__populate_non_job_params(param_dict)
 
         # Return the dictionary of parameters
@@ -334,6 +339,24 @@ def rewrite_unstructured_paths( input_values, input ):
             #the paths rewritten.
             self.__walk_inputs( self.tool.inputs, param_dict, rewrite_unstructured_paths )
 
+    def __sanitize_param_dict( self, param_dict ):
+        """
+        Sanitize all values that will be substituted on the command line, with the exception of ToolParameterValueWrappers,
+        which already have their own specific sanitization rules and also exclude special-cased named values.
+        We will only examine the first level for values to skip; the wrapping function will recurse as necessary.
+        
+        Note: this method follows the style of the similar populate calls, in that param_dict is modified in-place.
+        """
+        # chromInfo is a filename, do not sanitize it.
+        skip = [ 'chromInfo' ]
+        if not self.tool or not self.tool.options or self.tool.options.sanitize:
+            for key, value in param_dict.items():
+                if key not in skip:
+                    # Remove key so that new wrapped object will occupy key slot
+                    del param_dict[key]
+                    # And replace with new wrapped key
+                    param_dict[ wrap_with_safe_string( key, no_wrap_classes=ToolParameterValueWrapper ) ] = wrap_with_safe_string( value, no_wrap_classes=ToolParameterValueWrapper )
+
     def build( self ):
         """
         Build runtime description of job to execute, evaluate command and

diff --git a/lib/galaxy/tools/wrappers.py b/lib/galaxy/tools/wrappers.py
@@ -2,6 +2,7 @@
 from galaxy import exceptions
 from galaxy.util.none_like import NoneDataset
 from galaxy.util import odict
+from galaxy.util.object_wrapper import wrap_with_safe_string
 
 from logging import getLogger
 log = getLogger( __name__ )
@@ -162,10 +163,13 @@ def __getattr__( self, name ):
             if name in self.metadata.spec:
                 if rval is None:
                     rval = self.metadata.spec[name].no_value
-                rval = self.metadata.spec[name].param.to_string( rval )
+                rval = self.metadata.spec[ name ].param.to_safe_string( rval )
                 # Store this value, so we don't need to recalculate if needed
                 # again
                 setattr( self, name, rval )
+            else:
+                #escape string value of non-defined metadata value
+                rval = wrap_with_safe_string( rval )
             return rval
 
         def __nonzero__( self ):
@@ -190,9 +194,13 @@ def __init__( self, dataset, datatypes_registry=None, tool=None, name=None, data
                 ext = tool.inputs[name].extensions[0]
             except:
                 ext = 'data'
-            self.dataset = NoneDataset( datatypes_registry=datatypes_registry, ext=ext )
+            self.dataset = wrap_with_safe_string( NoneDataset( datatypes_registry=datatypes_registry, ext=ext ), no_wrap_classes=ToolParameterValueWrapper )
         else:
-            self.dataset = dataset
+            # Tool wrappers should not normally be accessing .dataset directly, 
+            # so we will wrap it and keep the original around for file paths
+            # Should we name this .value to maintain consistency with most other ToolParameterValueWrapper?
+            self.unsanitized = dataset
+            self.dataset = wrap_with_safe_string( dataset, no_wrap_classes=ToolParameterValueWrapper )
             self.metadata = self.MetadataWrapper( dataset.metadata )
         self.datatypes_registry = datatypes_registry
         self.false_path = getattr( dataset_path, "false_path", None )
@@ -210,7 +218,7 @@ def __str__( self ):
         if self.false_path is not None:
             return self.false_path
         else:
-            return self.dataset.file_name
+            return self.unsanitized.file_name
 
     def __getattr__( self, key ):
         if self.false_path is not None and key == 'file_name':
@@ -230,7 +238,7 @@ def __getattr__( self, key ):
                 # object store to find the static location of this
                 # directory.
                 try:
-                    return self.dataset.extra_files_path
+                    return self.unsanitized.extra_files_path
                 except exceptions.ObjectNotFound:
                     # NestedObjectstore raises an error here
                     # instead of just returning a non-existent

diff --git a/lib/galaxy/util/__init__.py b/lib/galaxy/util/__init__.py
@@ -360,46 +360,57 @@ def pretty_print_json(json_data, is_json_string=False):
                  '#': '__pd__'}
 
 
-def restore_text(text):
+def restore_text( text, character_map=mapped_chars ):
     """Restores sanitized text"""
     if not text:
         return text
-    for key, value in mapped_chars.items():
+    for key, value in character_map.items():
         text = text.replace(value, key)
     return text
 
 
-def sanitize_text(text):
+def sanitize_text( text, valid_characters=valid_chars, character_map=mapped_chars, invalid_character='X' ):
     """
     Restricts the characters that are allowed in text; accepts both strings
-    and lists of strings.
+    and lists of strings; non-string entities will be cast to strings.
     """
-    if isinstance( text, basestring ):
-        return _sanitize_text_helper(text)
-    elif isinstance( text, list ):
-        return [ _sanitize_text_helper(t) for t in text ]
+    if isinstance( text, list ):
+        return map( lambda x: sanitize_text( x, valid_characters=valid_characters, character_map=character_map, invalid_character=invalid_character ), text )
+    if not isinstance( text, basestring ):
+        text = smart_str( text )
+    return _sanitize_text_helper( text, valid_characters=valid_characters, character_map=character_map )
 
-
-def _sanitize_text_helper(text):
+def _sanitize_text_helper( text, valid_characters=valid_chars, character_map=mapped_chars, invalid_character='X' ):
     """Restricts the characters that are allowed in a string"""
 
     out = []
     for c in text:
-        if c in valid_chars:
+        if c in valid_characters:
             out.append(c)
-        elif c in mapped_chars:
-            out.append(mapped_chars[c])
+        elif c in character_map:
+            out.append( character_map[c] )
         else:
-            out.append('X')  # makes debugging easier
+            out.append( invalid_character )  # makes debugging easier
     return ''.join(out)
 
 
-def sanitize_param(value):
+def sanitize_lists_to_string( values, valid_characters=valid_chars, character_map=mapped_chars, invalid_character='X'  ):
+    if isinstance( values, list ):
+        rval = []
+        for value in values:
+            rval.append( sanitize_lists_to_string( value, valid_characters=valid_characters, character_map=character_map, invalid_character=invalid_character ) )
+        values = ",".join( rval )
+    else:
+        values = sanitize_text( values, valid_characters=valid_characters, character_map=character_map, invalid_character=invalid_character )
+    return values
+
+
+def sanitize_param( value, valid_characters=valid_chars, character_map=mapped_chars, invalid_character='X' ):
     """Clean incoming parameters (strings or lists)"""
     if isinstance( value, basestring ):
-        return sanitize_text(value)
+        return sanitize_text( value, valid_characters=valid_characters, character_map=character_map, invalid_character=invalid_character )
     elif isinstance( value, list ):
-        return map(sanitize_text, value)
+        return map( lambda x: sanitize_text( x, valid_characters=valid_characters, character_map=character_map, invalid_character=invalid_character ), value )
     else:
         raise Exception('Unknown parameter type (%s)' % ( type( value ) ))
 

diff --git a/lib/galaxy/util/dbkeys.py b/lib/galaxy/util/dbkeys.py
@@ -4,6 +4,7 @@
 #dbkeys read from disk using builds.txt
 from galaxy.util import read_dbnames
 from galaxy.util.json import loads
+from galaxy.util.object_wrapper import sanitize_lists_to_string
 import os.path
 
 
@@ -84,6 +85,7 @@ def get_chrom_info( self, dbkey, trans=None, custom_build_hack_get_len_from_fast
         # use configured server len path
         if not chrom_info:
             # Default to built-in build.
-            chrom_info = os.path.join( self._static_chrom_info_path, "%s.len" % dbkey )
+            # Since we are using an unverified dbkey, we will sanitize the dbkey before use
+            chrom_info = os.path.join( self._static_chrom_info_path, "%s.len" % sanitize_lists_to_string( dbkey ) )
         chrom_info = os.path.abspath( chrom_info )
         return ( chrom_info, db_dataset )