Login - SQL-Injection sikkert?

Prøv at tage et kig på min guide om parameterisering af SQL-queries med ADO. Her får du en idé om hvordan du kan gøre, så din kode bliver sikret mod SQL-injections (for det er din kode bestemt ikke)...

Du kan finde guiden her: http://www.eksperten.dk/guide/1250

Der kan laves sql injection allerede her

where mail = '" & username & "'

Du skal gøre som SOFTSPOT skriver eller analyse de parametre der kommer via requests for tampering med tal som ikk er tal eller konstrueret tekst "OR 1=1 " og lignende.

Det vil jeg lige tæske igennem.. smid et svar soft :)

God fornøjelse med det! :-)

Hej softspot.

Jeg prøvede med en masse sjov sql injection på det førstnævnte loginsystem, og ved at kende kodeordet på den første bruger, kunne jeg logge ind uden at benytte brugernavnet.
Nu har jeg ændret det, vha. guiden, og kan umiddelbart ikke finde nogen huller.
Kan du? :)

Først og fremmest en global funktion:
  function commandCreateText(conn, sql)
    set cmd = server.createobject("adodb.command")
    set cmd.activeconnection = conn
    cmd.commandtext = sql
    cmd.commandtype = adcmdtext
    set commandCreateText = cmd
  end function

dernæst et conn-object som indgår i alle sider vha includes:
  set conn = server.createobject("adodb.connection")
  dsn = "Provider = sqloledb; Data Source = SERVER; Initial Catalog = DATABASE; User Id = BRUGERNAVN; Password = PW"
  conn.connectiontimeout = 300
  conn.commandtimeout = 300
  response.buffer = true

Og til sidst selve scriptet:
  sql = "select id, username, password, attempts, active, level from " & userTable & " where username = ? and active = '1'"
  set cmd = commandCreateText(conn, sql)
  set rs = cmd.execute(, array(username))

Er der noget der kan optimeres?
Mening er, at systemet først tjekker om brugernavnet findes igennem parameterisering og DEREFTER sammenligner det eventuelt fundne brugernavn med dets kodeord som er lageret i databasen.

Jeg ved at begge disse to ting virker:
  cmd.Parameters.Append cmd.CreateParameter("@kodeord", adVarChar, adParamInput, 20, kodeord)
  cmd.Parameters.Append cmd.CreateParameter("@brugernavn", adVarChar, adParamInput, 20, brugernavn)
  set rs = cmd.Execute()
OG
  set rs = cmd.Execute(, array(kodeord,brugernavn))

Men jeg kan ikke lige forstå, hvorfor den nederste metode ikke skal have at vide, at der er tale om en varchar på 20 tegn, ligesom den øverste metode får at vide?

OG lige et par ting mere :)

Hvorfor filen skal der sættes et komma foran ens array i cmd.execute()?
Altså:
  set rs = cmd.Execute(, array(kodeord,brugernavn))

Og har man kun én ting, kan man da nøjes med fx at bruge:
  set rs = cmd.Execute(, brugernavn)

Da jeg ville lave:
  cmd.execute(, session("id"))

Fik jeg følgende fejl:
Der opstod en Microsoft VBScript-kompileringsfejl fejl '800a0414'

Der kan ikke bruges parenteser ved kald af en Sub

/intra/default.asp, linje 11

cmd.execute(, session("id"))
----------------------------^

Men fjernede jeg så paranteserne så det blev til:
  cmd.execute , session("id")

så virkede det fint? Har det noget at sige om paranteserne er der eller ej? :)

Generelt set bør du erklære dine variable i den kontekst du skal bruge dem, dvs. helt konkret bør du erklære cmd i commandCreateText for at undgå evt. konflikter med globale cmd-variable. Dette er ikke en decideret fejl, men det er en potentiel fejlkilde (som er svær at finde når den opstår!) og det kan man ligeså godt undgå, når nu det er så let :-)
Overvej f.eks. dette eksempel (med udgangspunkt i din nuværende implementering af commandCreateText):


Hvilken tabel tror du cmd slår op i når cmd.Execute kaldes (hvis altså den ikke ligefrem fejler fordi den command der rent faktisk udføres jo ikke tager nogle parametre... HINT! ;-)???

Dette er et fuldstændig legalt scenarium, hvis du skal arbejde med batchoperationer hvor der kaldes til databasen i en løkke, da det sparer nogle resurser, fordi der ikke skal oprettes nye command-objekter for hvert kald...

