We launched new forums in March 2019—join us there. In a hurry for help with your website? Get Help Now!
    • 21255
    • 215 Posts
    Please have a short look on these issues with cache, SQL injection and user login. I’m currently feeling a bit nervous about them wink
      • 32241
      • 1,495 Posts
      I do notice this SQL injection on document parser, considering the input for document id is not being checked thoroughly, but I will take a look on this. Btw Timon, I think on 0.9.2, this fix will need to be applied, before we release, right?
        Wendy Novianto
        [font=Verdana]PT DJAMOER Technology Media
        [font=Verdana]Xituz Media
        • 21255
        • 215 Posts
        Right, I would really feel better if it’ll be fixed in 0.9.2 wink
          • 6726
          • 7,075 Posts
          Quote from: netnoise at Mar 18, 2006, 12:21 PM
          Right, I would really feel better if it’ll be fixed in 0.9.2 wink

          I think everybody will agree to that, are those issues assigned yet (sorry, didn’t check...) ?

          Anyway, one thing that goes in our favor is that most targeted systems are widely used, which is not the case for MODx right now... not a reason not to act, but let’s not get too nervous either...
            .: COO - Commerce Guys - Community Driven Innovation :.


            MODx est l'outil id
            • 31337
            • 258 Posts
            Netnoise,

            Do you have any proposed solutions for those issues? I was going to tackle them this afternoon, but if you have some code ready, I’d be happy to use that.

              • 32241
              • 1,495 Posts
              Hi netnoise, here is one of the thing that I think need to be added to avoid mysql injection using document id.

              	/**
              	 * name: getDocumentObject  - used by parser
              	 * desc: returns a document object - $method: alias, id
              	 */
              	function getDocumentObject($method,$identifier){
              		$tblsc = $this->getFullTableName("site_content");
              		$tbldg = $this->getFullTableName("document_groups");
              		// Check document id validity (To avoid sql injection) - Added by Wendy Novianto
              		if($method == 'id' && (!is_numeric($identifier) || $identifier != ((int)$identifier))) {
              			$this->sendErrorPage();
              			exit; // stop here
              		}
              		// get document groups for current user
              		if($docgrp = $this->getUserDocGroups()) $docgrp = implode(",",$docgrp);
              		// get document
              		$access = ($this->isFrontend() ? "sc.privateweb=0":"1='".$_SESSION['mgrRole']."' OR sc.privatemgr=0").
              				  (!$docgrp ? "":" OR dg.document_group IN ($docgrp)");
              		$sql = "SELECT sc.*
              				FROM $tblsc sc
              				LEFT JOIN $tbldg dg ON dg.document = sc.id
              				WHERE sc.".$method." = '".$identifier."'
              				AND ($access) LIMIT 1;";
              		$result = $this->db->query($sql);
              		$rowCount = $this->recordCount($result);
              		if($rowCount<1) {
              			if ($this->config['unauthorized_page']) {
              				// check if file is not public
              				$secrs = $this->dbQuery("SELECT id FROM $tbldg WHERE document = '".$identifier."' LIMIT 1;");
              				if($secrs) $seclimit = mysql_num_rows($secrs);
              			}
              			if ($seclimit>0)  {
              				// match found but not publicly accessible, send the visitor to the unauthorized_page
              				$this->sendUnauthorizedPage();
              				exit; // stop here
              			}
              			else {
              				// no match found, send the visitor to the error_page
              				$this->sendErrorPage();
              				exit; // stop here
              			}
              		}
              		if($rowCount>1) {
              		 	// too many matches found, send the visitor to the error page
              			$this->messageQuit("More than one result returned when attempting to translate `alias` to `id` - there are multiple documents using the same alias");
              		}
              		# this is now the document :) #
              		$documentObject = $this->fetchRow($result);
              
              		// load TVs and merge with document - Orig by Apodigm - Docvars
              		$tbn = $this->dbConfig['dbase'].".".$this->dbConfig['table_prefix'];
              		$sql = "SELECT tv.*, IF(tvc.value!='',tvc.value,tv.default_text) as value ";
              		$sql.= "FROM ".$tbn."site_tmplvars tv ";
              		$sql.= "INNER JOIN ".$tbn."site_tmplvar_templates tvtpl ON tvtpl.tmplvarid = tv.id ";
              		$sql.= "LEFT JOIN ".$tbn."site_tmplvar_contentvalues tvc ON tvc.tmplvarid=tv.id AND tvc.contentid = '".$this->documentIdentifier."' ";
              		$sql.= "WHERE tvtpl.templateid = '".$documentObject['template']."'";
              		$rs = $this->dbQuery($sql);
              		$rowCount = $this->recordCount($rs);
              		if($rowCount>0) {
              			for($i=0;$i<$rowCount;$i++) {
              				$row = $this->fetchRow($rs);
              				$tmplvars[$row['name']] = array($row['name'],$row['value'],$row['display'],$row['display_params'],$row['type']);
              			}
              			$documentObject = array_merge($documentObject,$tmplvars);
              		}
              		return $documentObject;
              	}
              


              I only added 3 lines of code to make sure if document method is id, the doc id being passed needs to be integer.
              I found this problem when helping someone on the forum, hope you can apply it for next release, considering I have no access to the svn. Hope you fix the other security problem as well netnoise.

              EDIT: I mistyped == to =
                Wendy Novianto
                [font=Verdana]PT DJAMOER Technology Media
                [font=Verdana]Xituz Media
                • 32241
                • 1,495 Posts
                Oh, while you’re at it, could you address this issue, on this foum topic as well timon?
                http://modxcms.com/forums/index.php/topic,3479.0.html

                Maybe it’s been fixed by the core team, but anyway, hope it helps.
                  Wendy Novianto
                  [font=Verdana]PT DJAMOER Technology Media
                  [font=Verdana]Xituz Media
                  • 18397
                  • 3,250 Posts
                  After a discussion with Victor (and his approval) I created a Security task type for the bugtracker and tagged all of netnoise’s bugs with it. That way we can easily separate bugs from security issues. Victor would like the security tasks to be hidden from visitors. What is everyone’s opinion on this?
                    • 32241
                    • 1,495 Posts
                    agree on this. Next time I’ll make sure that most security issue not being address on public forum.
                      Wendy Novianto
                      [font=Verdana]PT DJAMOER Technology Media
                      [font=Verdana]Xituz Media
                      • 18397
                      • 3,250 Posts
                      Only one problem. If I make security issues private via FlySpray’s MakePrivate button only people with Admin Access will be able to see them...