[ SiteAtSchool logo ] Site@School [ SourceForge.NET logo ]
Latest news: FINALLY: WEBSITE@SCHOOL (27-02-2011)
General  
Intro Developers
Developers manual
Security reports
S@S V3 Features
 
 
 
 
 
 

Security Report

Below you find the security report in three parts.
  1. Part one
  2. Part two
  3. Part three
The third part is conclusive.

Part one

Van:     Peter Fokker
Aan:     Dirk Schouten
         Karin Abma
         Fred Stuurman
Datum:   2006-09-13
Betreft: Advies Site@School


1. Inleiding
============

In juli 2006 is mij gvraagd om het Site@School Team te adviseren op
het gebied van de beveiliging van Site@School. Aan dat verzoek geef
ik graag gehoor.

In dit document zet ik eerst een aantal feiten op een rijtje. Daarna
doe ik een paar voorstellen voor stappen die het Site@School Team moet
zetten om te komen tot een verbeterde versie van Site@School.


2. Analyse
==========

2.1 Versies van Site@School
---------------------------

Hieronder een - incompleet - overzicht van de afgelopen 2 jaar van
Site@School-versies en fixpac's, met releasedatum.

Versie/fixpac	Releasedatum
-------------	------------
fixpac 2.4.03	2006-08-22
fixpac 2.4.02	2006-07-07
fixpac 2.4.01	2006-06-11
versie 2.4	2006-05-15

fixpac 2.3.05	2006-02-15
fixpac 2.3.04	2005-12-09
fixpac 2.3.03	2005-09-27
fixpac 2.3.02	2005-07-19
fixpac 2.3.01	2005-06-20
versie 2.3	2005-05-29

fixpac 2.2.04	2005-01-19
fixpac 2.2.03	2004-12-15
fixpac 2.2.02	2004-11-14 (?)
fixpac 2.2.01	(?)
versie 2.2	2004-10-31

Uit dit lijstje is af te leiden dat er na een officiele release altijd
binnen 1 maand een eerste fixpac verscheen. Voor versie 2.4 geldt dat
sinds de releasedatum tot nu toe elke maand een fixpac is
verschenen. Voor versie 2.3 was dat gemiddeld met tussenpozen van
ongeveer 7 weken, voor versie 2.2 was dat met tussenpozen van ongeveer
3 weken.

In dit overzicht is geen rekening gehouden met 'tijdelijke' versies
van het programma of van een fixpac. In het verleden is het meermalen
voorgekomen dat er verschillende versies zijn geweest met exact
hetzelfde versienummer, vaak omdat een kleine omissie in een fixpac
werd gecorrigeerd, zonder het versienummer op te hogen. Dat betekent
dat er een kans is dat er in werkelijkheid meer verschillende versies
van Site@School in omloop zijn dan de in bovenstaand lijstje
opgesomde.


2.2 Omvang van de broncode
--------------------------

In totaal omvat Site@School 2.4.03 meer dan 1500 bestanden van ruim 15
verschillende bestandstypes (extensies). Ongeveer 40% daarvan is aan
te merken als plaatje (.GIF, .JPG, .PNG), 40% is een .PHP bronbestand
en 8% is JavaScript (.JS).

De ca. 600 .PHP-bestanden bevatten samen ongeveer 81.000 regels code
in ruim 3,5 MB. De ca. 180 JavaScript-bestanden bevatten samen
ongeveer 30.000 regels code in 1,2 MB. Dit komt overeen met ongeveer
1.400 kantjes A4 (.PHP) resp. 480 kantjes A4 (.JS).

Het bestuderen van alle broncode zou, met een tempo van 4 pagina's
per uur, ongeveer 11 manweken in beslag nemen.


2.3 Broncode in .PHP-bestanden
------------------------------

Uit een paar steekproeven komen de volgende problemen met de PHP-code
naar voren.


2.3.1 Opmaak van de code
------------------------

De bronbestanden zijn bijzonder moeilijk te lezen en te doorgronden om
de volgende redenen.