Umiddelbart er der ikke lige noget der springer i øjnene som fejl, men jeg vil dog gerne vide hvor userTable-variablen kommer fra? Hvis det er en konstant du har defineret i koden, så fair nok... hvis det er en værdi som genereres på grundlag af udefrakommende input, så er det et potentielt sikkerhedshul.

At du kan kalde cmd.Execute med en parameterliste uden at angive parametertyper osv. skyldes givetvis (jeg er ikke helt sikker på hvad der rent faktisk sker), at ADO selv tolker på, hvilke felttyper der arbejdes med i SQL-sætningen. Dvs. er feltet, som parameteren sammenlignes med i SQL-sætningen, en varchar, så forventer ADO en streng (eller en værdi der kan konverteres til en streng med de, i VBScript, gængse typekonverteringsregler).

Grunden til at du skal kalde Execute med et komma først (hvis du ikke angiver nogen første parameter) er, at execute rent faktisk tager 3 (valgfrie) parametre. Dette betyder i VBScript, at du kan undlade at angive nogen værdi for paremeteren i kaldet. Så hvis du kun vil angive en værdi til anden parameter, skal du indikere dette overfor VBScript og det gøres ved at angive et komma...

Jf. dokumentationen af Command.Execute (http://msdn.microsoft.com/en-us/library/ms681559(v=VS.85).aspx) forventes der et array af værdier i parameterlisten (anden parameter til Execute), så om det rent faktisk fungerer at sende en konkret værdi må være et spørgsmål om at teste det. Jeg plejer at sende arrays...

Grunden til at du får fejlen med paranteser ved kald af sub er, at VBScript ikke tillader dette, når du kalder subs og funktioner... med mindre du kun sender én parameter (det vil jeg dog ikke anbefale med mindre du 100% ved hvad du gør), f.eks.


eller at du foranstiller kaldet med kommandoen call, f.eks.


Personligt foretrækker jeg at benytte call, da det giver et mere konsistent (og normalt) udseende funktionskald  - i det mindste når man er vant til at kode i andre sprog end VBScript...

Så for at erklære variablen cmd skal jeg blot gøre nedenstående:

function commandCreateText(conn, sql)
  dim cmd
  set cmd = server.createobject("adodb.command")
  set cmd.activeconnection = conn
  cmd.commandtext = sql
  cmd.commandtype = adcmdtext
  set commandCreateText = cmd
end function

Og så er det problem løst? :)

userTable variablen er defineret i en settings.asp, så i tilfælde af at jeg ændre navnet i databasen, skal jeg kun ændre den ét sted i ét dokument, istedet for 400 steder i 18 dokumenter.
Så ingen fejl der, går jeg ud fra :)

Tjek mht kommaet i execute kommandoen :) det gir sig selv, efter lidt forklaring :)

Så dvs, hvor jeg før gjorde fx følgende:
  sql = "update " & userTable & " set attempts = 0 where id = " & id
  conn.execute(sql)
Skal jeg nu gøre følgende:
  sql = "update " & userTable & " set attempts = 0 where id = ?"
  set cmd = commandCreateText(conn, sql)
  call cmd.execute(, array(id))
Variablen id er hentet fra request.querystring("id")
Og sådan burde jeg gøre overalt på sitet? :)

Btw, hvilken BB-code bruger du til at indramme kode i? :) Bare så jeg kan lave mere overskuelig spørgsmål/svar i fremtiden ;)

Ja, med den erklæring af cmd i funktionen er din kode blevet lidt mere robust :-)

Hvis du har brug for at referere til userTable så mange steder, ville det måske give mening at pakke koden ind i en klasse eller en håndfuld funktioner som ligger i den samme ASP-fil. Denne kunne du så inkludere, hvor du har brug for dem...
Men bort set fra det, så er der ikke umiddelbart noget problem ved at gøre det på den måde :-)

Din omskrivning af SQL-sætningen til parameterisering ser korrekt ud.

Du kan benytte andre koder end dem der er listet under kommentarfeltet (b, i, u og img). Jeg har haft succes med div og pre, hvor div rammer teksten ind og pre viser teksten i monospacefont. Du kan muligvis benytte andre HTML-elementnavne, men det har jeg ikke eksperimenteret med... mon ikke der findes en komplet liste med fungerende BB-koder et eller andet sted på eksperten.dk...? :-)

Fantastisk :)

Mange tak for det hele! :)

Velbekomme :-)