Index: openacs-4/packages/acs-tcl/tcl/security-procs.tcl =================================================================== RCS file: /usr/local/cvsroot/openacs-4/packages/acs-tcl/tcl/security-procs.tcl,v diff -u -r1.126.2.72 -r1.126.2.73 --- openacs-4/packages/acs-tcl/tcl/security-procs.tcl 25 Aug 2022 12:37:13 -0000 1.126.2.72 +++ openacs-4/packages/acs-tcl/tcl/security-procs.tcl 25 Aug 2022 15:37:01 -0000 1.126.2.73 @@ -1174,6 +1174,8 @@ ad_proc security::safe_tmpfile_p { -must_exist:boolean + -recursive:boolean + -subsite_id tmpfile } { @@ -1186,14 +1188,51 @@ @param tmpfile absolute path to a possibly existing tmpfile @param must_exist make sure the file exists + @param recursive accept also files in a subfolder of a valid + tmpfolder + @param subsite_id when specified, the list of allowed tmpdirs will + be taken from the TmpDir subsite + parameter. Server-wide configuration will be + used if no subsite is specified or if the + parameter turns out to be empty. @return boolean } { - if {[ad_file dir $tmpfile] ne [ns_config ns/parameters tmpdir]} { + # + # Ensure no ".." in the path + # + set tmpfile [ns_normalizepath $tmpfile] + + if {[info exists subsite_id]} { # - # File does not belong to the tmpdir: not safe + # We fetch the tmpdirs from the subsite parameter # + set tmpdirs [parameter::get -package_id $subsite_id -parameter TmpDir] + } else { + set tmpdirs [list] + } + + if {[llength $tmpdirs] == 0} { + # + # Server-wide tmpdirs + # + set tmpdirs [ns_config ns/parameters tmpdir] + } + + if {!$recursive_p && [ad_file dirname $tmpfile] ni $tmpdirs} { + # + # File is not a direct child of one of the tmpfolders: not safe + # return false + } else { + # + # File does not belong to the hierarchy of any of the + # tmpfolders: not safe + # + set separator [file separator] + if { ![regexp ^([join $tmpdirs |])${separator}.*\$ $tmpfile] } { + return false + } } if {![ad_file exists $tmpfile]} { Index: openacs-4/packages/acs-tcl/tcl/tcl-documentation-procs.tcl =================================================================== RCS file: /usr/local/cvsroot/openacs-4/packages/acs-tcl/tcl/tcl-documentation-procs.tcl,v diff -u -r1.61.2.29 -r1.61.2.30 --- openacs-4/packages/acs-tcl/tcl/tcl-documentation-procs.tcl 23 Aug 2022 18:44:55 -0000 1.61.2.29 +++ openacs-4/packages/acs-tcl/tcl/tcl-documentation-procs.tcl 25 Aug 2022 15:37:01 -0000 1.61.2.30 @@ -1954,24 +1954,15 @@ @author Lars Pind (lars@pinds.com) @creation-date 25 July 2000 } { - # ensure no .. in the path - ns_normalizepath $value - - # check to make sure path is to an authorized directory - set tmpdir_list [ad_parameter_all_values_as_list -package_id [ad_conn subsite_id] TmpDir] - if { $tmpdir_list eq "" } { - set tmpdir_list [list [ns_config ns/parameters tmpdir] "/var/tmp" "/tmp"] + if {[security::safe_tmpfile_p \ + -recursive \ + -subsite_id [ad_conn subsite_id] \ + $value]} { + return 1 } - foreach tmpdir $tmpdir_list { - if { [string match "$tmpdir*" $value] } { - return 1 - } - } + ad_log warning "They tried to sneak in invalid tmpfile '$value'" - # Log details about this filter failing, to make it easier to debug. - ns_log Notice "ad_page_contract tmpfile filter on variable '$name' at URL '[ad_conn url]': The tmpfile given was '$value', and the list of valid directories is '$tmpdir_list'." - ad_complain [_ acs-tcl.lt_You_specified_a_path_] return 0 } Index: openacs-4/packages/acs-tcl/tcl/test/security-procs.tcl =================================================================== RCS file: /usr/local/cvsroot/openacs-4/packages/acs-tcl/tcl/test/security-procs.tcl,v diff -u -r1.1.2.6 -r1.1.2.7 --- openacs-4/packages/acs-tcl/tcl/test/security-procs.tcl 25 Aug 2022 12:37:14 -0000 1.1.2.6 +++ openacs-4/packages/acs-tcl/tcl/test/security-procs.tcl 25 Aug 2022 15:37:02 -0000 1.1.2.7 @@ -120,16 +120,39 @@ aa_true "An existing tmpfile is safe" [security::safe_tmpfile_p -must_exist $tmpfile] file delete -- $tmpfile + aa_section {Path to an existing file in a tmpdir subfolder} + set tmpdir [ad_tmpnam] + file mkdir $tmpdir + set tmpfile $tmpdir/onefile + set wfd [open $tmpfile w] + puts $wfd 1234 + close $wfd + aa_false "File is not considered safe when not searching recursively" \ + [security::safe_tmpfile_p -must_exist $tmpfile] + aa_true "File is considered safe when searching recursively" \ + [security::safe_tmpfile_p -recursive -must_exist $tmpfile] + file delete -force -- $tmpdir + aa_section {Path to a tmpfile in a folder of the tmpdir} set tmpfile [ad_tmpnam]/test aa_false "A safe tmpfile can only be a direct child of the tmpdir" \ [security::safe_tmpfile_p $tmpfile] + aa_section {Path to a tmpfile in a folder of the tmpdir when we allow recursive paths} + set tmpfile [ad_tmpnam]/test + aa_true "A safe tmpfile can be a at any depth in the hierachy of a tmpdir" \ + [security::safe_tmpfile_p -recursive $tmpfile] + aa_section {Trying to confuse the proc with ".."} - set tmpfile [ad_tmpnam]/../test + set tmpfile [ad_tmpnam]/../../test aa_false "Proc is not fooled by .." \ [security::safe_tmpfile_p $tmpfile] + aa_section {Trying to confuse the proc with ".." when we allow recursive paths} + set tmpfile [ad_tmpnam]/../test + aa_true "Proc is not fooled by .." \ + [security::safe_tmpfile_p -recursive $tmpfile] + aa_section {Trying to confuse the proc with "~"} set tmpfile ~/../../test aa_false "Proc is not fooled by ~" \ @@ -139,4 +162,10 @@ set tmpfile [acs_root_dir]/mypreciouscode aa_false "A safe tmpfile can only be a direct child of the tmpdir" \ [security::safe_tmpfile_p $tmpfile] + + aa_section {Path to a file outside of the tmpdir when we allow recursive paths} + set tmpfile [acs_root_dir]/mypreciouscode + aa_false "A safe tmpfile can only be in the hierachy of the tmpdir" \ + [security::safe_tmpfile_p $tmpfile] + }