a) De PHP-code is vaak gemixt met HTML-code. Het kost extra inspanning
   om de logica van de PHP-code te volgen tussen alle stukken HTML
   door.

b) De PHP-code is niet opgemaakt volgens de Site@School Coding
   Guidelines [1]. Vooral het niet laten inspringen van code
   d.m.v. tabs maakt het erg moeilijk om de logica van het programma
   te volgen in de code, vooral bij complexe
   it-then-else-constructies.

c) Sommige bronbestanden bestaan uit 1 hele lange rij instructies die
   op papier ettelijke pagina's A4 zou beslaan. Dit maakt het moeilijk
   om overzicht te krijgen in de logica van het programma.


2.3.2 Programmalogica
---------------------

De logica van het programme leidt soms tot onvoorziene situaties en
soms tot gaten die door kwaadwillenden misbruikt kunnen worden.

a) De de door de gebruiker aangeleverde gegevens worden niet voldoende
   gecontroleerd, bijv. op syntaxis (een parameter behoort
   numeriek te zijn, maar dat wordt niet gecontroleerd of afgedwongen) en
op
   inhoud (een parameter wordt gebruikt als deel van een padnaam maar
   er wordt niet gecontroleerd of het resultaat niet naar een
   systeembestand als /etc/passwd zou kunnen leiden).

b) Niet alle programmabestanden zijn nog voorzien van een
   beveiliging die het onmogelijk maakt om een zogenaamd
   'include'-bestand te misbruiken door het rechtstreeks aan te
   roepen.

c) Het systeem maakt het in principe mogelijk om bestanden naar de
   server te uploaden. De permissies van deze bestanden zijn over het
   algemeen te ruim ingesteld (0777).


2.3.3 Documentatie
------------------

Er is documentatie voor ontwikkelaars on-line beschikbaar [2],
inclusief een datadefinitie van zowel de tabellen van de applicatie
zelf als die van de modules.

In de bronbestanden zelf is de documentatie slechts spaarzaam
aanwezig. Sommige delen van de programmalogica zijn bijzonder moeilijk
te doorgronden zonder aanvullend commentaar.


3. Aanbevelingen
================

Op grond van de beschikbare informatie en de beperkte tijd is het
onmogelijk om op dit moment te komen tot een definitief antwoord op deze
vraag:

    "Moet Site@School geheel vanaf nul worden herschreven of is het
    mogelijk om de bestaande code zodanig aan te passen dat het
    systeem veilig en robuust wordt".

Een beter antwoord op deze vraag is eerst te geven na nadere
bestudering van (een deel van) de broncode. Hierover zullen wij eerst
afspraken moeten maken.

Desondanks is het mogelijk om nu reeds een aantal aanbevelingen te
doen die het systeem zelf alsmede de ontwikkelingsmethodiek ten goede
komen. Bovendien is het mogelijk om aanbevelingen te doen om de
bezwaren zoals geformuleerd in paragraaf 2.3.1 hiervoor deels op te
lossen.


3.1 Gebruik de beschikbare kennis
---------------------------------

De "OWASP Guide to Building Secure Web Applications" [3] bevat zeer
veel informatie over het ontwikkelen van veilige
(web-)applicaties. Deze gids bevat ettelijke voorbeelden van manieren
waarop eeb (web-)applicatie gecompromitteerd kan raken, met daarbij de
mogelijke oplossingen. Het is aan te raden dit boek van voor tot
achter door te lezen en te bestuderen en vervolgens de opgedane kennis
in praktijk te brengen.


3.2 Herformatteer de code
-------------------------

De huidige layout van de code maakt het bijzonder moeilijk om de
logica te volgen. Alleen al het herformatteren van de broncode helpt
om onlogische programmadelen te identificeren. Het herformatteren van
de code kan gebeuren met een zgn. beautifier [4]. Het consequent en
systematisch toepassen van de eerdergenoemde Coding guidelines [1] is
essentieel.


3.3 Splits code in hanteerbare stukken
--------------------------------------

