 |
|
Item460: Remove use of dangerous untaintUnchecked function
Priority: Normal
Current State: No Action Required
Released In:
Target Release:
Current State: No Action Required
Released In:
Target Release:
Applies To: Engine
Component:
Branches:
Component:
Branches:
Additional details
When running in taint mode, Perl marks variables as tainted if they have been assigned values that have derived from external sources, such as command line arguments, environment variables, system calls that read from the file system or shared memory, and all file input. See the perlsec man page for a more complete list. If the variable is subsequently used as part of a command given to the OS to execute (e.g. within backquotes, or as an argument to thesystem command), or in a Perl function that interacts with the filesystem or processes (e.g. open, kill), then Perl will throw a fatal error exception and not execute the command.
As KennethLavrsen pointed out, all variables with information that came from external sources should be handled with great care, especially when doing something potentially harmful (using qx{}, opening a file, using system even in Sandbox, etc...). For instance, code injection vulnerabilities can occur if a tainted variable is not properly untainted. One should validate data from external sources before using it in system commands or storing/modifying the data in the program's persistent store. Taint mode provides a safety net to automatically catch some common problems. One shouldn't rely on it to catch these kind of things (for example, it won't keep you from storing tainted data into a database, which will not be tainted when it is subsequently retrieved), but it's still best to use it as a fall-back security measure.
To untaint a variable, pass it though a regexp that checks that the variable only holds valid data, using a subexpression match, and extract the subexpression match from the regexp. The subexpression match will be untainted. What untaintUnchecked does is pass its argument through a /(.*)/ regexp, which doesn't really test anything. The "proper" way is to provide real regexp to test real input values, and use these instead.
A simple example for validating a filename (including path):
my $filepath = CWD::getcwd(); # Tainted on most systems $filepath .= "myfile.txt"; $filepath =~ m#^([\w/\\ ]+)$#; # Contains letters, /, \, or space. It's a bit of a naive one, but just to make an example $filepath = $1; # Untaint it
Summary of uses of untaintUnchecked
The table below summarizes the work done and left to do:-
: the code has been fixed
-
: someone is working on it
-
: the code is still there and nobody it taking care of it
-
: no validation currently performed
-
: incomplete validation performed
-
: tainted data is a filename read from a Foswiki directory so is problematic if no validation is performed when creating the file
-
: tainted data is from CGI session so is problematic if no validation is performed when the data is stored
grep -rn untaintUnchecked *|perl -nle 'if(/^([^:]*):(\d+):\s+(.*)$/){if($o&&$o eq$1){$f="^"}else{$o=$f=$1};print "| $f | $2 | $3 | %N% |"}'
I don't think you fully understand the purpose of the untainUnchecked function. It was added specifically because it shouts at you "I am dangerous". The idea was that calls to it would provide a way that untaints could be tracked quickly and effectively in the code - as you have showed above, by easily finding all the calls. We must of course aggressively audit how the data passed to the calls is checked. But the point of the function was not to replace or nullify these checks, just to provide a search placeholder for them and discourage the bad practice of "manual untainting". I think it would be a backward step to remove this function; though auditing the calls to ensure the data being untainted is safe is something I support wholeheartedly. -- Main.CrawfordCurrie - 15 Dec 2008 I was under the impression that Olivier was suggesting that calls to untaintUnchecked() be replaced with proper untainting, using helper functions that serve the required purpose within the context of each instance (which is why I started to categorize the purpose of each untainting). I don't think he wanted to just remove it and replace it with inline unsafe untainting. -- Main.IsaacLin - 15 Dec 2008 Yes Isaac, that was exactly my point. My idea was that untaintUnchecked was good, at least one could spot them easily, but the idea was to go one step further and validate that, for example, a path was a path, and not some random potentially unsafe garbage. I haven't started ported some checks, as some are a bit hard to code to keep them portable but quick. I just opened this task as a reminder that this should be done, so that people like Isaac could help, and in fact, you Crawford already did most of the work with Item528 -- Main.OlivierRaginel - 21 Dec 2008 We should for sure not remove the untaintUnchecked. We can change the code but we cannot change people. People will instead untaint using regex and $1 and that is harder to spot. We cannot remove untainting unconditionally because it is a one liner standard perl to do it. At least with the Sandbox untaintUnchecked we have a chance to spot them. Another thing is that Func, Meta and Sandbox functions are part of our published API so extension developers will have used this function. We cannot break plugins by changing API. And in most cases the untaintUnchecked is OK because often the taint is not relevant unless you run the script from command line and have file access. I have changed the title by adding "use of" so people do not misunderstand what we actually did in reaction to the original report. The SVN checkin in this report is a mistake - probably a mistyped item number in amn SVN checkin. -- Main.KennethLavrsen - 03 Jan 2009 I went through and converted all unchecked untaints to checked untaints in the core. There may be some left, in which case this report can be reopened. For now, no further action is required. -- Main.CrawfordCurrie - 04 Jan 2009
ItemTemplate edit
| Summary | Remove use of dangerous untaintUnchecked function |
| ReportedBy | Foswiki:Main.OlivierRaginel |
| Codebase | trunk |
| SVN Range | TWiki-4.2.3, Wed, 06 Aug 2008, build 17396 |
| AppliesTo | Engine |
| Component | |
| Priority | Normal |
| CurrentState | No Action Required |
| WaitingFor | |
| Checkins | distro:dfb018e2baf9 |
| ReleasedIn |
Edit | Attach | Print version | History: r17 < r16 < r15 < r14 | Backlinks | View wiki text | Edit wiki text | More topic actions
Topic revision: r17 - 04 Jan 2009, CrawfordCurrie
The copyright of the content on this website is held by the contributing authors, except where stated elsewhere. See Copyright Statement. 
Legal Imprint Privacy Policy
Legal Imprint Privacy Policy