From 5f85fcd863fe261921953ea3bd1742f3e1b7cf68 Mon Sep 17 00:00:00 2001 From: ArthurHoaro Date: Thu, 11 Jun 2015 13:53:27 +0200 Subject: [PATCH] Working on shaarli/Shaarli#224 I reviewed character escaping everywhere with the following ideas: * use a single common function to escape user data: `escape` using `htmlspecialchars`. * sanitize fields in `index.php` after reading them from datastore and before sending them to templates. It means no escaping function in Twig templates. 2 reasons: * it reduces risks of security issue for future user made templates * more readable templates * sanitize user configuration fields after loading them. --- application/LinkDB.php | 5 ++ index.php | 159 ++++++++++++++++++++++------------------- tpl/daily.html | 4 +- tpl/dailyrss.html | 6 +- tpl/editlink.html | 10 +-- tpl/import.html | 4 +- tpl/linklist.html | 10 +-- tpl/loginform.html | 2 +- tpl/page.footer.html | 2 +- tpl/page.header.html | 2 +- tpl/picwall.html | 2 +- tpl/tagcloud.html | 2 +- 12 files changed, 113 insertions(+), 95 deletions(-) diff --git a/application/LinkDB.php b/application/LinkDB.php index 137f42e..0f7c5bf 100644 --- a/application/LinkDB.php +++ b/application/LinkDB.php @@ -245,6 +245,11 @@ class LinkDB implements Iterator, Countable, ArrayAccess foreach ($this->links as $link) { $this->urls[$link['url']] = $link['linkdate']; } + + // Escape links data + foreach($this->links as &$link) { + sanitizeLink($link); + } } /** diff --git a/index.php b/index.php index 5aa7116..39b01a2 100644 --- a/index.php +++ b/index.php @@ -98,7 +98,7 @@ header("Pragma: no-cache"); if (!is_writable(realpath(dirname(__FILE__)))) die('
ERROR: Shaarli does not have the right to write in its own directory.
'); // Handling of old config file which do not have the new parameters. -if (empty($GLOBALS['title'])) $GLOBALS['title']='Shared links on '.htmlspecialchars(indexUrl()); +if (empty($GLOBALS['title'])) $GLOBALS['title']='Shared links on '.escape(indexUrl()); if (empty($GLOBALS['timezone'])) $GLOBALS['timezone']=date_default_timezone_get(); if (empty($GLOBALS['redirector'])) $GLOBALS['redirector']=''; if (empty($GLOBALS['disablesessionprotection'])) $GLOBALS['disablesessionprotection']=false; @@ -111,6 +111,9 @@ if (empty($GLOBALS['titleLink'])) $GLOBALS['titleLink']='?'; if (!is_file($GLOBALS['config']['CONFIG_FILE'])) install(); require $GLOBALS['config']['CONFIG_FILE']; // Read login/password hash into $GLOBALS. +$GLOBALS['title'] = !empty($GLOBALS['title']) ? escape($GLOBALS['title']) : ''; +$GLOBALS['titleLink'] = !empty($GLOBALS['titleLink']) ? escape($GLOBALS['titleLink']) : ''; +$GLOBALS['redirector'] = !empty($GLOBALS['redirector']) ? escape($GLOBALS['redirector']) : ''; // a token depending of deployment salt, user password, and the current ip define('STAY_SIGNED_IN_TOKEN', sha1($GLOBALS['hash'].$_SERVER["REMOTE_ADDR"].$GLOBALS['salt'])); @@ -272,6 +275,17 @@ function nl2br_escaped($html) return str_replace('>','>',str_replace('<','<',nl2br($html))); } +function escape($str) { + return htmlspecialchars($str, ENT_COMPAT, 'UTF-8', false); +} + +function sanitizeLink(&$link) { + $link['url'] = escape($link['url']); // useful? + $link['title'] = escape($link['title']); + $link['description'] = escape($link['description']); + $link['tags'] = escape($link['tags']); +} + // In a string, converts URLs to clickable links. // Function inspired from http://www.php.net/manual/en/function.preg-replace.php#85722 function text2clickable($url) @@ -651,8 +665,8 @@ class pageBuilder private function initialize() { $this->tpl = new RainTPL; - $this->tpl->assign('newversion',checkUpdate()); - $this->tpl->assign('feedurl',htmlspecialchars(indexUrl())); + $this->tpl->assign('newversion',escape(checkUpdate())); + $this->tpl->assign('feedurl',escape(indexUrl())); $searchcrits=''; // Search criteria if (!empty($_GET['searchtags'])) $searchcrits.='&searchtags='.urlencode($_GET['searchtags']); elseif (!empty($_GET['searchterm'])) $searchcrits.='&searchterm='.urlencode($_GET['searchterm']); @@ -716,15 +730,15 @@ function showRSS() $nblinksToDisplay = $_GET['nb']=='all' ? count($linksToDisplay) : max($_GET['nb']+0,1) ; } - $pageaddr=htmlspecialchars(indexUrl()); + $pageaddr=escape(indexUrl()); echo ''; - echo ''.htmlspecialchars($GLOBALS['title']).''.$pageaddr.''; + echo ''.$GLOBALS['title'].''.$pageaddr.''; echo 'Shared linksen-en'.$pageaddr.''."\n\n"; if (!empty($GLOBALS['config']['PUBSUBHUB_URL'])) { echo ''; - echo ''; - echo ''; + echo ''; + echo ''; echo ''; } $i=0; @@ -734,16 +748,16 @@ function showRSS() $link = $linksToDisplay[$keys[$i]]; $guid = $pageaddr.'?'.smallHash($link['linkdate']); $rfc822date = linkdate2rfc822($link['linkdate']); - $absurl = htmlspecialchars($link['url']); + $absurl = $link['url']; if (startsWith($absurl,'?')) $absurl=$pageaddr.$absurl; // make permalink URL absolute if ($usepermalinks===true) - echo ''.htmlspecialchars($link['title']).''.$guid.''.$guid.''; + echo ''.$link['title'].''.$guid.''.$guid.''; else - echo ''.htmlspecialchars($link['title']).''.$guid.''.$absurl.''; - if (!$GLOBALS['config']['HIDE_TIMESTAMPS'] || isLoggedIn()) echo ''.htmlspecialchars($rfc822date)."\n"; + echo ''.$link['title'].''.$guid.''.$absurl.''; + if (!$GLOBALS['config']['HIDE_TIMESTAMPS'] || isLoggedIn()) echo ''.escape($rfc822date)."\n"; if ($link['tags']!='') // Adding tags to each RSS entry (as mentioned in RSS specification) { - foreach(explode(' ',$link['tags']) as $tag) { echo ''.htmlspecialchars($tag).''."\n"; } + foreach(explode(' ',$link['tags']) as $tag) { echo ''.$tag.''."\n"; } } // Add permalink in description @@ -751,10 +765,10 @@ function showRSS() // If user wants permalinks first, put the final link in description if ($usepermalinks===true) $descriptionlink = '(Link)'; if (strlen($link['description'])>0) $descriptionlink = '
'.$descriptionlink; - echo ''."\n\n"; + echo ''."\n\n"; $i++; } - echo ''; + echo ''; $cache->cache(ob_get_contents()); ob_end_flush(); @@ -779,7 +793,6 @@ function showATOM() $LINKSDB = new LinkDB(isLoggedIn() || $GLOBALS['config']['OPEN_SHAARLI']); // Read links from database (and filter private links if used it not logged in). - // Optionally filter the results: $linksToDisplay=array(); if (!empty($_GET['searchterm'])) $linksToDisplay = $LINKSDB->filterFulltext($_GET['searchterm']); @@ -792,7 +805,7 @@ function showATOM() $nblinksToDisplay = $_GET['nb']=='all' ? count($linksToDisplay) : max($_GET['nb']+0,1) ; } - $pageaddr=htmlspecialchars(indexUrl()); + $pageaddr=escape(indexUrl()); $latestDate = ''; $entries=''; $i=0; @@ -803,44 +816,44 @@ function showATOM() $guid = $pageaddr.'?'.smallHash($link['linkdate']); $iso8601date = linkdate2iso8601($link['linkdate']); $latestDate = max($latestDate,$iso8601date); - $absurl = htmlspecialchars($link['url']); + $absurl = $link['url']; if (startsWith($absurl,'?')) $absurl=$pageaddr.$absurl; // make permalink URL absolute - $entries.=''.htmlspecialchars($link['title']).''; + $entries.=''.$link['title'].''; if ($usepermalinks===true) $entries.=''.$guid.''; else $entries.=''.$guid.''; - if (!$GLOBALS['config']['HIDE_TIMESTAMPS'] || isLoggedIn()) $entries.=''.htmlspecialchars($iso8601date).''; + if (!$GLOBALS['config']['HIDE_TIMESTAMPS'] || isLoggedIn()) $entries.=''.escape($iso8601date).''; // Add permalink in description - $descriptionlink = htmlspecialchars('(Permalink)'); + $descriptionlink = '(Permalink)'; // If user wants permalinks first, put the final link in description - if ($usepermalinks===true) $descriptionlink = htmlspecialchars('(Link)'); - if (strlen($link['description'])>0) $descriptionlink = '<br>'.$descriptionlink; + if ($usepermalinks===true) $descriptionlink = '(Link)'; + if (strlen($link['description'])>0) $descriptionlink = '
'.$descriptionlink; - $entries.=''.htmlspecialchars(nl2br(keepMultipleSpaces(text2clickable(htmlspecialchars($link['description']))))).$descriptionlink."\n"; + $entries.='\n"; if ($link['tags']!='') // Adding tags to each ATOM entry (as mentioned in ATOM specification) { foreach(explode(' ',$link['tags']) as $tag) - { $entries.=''."\n"; } + { $entries.=''."\n"; } } $entries.="\n"; $i++; } $feed=''; - $feed.=''.htmlspecialchars($GLOBALS['title']).''; - if (!$GLOBALS['config']['HIDE_TIMESTAMPS'] || isLoggedIn()) $feed.=''.htmlspecialchars($latestDate).''; - $feed.=''; + $feed.=''.$GLOBALS['title'].''; + if (!$GLOBALS['config']['HIDE_TIMESTAMPS'] || isLoggedIn()) $feed.=''.escape($latestDate).''; + $feed.=''; if (!empty($GLOBALS['config']['PUBSUBHUB_URL'])) { $feed.=''; - $feed.=''; + $feed.=''; $feed.=''; } - $feed.=''.htmlspecialchars($pageaddr).''.htmlspecialchars($pageaddr).''; - $feed.=''.htmlspecialchars($pageaddr).''."\n\n"; // Yes, I know I should use a real IRI (RFC3987), but the site URL will do. + $feed.=''.$pageaddr.''.$pageaddr.''; + $feed.=''.$pageaddr.''."\n\n"; // Yes, I know I should use a real IRI (RFC3987), but the site URL will do. $feed.=$entries; - $feed.=''; + $feed.=''; echo $feed; $cache->cache(ob_get_contents()); @@ -882,18 +895,18 @@ function showDailyRSS() // Build the RSS feed. header('Content-Type: application/rss+xml; charset=utf-8'); - $pageaddr=htmlspecialchars(indexUrl()); + $pageaddr=escape(indexUrl()); echo ''; - echo 'Daily - '.htmlspecialchars($GLOBALS['title']).''.$pageaddr.''; + echo 'Daily - '.$GLOBALS['title'].''.$pageaddr.''; echo 'Daily shared linksen-en'.$pageaddr.''."\n"; foreach($days as $day=>$linkdates) // For each day. { $daydate = utf8_encode(strftime('%A %d, %B %Y',linkdate2timestamp($day.'_000000'))); // Full text date $rfc822date = linkdate2rfc822($day.'_000000'); - $absurl=htmlspecialchars(indexUrl().'?do=daily&day='.$day); // Absolute URL of the corresponding "Daily" page. - echo ''.htmlspecialchars($GLOBALS['title'].' - '.$daydate).''.$absurl.''.$absurl.''; - echo ''.htmlspecialchars($rfc822date).""; + $absurl=escape(indexUrl().'?do=daily&day='.$day); // Absolute URL of the corresponding "Daily" page. + echo ''.$GLOBALS['title'].' - '.$daydate.''.$absurl.''.$absurl.''; + echo ''.escape($rfc822date).""; // Build the HTML body of this RSS entry. $html=''; @@ -903,7 +916,7 @@ function showDailyRSS() foreach($linkdates as $linkdate) { $l = $LINKSDB[$linkdate]; - $l['formatedDescription']=nl2br(keepMultipleSpaces(text2clickable(htmlspecialchars($l['description'])))); + $l['formatedDescription']=nl2br(keepMultipleSpaces(text2clickable($l['description']))); $l['thumbnail'] = thumbnail($l['url']); $l['timestamp'] = linkdate2timestamp($l['linkdate']); if (startsWith($l['url'],'?')) $l['url']=indexUrl().$l['url']; // make permalink URL absolute @@ -917,7 +930,7 @@ function showDailyRSS() echo ''."\n\n\n"; } - echo ''; + echo ''; $cache->cache(ob_get_contents()); ob_end_flush(); @@ -929,7 +942,6 @@ function showDaily() { $LINKSDB = new LinkDB(isLoggedIn() || $GLOBALS['config']['OPEN_SHAARLI']); // Read links from database (and filter private links if used it not logged in). - $day=Date('Ymd',strtotime('-1 day')); // Yesterday, in format YYYYMMDD. if (isset($_GET['day'])) $day=$_GET['day']; @@ -948,10 +960,11 @@ function showDaily() // We pre-format some fields for proper output. foreach($linksToDisplay as $key=>$link) { + $taglist = explode(' ',$link['tags']); uasort($taglist, 'strcasecmp'); $linksToDisplay[$key]['taglist']=$taglist; - $linksToDisplay[$key]['formatedDescription']=nl2br(keepMultipleSpaces(text2clickable(htmlspecialchars($link['description'])))); + $linksToDisplay[$key]['formatedDescription']=nl2br(keepMultipleSpaces(text2clickable($link['description']))); $linksToDisplay[$key]['thumbnail'] = thumbnail($link['url']); $linksToDisplay[$key]['timestamp'] = linkdate2timestamp($link['linkdate']); } @@ -1002,7 +1015,7 @@ function renderPage() $token=''; if (ban_canLogin()) $token=getToken(); // Do not waste token generation if not useful. $PAGE = new pageBuilder; $PAGE->assign('token',$token); - $PAGE->assign('returnurl',(isset($_SERVER['HTTP_REFERER']) ? $_SERVER['HTTP_REFERER']:'')); + $PAGE->assign('returnurl',(isset($_SERVER['HTTP_REFERER']) ? escape($_SERVER['HTTP_REFERER']):'')); $PAGE->renderPage('loginform'); exit; } @@ -1030,7 +1043,7 @@ function renderPage() // Get only links which have a thumbnail. foreach($links as $link) { - $permalink='?'.htmlspecialchars(smallhash($link['linkdate']),ENT_QUOTES); + $permalink='?'.escape(smallhash($link['linkdate'])); $thumb=lazyThumbnail($link['url'],$permalink); if ($thumb!='') // Only output links which have a thumbnail. { @@ -1239,8 +1252,8 @@ function renderPage() $PAGE = new pageBuilder; $PAGE->assign('linkcount',count($LINKSDB)); $PAGE->assign('token',getToken()); - $PAGE->assign('title',htmlspecialchars( empty($GLOBALS['title']) ? '' : $GLOBALS['title'] , ENT_QUOTES)); - $PAGE->assign('redirector',htmlspecialchars( empty($GLOBALS['redirector']) ? '' : $GLOBALS['redirector'] , ENT_QUOTES)); + $PAGE->assign('title', empty($GLOBALS['title']) ? '' : $GLOBALS['title'] ); + $PAGE->assign('redirector', empty($GLOBALS['redirector']) ? '' : $GLOBALS['redirector'] ); list($timezone_form,$timezone_js) = templateTZform($GLOBALS['timezone']); $PAGE->assign('timezone_form',$timezone_form); // FIXME: Put entire tz form generation in template? $PAGE->assign('timezone_js',$timezone_js); @@ -1400,7 +1413,7 @@ function renderPage() $PAGE->assign('link',$link); $PAGE->assign('link_is_new',false); $PAGE->assign('token',getToken()); // XSRF protection. - $PAGE->assign('http_referer',(isset($_SERVER['HTTP_REFERER']) ? $_SERVER['HTTP_REFERER'] : '')); + $PAGE->assign('http_referer',(isset($_SERVER['HTTP_REFERER']) ? escape($_SERVER['HTTP_REFERER']) : '')); $PAGE->assign('tags', $LINKSDB->allTags()); $PAGE->renderPage('editlink'); exit; @@ -1524,10 +1537,10 @@ HTML; ($exportWhat=='private' && $link['private']!=0) || ($exportWhat=='public' && $link['private']==0)) { - echo '
'.htmlspecialchars($link['title'])."\n"; - if ($link['description']!='') echo '
'.htmlspecialchars($link['description'])."\n"; + echo '
'.$link['title']."\n"; + if ($link['description']!='') echo '
'.$link['description']."\n"; } } exit; @@ -1540,7 +1553,7 @@ HTML; if (!isset($_POST['token']) || (!isset($_FILES)) || (isset($_FILES['filetoupload']['size']) && $_FILES['filetoupload']['size']==0)) { $returnurl = ( empty($_SERVER['HTTP_REFERER']) ? '?' : $_SERVER['HTTP_REFERER'] ); - echo ''; + echo ''; exit; } if (!tokenOk($_POST['token'])) die('Wrong token.'); @@ -1663,13 +1676,13 @@ function buildLinkList($PAGE,$LINKSDB) if (isset($_GET['searchterm'])) // Fulltext search { $linksToDisplay = $LINKSDB->filterFulltext(trim($_GET['searchterm'])); - $search_crits=htmlspecialchars(trim($_GET['searchterm'])); + $search_crits=escape(trim($_GET['searchterm'])); $search_type='fulltext'; } elseif (isset($_GET['searchtags'])) // Search by tag { $linksToDisplay = $LINKSDB->filterTags(trim($_GET['searchtags'])); - $search_crits=explode(' ',trim($_GET['searchtags'])); + $search_crits=explode(' ',escape(trim($_GET['searchtags']))); $search_type='tags'; } elseif (isset($_SERVER['QUERY_STRING']) && preg_match('/[a-zA-Z0-9-_@]{6}(&.+?)?/',$_SERVER['QUERY_STRING'])) // Detect smallHashes in URL @@ -1721,7 +1734,7 @@ function buildLinkList($PAGE,$LINKSDB) while ($i<$end && $iindexUrl().'?do=genthumbnail&hmac='.htmlspecialchars($sign).'&url='.urlencode($url), + return array('src'=>indexUrl().'?do=genthumbnail&hmac='.$sign.'&url='.urlencode($url), 'href'=>$href,'width'=>'120','style'=>'height:auto;','alt'=>'thumbnail'); } @@ -1878,7 +1891,7 @@ function computeThumbnail($url,$href=false) if ($ext=='jpg' || $ext=='jpeg' || $ext=='png' || $ext=='gif') { $sign = hash_hmac('sha256', $url, $GLOBALS['salt']); // We use the salt to sign data (it's random, secret, and specific to each installation) - return array('src'=>indexUrl().'?do=genthumbnail&hmac='.htmlspecialchars($sign).'&url='.urlencode($url), + return array('src'=>indexUrl().'?do=genthumbnail&hmac='.$sign.'&url='.urlencode($url), 'href'=>$href,'width'=>'120','style'=>'height:auto;','alt'=>'thumbnail'); } return array(); // No thumbnail. @@ -1897,11 +1910,11 @@ function thumbnail($url,$href=false) $t = computeThumbnail($url,$href); if (count($t)==0) return ''; // Empty array = no thumbnail for this URL. - $html=''; + $html=''; // Lazy image - $html.='alert("Shaarli is now configured. Please enter your login/password and start shaaring your links!");document.location=\'?do=login\';'; @@ -2210,7 +2223,7 @@ function genThumbnail() // This is more complex: we have to perform a HTTP request, then parse the result. // Maybe we should deport this to JavaScript ? Example: http://stackoverflow.com/questions/1361149/get-img-thumbnails-from-vimeo/4285098#4285098 $vid = substr(parse_url($url,PHP_URL_PATH),1); - list($httpstatus,$headers,$data) = getHTTP('https://vimeo.com/api/v2/video/'.htmlspecialchars($vid).'.php',5); + list($httpstatus,$headers,$data) = getHTTP('https://vimeo.com/api/v2/video/'.escape($vid).'.php',5); if (strpos($httpstatus,'200 OK')!==false) { $t = unserialize($data); diff --git a/tpl/daily.html b/tpl/daily.html index 0f76249..38aa401 100644 --- a/tpl/daily.html +++ b/tpl/daily.html @@ -36,12 +36,12 @@ {if="$link.tags"}
{loop="link.taglist"} - {$value|htmlspecialchars} - + {$value} - {/loop}
{/if}
- {$link.title|htmlspecialchars} + {$link.title}
{if="$link.thumbnail"}
{$link.thumbnail}
diff --git a/tpl/dailyrss.html b/tpl/dailyrss.html index a9b11e1..1b7ab8e 100644 --- a/tpl/dailyrss.html +++ b/tpl/dailyrss.html @@ -1,7 +1,7 @@ {loop="links"} -