Een procedure die ettelijke bladzijden beslaat is niet of slechts zeer
moeilijk te volgen. Beter is om zo'n procedure op te splitsen in
aparte functies met duidelijke namen. Idealiter beslaat een functie
niet meer dan 1 kantje A4.


3.4 Splits HTML en PHP
----------------------

Het mixen van PHP-code en HTML-code maakt de broncode moeilijk te
vatten. Door alle HTML-code te genereren vanuit PHP wordt het veel
eenvoudiger om alle broncode te doorgronden, zeker in combinatie met
het onderbrengen van de code in kleine, losse functies.


3.5 Defensief programmeren
--------------------------

Trust no 1! Vertrouw niet op "het zou nu zo moeten zijn dat ..." maar
dwing dat af. Voorbeelden:

- Als een variabele numeriek is of zou moeten zijn, dan is er geen
  enkel bezwaar om dat dan ook af te dwingen, bijv. d.m.v. de
  standaardfunctie intval().

- Als een variabele een gebruikersnaam moet voorstellen waarin
  uitsluitend letters en cijfers mogen voorkomen, dan is er geen
  bezwaar om zo'n variabele met behulp van een reguliere expressie en
  een zoek-en-vervang de dwingen in dat keurslijf en daarme alle
  andere, potentieel gevaarlijke tekens en mogelijk misbruik
  onmogelijk te maken.

- Als een variabele een keuze voorstelt uit een lijst van beschikbare
  mogelijkheden, dan moet die variabele ook gevalideerd worden tegen
  die lijst. Met deze programmeerwijze valt een parameter 'theme'
  (door de gebruiker te kiezen  uit een lijst met bestaande
  directorynamen) met de waarde "etc/passwd%00" meteen
  door de mand.


3.6 Controleer alle eenvoudige potentiele gaten
-----------------------------------------------

Er zijn hele venijnige aanvallen mogelijk door middel van het
include()'en van bestanden, vooral in combinatie met de flexibiliteit
van 'allow_url_fopen' (remote file inclusion) in php.ini. Het ligt
voor de hand om _alle_ plaatsen waar een bestand wordt ingevoegd met
bijv. include() of require() te controleren. Zodra er sprake is van
een variabele, bijv. include($filename), moet gecontroleerd worden of
de variabele (hier: $filename) niet misbruikt kan worden, bijv. omdat
de variabele niet geinitialiseerd wordt en register_globals 'aan'
staat. Ook zou afgedwongen moeten worden dat zo'n in te voegen bestand
daadwerkelijk een lokaal bestand is.

Controleer alle plaatsen waar mogelijk een bestand op de server
geplaatst wordt (de upload()-functies). Laat uitsluitend bestanden toe
uit een lijst met 'goedgekeurde' extensies. Dus: wel .jpg, .gif, .png,
en dergelijke, en elke andere eenvoudigweg niet. Dit is veel effectiever
dan een uitputtende lijst van extensies die NIET toegestaan zijn,
bijv. .php, .pl, .py want misschien komt er dan alsnog een file .cgi
of .php3 binnen. Ook dit is weer een vorm van defensief programmeren:
niets mag tenzij het wel mag.

Controleer het hele programma op aanroepen van chmod() en misschien
ook wel op umask() en fopen(). Het uitvoerbaar maken van bestanden
(het x-bit in de permissies 'rwx') in combinatie met ueberhaupt de
faciliteit van het uploaden van bestanden, ook al zijn dat bestanden
met 'goede' extensies levert potentiele problemen op, nl. dat een
"uitvoerbaar" plaatje foo.jpg (met permissies 0777) mogelijk toch
shellcode zou kunnen bevatten. Dankzij de permissies is dan die
ogenschijnlijk onschuldige foto opeens een gevaarlijk shellscript dat
met permissies van de www-user zou kunnen werken. Vuistregel is dat
bestanden permissies 0640, 0644, 0660 of 0664 zouden mogen hebben
maar niet 0666. Voor directories geldt 0750, 0755, 0770 of 0775 maar
niet 0777. Bestanden met een '7' of een '5' zijn uit den boze.

