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]
+
}