rejetto forum

BUG because of the successful upload of hfs.diff.tpl

Mars · 7 · 1796

0 Members and 1 Guest are viewing this topic.

Offline Mars

  • Operator
  • Tireless poster
  • *****
    • Posts: 2038
    • View Profile
Quote
 function complyUploadFilter():boolean;    //mod by mars  uploadFailed was not initialized with DIFF_TPL_FILE
  var
    s: string;
  begin
  result:=FALSE;
  if ansiContainsText(data.uploadSrc, DIFF_TPL_FILE) then
    begin
    data.uploadFailed:='File name forbidden';
    exit;
    end;
  if f.isTemp() then s:=f.parent.uploadFilterMask
  else s:=f.uploadFilterMask;
  if s = '' then s:='\hfs.*;index.htm*;default.htm*;descript.ion;*.md5'; // the user can disable this default filter by inputing * as mask
  result:=fileMatch(s, data.uploadSrc);
  if result then exit;
  data.uploadFailed:='File name or extension forbidden';
  end; // complyUploadFilter
« Last Edit: February 19, 2010, 11:39:41 PM by Mars »


Offline rejetto

  • Administrator
  • Tireless poster
  • *****
    • Posts: 13308
    • View Profile
hfs.diff.tpl should be matched by hfs.* in the default mask.
maybe you specified a different mask for that folder?


Offline Mars

  • Operator
  • Tireless poster
  • *****
    • Posts: 2038
    • View Profile
Quote
hfs.diff.tpl should be matched by hfs.* in the default mask.
maybe you specified a different mask for that folder?

original code of hfs is

result:=FALSE;
  if ansiContainsText(data.uploadSrc, DIFF_TPL_FILE) then exit;
  if f.isTemp() then s:=f.parent.uploadFilterMask
 
in this case all files matched   *hfs.diff.tpl* are are exread by the upload, BUT  the obtained answer is: transferred file as in the image before.png

make test with upload mask file as *.*
and upload a hfs.diff.tpl file to the real folder

what append for you?? ;D

forbid all upload of files as hfs.diff.tpl is a good security, don't delete it , just inform the user that it is a secure file of hfs;)
« Last Edit: February 20, 2010, 06:34:04 PM by Mars »


Offline rejetto

  • Administrator
  • Tireless poster
  • *****
    • Posts: 13308
    • View Profile
yes, the diff tpl is much more dangerous than other masked files.

i changed "ansiContainsText" in "sameText". silly error. ;)
i changed it to this
Code: [Select]
  function complyUploadFilter():boolean;

    function getMask():string;
    begin
    if f.isTemp() then result:=f.parent.uploadFilterMask
    else result:=f.uploadFilterMask;
    if result = '' then
      result:='\hfs.*;index.htm*;default.htm*;descript.ion;*.md5'; // the user can disable this default filter by inputing * as mask
    end;

  begin
  result:=not sameText(data.uploadSrc, DIFF_TPL_FILE)
    and fileMatch(getMask(), data.uploadSrc);
  if not result then
    data.uploadFailed:='File name or extension forbidden';
  end; // complyUploadFilter
« Last Edit: February 20, 2010, 07:55:20 PM by rejetto »


Offline Mars

  • Operator
  • Tireless poster
  • *****
    • Posts: 2038
    • View Profile
In some templates is offered to the user the possibility to rename a file as for example xxx.txt  to hfs.diff.tpl


  
Quote
 if name = 'rename' then
      begin
     if ( filename(p) <> DIFF_TPL_FILE) and (filename(par(1)) <> DIFF_TPL_FILE) then
        spaceIf(renameFile(p, par(1)))
      else result:='';

      if (result > '') and not stopOnMacroRename then // should we stop recursion?
        try
          // by default, we'll stop after first stacked [on macro rename], but recursive=1 will remove this limit
          stopOnMacroRename:=isFalse(par('recursive'));
          runEventScript('on macro rename', toSA(['%old-name%',p,'%new-name%',par(1)]));
        finally
          stopOnMacroRename:=FALSE;
          end;
      end;

then it is possible to upload one file with our template and rename it to hfs.diff.tpl.

Silentpliz was my victim to make this test, I was a gentile not to put him a template with one macro exec in the main section ;)



same bug security with macros:

    if name = 'move' then
      spaceIf(movefile(p, par(1)));

    if name = 'copy' then
      spaceIf(copyfile(p, par(1)));

    if (name = 'save') or (name = 'append') then
      save();

    if name = 'delete' then
      if isTrue(par('bin',TRUE,'1')) then
        spaceIf(moveToBin(p, isTrue(par('forced'))))
      else
        spaceIf(deltree(p));

 >:( :o :-[ :'(
« Last Edit: February 22, 2010, 12:20:00 AM by Mars »


Offline rejetto

  • Administrator
  • Tireless poster
  • *****
    • Posts: 13308
    • View Profile
very important point.

my opinion is that  we should focus on the script-side security (a safe script instead of a safe scripting engine).
i give two reasons for this:

1. total prevention is hard or impossible to achieve: i can use {.save.} to create a batch file with "ren my hfs.diff.tpl", and the {.exec.}
even if you would want to detect the string, i can split it in two parts, and use {.append.}
so, on this side, it seems to be a battle already lost.

2. being able to manipulate hfs.diff.tpl can be an interesting opportunity for useful features in scripts.


that's why i think this way, the problem is giving such power to the wrong user.
working on this side, we may try to mark unsafe scripts (on the forum, or in a repository), or alerting about the intended audiance (admin, occasional surfers, etc), or giving commands to ease the making of safe scripts.


Offline SilentPliz

  • Operator
  • Tireless poster
  • *****
    • Posts: 1298
  • ....... chut ! shh!
    • View Profile

i changed "ansiContainsText" in "sameText". silly error. ;)



Choosing Semtex would have been more explosive ! ;D

I go outside ... ;)
« Last Edit: February 22, 2010, 01:18:45 PM by SilentPliz »