Maak gebruik van constructies die bij kleine vergissingen een
foutmelding geven in plaats van een niet of moeilijk te detecteren
fout. Voorbeeld:

    if ($var = 'foo') then  { ... }

betekent heel iets anders dan

    if ($var == 'foo') then  { ... }

Dat extra '='-teken is gemakkelijk over het hoofd te zien. Beter is
altijd de constante links te zetten en de variabele rechts:

    if ('foo' == $var) then  { ... }

omdat de constructie

    if ('foo' = $var) then  { ... }

leidt tot een gemakkelijk te herkennen (en te fixen) foutmelding.


3.7 In-line documentatie
------------------------

Documentatie is essentieel. Veel programmabestanden hebben behoefte
aan goede in-line documentatie, vooral op punten waar iets onverwachts
gebeurt. Bijvoorbeeld:

    if ($user_type == 19) then { ... }

Voor een buitenstaander is het volstrekt onduidelijk wat de diepere
betekenis van het magische getal 19 is. Een enkele regel commentaar
kan in zo'n geval veel verduidelijken. Commentaar als "check of
user_type 19 is" daarentegen werkt contra-productief en voegt niets
toe.


3.8 Versiebeheer met CVS
------------------------

Alle wijzigingen die aangebracht worden in de code, of het nu security
fixes zijn of nieuwe features of gestroomlijnde code, moeten ergens
gedocumenteerd worden. Een zeer goede methode daarvoor is het
onderbrengen van _alle_ Site@School-broncode, d.w.z. inclusief
grafische bestanden en bovendien alle wijzigingen, in het
versiebeheersysteem CVS [5]. CVS is reeds beschikbaar voor het project
Site@School [6]. CVS dwingt min of meer af om bij elk gewijzigd
bestand een of meer regels commentaar toe te voegen aan de CVS
repository. Daarmee wordt het een fluitje van een cent om wijzigingen
te documenteren.

Bij het publiceren van releases, fixpacs of losse bestanden van
Site@School is het essentieel om verschillende versies van elkaar te
kunnen onderscheiden, ook achteraf. Het mag niet zo zijn dat er twee
verschillende versies van een bestand gepubliceerd worden met exact
hetzelfde versienummer. Ook hier kan CVS goede diensten bewijzen omdat
dit systeem in staat is om automatisch in elke versie van een bestand
een regel tekst op te nemen waarin o.a. het versienummer van dat
bestand wordt opgenomen. Ik doel hier op de parameter $Id$. Als elk
bestand op deze wijze een uniek kenmerk krijgt, in leesbare vorm,
wordt het gemakkelijker om op afstand te bepalen om welke versie het
exact gaat.


4. Vervolgafspraken
===================

De hierboven opgesomde aanbevelingen zijn nu reeds toepasbaar,
onafhankelijk van de vraag of Site@School herschreven moet worden of
slechts aangepast. Een definitief advies hieromtrent is eerst mogelijk
na nadere bestudering van de code. Op dit moment is het ondoenlijk om
de code binnen een redelijke termijn goed te beoordelen.  Hooguit is
het mogelijk om een representatieve steekproef te nemen uit de massa
van 600 bronbestanden om op grond daarvan tot een met goede argumenten
onderbouwd advies te komen.  Hierover zullen wij nadere afspraken
moeten maken.

5 Referenties
=============


[1] Coding guidelines, http://wyxs.net/starnet/manual/coding.html
    of Site@School Coding Standards
    http://siteatschool.sourceforge.net/index.php?section=13&page=55,
    (voorjaar 2005) 

[2] Site@School Developers Manual, 
    http://siteatschool.sourceforge.net/index.php?section=13&page=60