{$value.title|htmlspecialchars}

- {if="!$GLOBALS['config']['HIDE_TIMESTAMPS']"}{function="strftime('%c', $value.timestamp)"} - {/if}{if="$value.tags"}{$value.tags|htmlspecialchars}{/if}
- {$value.url|htmlspecialchars}
+

{$value.title}

+ {if="!$GLOBALS['config']['HIDE_TIMESTAMPS']"}{function="strftime('%c', $value.timestamp)"} - {/if}{if="$value.tags"}{$value.tags}{/if}
+ {$value.url}
{if="$value.thumbnail"}{$value.thumbnail}{/if}
{if="$value.description"}{$value.formatedDescription}{/if}

diff --git a/tpl/editlink.html b/tpl/editlink.html index 0276f08..6737c41 100644 --- a/tpl/editlink.html +++ b/tpl/editlink.html @@ -15,11 +15,11 @@
-

-

-

+

+

+


-
{if="($link_is_new && $GLOBALS['privateLinkByDefault']==true) || $link.private == true"} @@ -32,7 +32,7 @@ {if="!$link_is_new"}{/if} - {if="$http_referer"}{/if} + {if="$http_referer"}{/if}
diff --git a/tpl/import.html b/tpl/import.html index 9ac3c2f..6c4f942 100644 --- a/tpl/import.html +++ b/tpl/import.html @@ -5,11 +5,11 @@ diff --git a/tpl/loginform.html b/tpl/loginform.html index 91b948d..678375f 100644 --- a/tpl/loginform.html +++ b/tpl/loginform.html @@ -17,7 +17,7 @@ Stay signed in (Do not check on public computers) - {if="$returnurl"}{/if} + {if="$returnurl"}{/if} {/if} diff --git a/tpl/page.footer.html b/tpl/page.footer.html index 42c621b..8143669 100644 --- a/tpl/page.footer.html +++ b/tpl/page.footer.html @@ -2,7 +2,7 @@ Shaarli - The personal, minimalist, super-fast, no-database delicious clone by the Shaarli community - Help/documentation {if="$newversion"} -
● Shaarli {$newversion|htmlspecialchars} is available.
+
● Shaarli {$newversion} is available.
{/if} {if="isLoggedIn()"} diff --git a/tpl/page.header.html b/tpl/page.header.html index 0fd65e4..2d186aa 100644 --- a/tpl/page.header.html +++ b/tpl/page.header.html @@ -8,7 +8,7 @@