[3] Van de webstek http://www.owasp.org:
    > The OWASP Guide to Building Secure Web Applications v2 is now
    > released. Its release was announced at Black Hat in Las Vegas in
    > late July 2005. This new version of the OWASP Guide is a major
    > overhaul of the original document, containing nearly three times
    > as much material. The project is currently steered by Andrew van
    > der Stock.
    > The original OWASP Guide had become a staple diet for many web
    > security professionals. Since 2002, the initial version was
    > downloaded over 2 million times. Today, the Guide is referenced by
    > many leading government, financial, and corporate standards and is
    > the Gold standard for web application security.
    > The Guide is aimed at architects, developers, consultants and
    > auditors and is a comprehensive manual for designing, developing
    > and deploying secure web applications.
    Grootte: 3 MB, 293 pagina's. Dit naslagwerk is een must.

[4] Voor PHP4 is er Beautify PHP 0.5.0,
    http://www.bierkandt.org/beautify 

[5] CVS - Concurrent Versions System. Meer informatie op
    http://www.nongnu.org/cvs/ en
    http://savannah.nongnu.org/projects/cvs/ Het standaardwerk waarin
    CVS tot in detail gedocumenteerd is, staat bekend onder de naam
    'Cederqvist', naar de originele auteur Per Cederqvist. Dit
    naslagwerk is een must.

[6] Sourceforge.NET biedt standaard de mogelijkheid om projecten op te
    slaan in CVS, zie http://siteatschool.cvs.sourceforge.net/
	

Back to top of part one
To part three

Part two


Van:     Peter Fokker
Aan:     Dirk Schouten
         Karin Abma 
         Fred Stuurman
Datum:   2006-12-22
Betreft: Bevindingen mailpage-module Site@School


1. Inleiding
============

Medio september 2006 heb ik een aantal adviezen geformuleerd [1] om te
komen tot de beantwoording van de vraag

    "Moet Site@School geheel vanaf nul worden herschreven of is het
    mogelijk om de bestaande code zodanig aan te passen dat het
    systeem veilig en robuust wordt".

De conclusie was dat het op dat moment onmogelijk was om deze vraag te
beantwoorden. Wel was het mogelijk om een aantal algemene
aanbevelingen te doen om de code leesbaarder te maken en de kwaliteit
van de code te verhogen.

Om te voorkomen dat het gehele systeem overhoop gehaald moest worden
zijn deze aanbevelingen in eerste instantie uitgewerkt in een enkele
'model-module'. Gekozen is voor de module 'mailpage'.

De aangepaste bestanden zijn:
- starnet/modules/mailpage/mailpage.php [2]
- starnet/modules/mailpage/admin.php [3]
- starnet/core/common.inc.php (sanitize function) [4]

Ik heb deze bestanden beoordeeld op de aanbevelingen uit de eerder
aangehaalde netpostbericht [1].


2. Bevindingen
==============

Kortheidshalve refereer ik in onderstaand overzicht aan de
paragraafnummers uit [1].


Referentie: 2.3.1 Opmaak van de code
------------------------------------

De code is iets minder onoverzichtelijk geworden, waarschijnlijk
dankzij een beautifier. Wel zie ik nog steeds veel gemixte code,
bijv. in mailpage.php rond regel 220; daar staat een heel stuk
JavaScript in moeilijk te lezen opmaak en niet-ingesprongen blokken.
Kennelijk kwam de beautifier hier ook niet uit.

Opvallend is dat de code nog steeds een zeer lange rij instructies is,
zonder functies (subroutines). Voorbeeld: een enkele switch/case van
ruim 225 regels (=bijna 4 kantjes A4) in mailpage.php ongeveer
vanaf regel 112. Zelfs bij het in landscape afdrukken op A3 is dit
zeer moeilijk te overzien.


Referentie: 2.3.2 Programmalogica, sub a: inputvalidatie
--------------------------------------------------------

Hier is wel een verbetering te zien in de vorm van de functie
sanitize(). Echter, deze functie op zich is wel enigszins
problematisch. Ik kom daar later nog op terug.


Referentie: 2.3.3 Documentatie
------------------------------

Ook het bestand admin.php (252 regels) is een lange rij van
instructies met op regel 108 het begin van een switch/case die
doorloopt tot regel 248. De in-line documentatie "start of switch" op
deze regel 108 zegt mij in het geheel niets. Ik kan de code lezen en
op regel 108 staat dit:

    switch ($modoption) // start of switch

Tja. Ik blijf zitten met de vraag wat er nu eigenlijk gebeurt.


Referentie: 3.2 Herformatteer de code
-------------------------------------

Zoals hiervoor al gezegd is hier een verbetering op te merken. Dat wil
niet zeggen dat het niet beter zou kunnen. Met name de formattering
van de _output_ is nog niet optimaal. Immers, ook de gegenereerde HTML
heeft ook baat bij een systematische formattering, met name op het
gebied van de embedded javascript.


Referentie: 3.3 Splits de code in hanteerbare stukken
-----------------------------------------------------

Helaas moet ik constateren dat hier nauwelijks verbetering is te
zien. De gigantische switch/case-constructies zouden echt prima
opgesplitst kunnen worden zodanig dat het programma veel beter te
begrijpen en te onderhouden is. Ik zie in admin.php dit (met
regelnummers):

108:    switch ($modoption) // start of switch
111:    	case save_config :
122:    	case delete_contact :
127:    	case save_contact :
147:    	case config :
166:    	case add_contact :
189:    	case edit_contact :
215:    	case list_contacts :
248:    } //end of switch


Overzichtelijker zou zijn:

switch ($modoption)
{
	case save_config :
		do_save_config();
		break;
	case delete_contact :
		do_delete_contact();
		break;
	case save_contact :
		do_save_contact();
		break;
	case config :
		do_config();
		break;
	case add_contact :
		do_add_contact();
		break;
	case edit_contact :
		do_edit_contact();
		break;
	case list_contacts :
		do_list_contacts();
		break;
	default:
		trigger_error("unknonw option '$modoption'");
		do_list_contacts();
		break;
} //end of switch

gevolgd door de definities van de verschillende do_xxx()
functies. Door deze werkwijze zie je in ongeveer 30 regels -- een half
kantje A4 -- precies de logica van het programma. De details zijn dan
wel te vinden in de individuele routines, maar voor de grote lijn
leidt dat alleen maar af.

Een bijkomend voordeel van het gebruiken van functies is dat elke
gebruikte globale variabele expliciet gedeclareerd MOET
worden. Daarmee wordt je als programmeur min of meer gedwongen om je
af te vragen welke variabele je eigenlijk binnen die functie gebruikt
en vervolgens of die globale variabelen eigenlijk wel 'sane' zijn (dus
niet dankzij een register_globals = true gemanipuleerd kunnen worden
door een indringer).


Referentie: 3.5 Defensief programmeren
--------------------------------------

Dit gaat al beter dan voorheen, o.a. wegens de functie sanitize().
Wel levert het toepassen van reguliere expressies soms onbedoelde
resultaten op die moeilijk te vinden zijn, vooral als er geen enkele
documentatie bij zit. Een voorbeeld is te vinden in mailpage.php (met
regelnummers): 

229:    function checkinput ( form )
230:    {  
231:    	var filter  =
/^([a-zA-Z0-9_\.\-])+\@(([a-zA-Z0-9\-])+\.)+([a-zA-Z0-9]{2,4})+$/;
232:    	
        [...]
246:    	if (!filter.test(form.email.value)) 
247:    	{
248:    	alert( \"$sas_lang[email_module_noemail_error]\" );
249:    	return false ;
250:    	}

Mijn interpretatie van het reguliere expressie-object 'filter' is dat
hier een poging wordt gedaan om client-side een netpostadres te
valideren. Ruwweg moet een geldig adres voldoen aan 1 of meer
letters/cijfers/underscore/dot/dash gevolgd door 1 at gevolgd door 1
of meer reeksn van 1 of meer letters/cijfers/dash eindigend op een
dot, gevolgd door een string van 2 tot 4 letters/cijfers. Mmmmm, en
als ik nou het perfect legale email-adres
info@leiden.volkenkunde.museum wil invoeren? Anderzijds wordt het
apert onjuiste adres 123@456.789 wel geaccepteerd. Tja.


Referentie: 3.7 In-line documentatie
------------------------------------

In mailpage.php lees ik

220: