ASP 3.x is full of security vulnerabilities. Almost everything that is grabbed via the querystring can be used by a hacker to easily access and manipulate the database, including dropping/deleting tables, or adding data into the fields.

yes and no. You first need to be logged in. And that code (checking login) doesn't build dynamic SQL from request params so it's not all that bad, actually. After being logged in - yes, I agree I have been lazy just building SQL dynamically however I think that layer of security is good enough for a free product. The pro version doesn't have these type of problems since it uses parameterized queries.

Hi Stefan, In the Javascript that appears on the pages where the ad is displayed and clicked we find this link in the pageview source: http://localhost/ads/adclick.asp?F=0&Z=2 I reviewed that public code where the querystring is grabbed and processed and throughout the code the request parameters are never sanitized and are inserted "as is" into the database and it's not behind the admin login either. In essence, this is wide-open to hackers. Am I reading this wrong?

thanks astralis. You are definetely on to something. While the adclick is not dangerous according my first look - since is never uses the params but a session variable (adclick is only used in img/href scenarios, when js is not available) - it's interesting to see what happens in adserve.asp. Also in scriptinject.asp. I'll investigate and get back!

Been checking it out - and I can confirm scriptinject.asp and adserve.asp is indeed open for some vulnerablility through one of the parameters - however I have still not figured out HOW to create the query param string to really exploit it. But - never cared that much. I have a fix - and wanted to get some input before I release it: My idea is to simply sanitize the input - I am SOOOOOOOO rusty when it comes to VBScript I am almost ashamed over the lameness over the code but never mind. In scriptinject.asp, adclick.asp and adserve.asp Before anything else: [code] for each x in Request.Form If UCase(x) = "Z" Then AdMentor_SecureZoneParam(Request(x)) Else If UCase(x) <> "NOCACHE" Then AdMentor_SecureLongParam(Request(x)) End If End If Next for each x in Request.QueryString If UCase(x) = "Z" Then AdMentor_SecureZoneParam(Request(x)) Else If UCase(x) <> "NOCACHE" Then AdMentor_SecureLongParam(Request(x)) End If End If Next [/code] For every parameter I simply call AdMentor_SecureLongParam [code] Function AdMentor_SecureLongParam(sParam) If sParam = "" Then Exit Function End If Dim nVar nVar = CInt(sParam) Exit Function End Function [/code] which checks it's a valid int. Crashes and stops if it isn't. I already did that for some of the params but in different areas in the code - better to do it in the same place. However - the Z param gets other treatment - being a "string" parameter [code] Function AdMentor_SecureZoneParam(sParam) If Len(sParam) = 0 Then Exit Function End if Dim sPart Dim fOk fOk = true For n = 1 to Len(sParam) fOk = false sPart = Mid(sParam,n,1) If sPart = "," Then fOk = true End If If sPart = "(" Then fOk = true End If If sPart = ")" Then fOk = true End If If sPart = " " Then fOk = true End If If sPart = "1" Or sPart = "2" Or sPart = "3" Or sPart = "4" Or sPart = "5" Or sPart = "6" Or sPart = "7" Or sPart = "8" Or sPart = "9" Or sPart = "0" Then fOk = true End If If fOK = False Then Response.Write "SECURITY MESSAGE" Response.End End IF Next End Function [/code] it could be z=1 or z=1,2 or z=1,2,3 or something like that. So I am checking for numbers and "," Hrmm, pretty lame, some regex should be much nicer - but as I said, I am soo rusty doing vbscript nowadays. And I actually don't any time or energy to spend on the admentor asp version... Input somebody? Last is nocache which is never used anyway so I never care for that.

Hi Stefan, I'm taking a look at this. Are you going through the entire public application (admin is a different issue)? -- admentor2.asp -- admentorredir.asp From admentorredir.asp ----> AdMentor2.asp Admentor2_ClickAd function ---> include_admentordb.asp AdMentor_DBUpdateClickCountAndGetUrl function: the banner id is inserted without sanitizing it. Essentially, anywhere the variables are picked up from the querystring need sanitized. For most of them it only needs to check for the integer but the Z variable also needs checked because it's a text string. I think your fix will correct it although I think it's a bit long. I have to study it some more to see what you're doing.

thanks atralis. Really appriciate you taking the time on this! Basically my idea - to save time etc - was not to do the check anytime a request("param") is done, but to just make changes to the parts accessible from the outer world - and to abort the request as son as it's not a valid input. With accessible from the outer world I mean a) publically available (not admin since it requires login) and b) just the three files mentioned. The admentor2.asp is never called from outside, it only contains functions called from the three files. Need to check admentorredir.asp, I thought it wasn't used anymore. Simply to save time. I have so much work to do the next couple of weeks so.

Hi Stefan, I found a solution. I understand what you're saying about laying a security layer at the top but I think that will cause more problems than it's worth and also add a layer that really isn't necessary compared to the little amount of work that can be done to secure everywhere that a variable is grabbed. It's really not that difficult and will make certain that nothing gets inserted into the database that you don't want. I did not use the script you produced above and instead created something else and sent it to Bill Wilkinson who is fantasic with ASP on aspmessageboard.com who essentially did all the work wrote and the following new function based on what I sent him (so any misunderstanding is mine, not his) -- I kept his notes in it: [code] Function am_intarr(sParam) sParam = Trim(sParam) ' if you don't do this, a string of spaces will cause trouble If Len(sParam) = 0 Then am_intarr = "" Exit Function End if Dim junk, arrStr strIndex = "" ' don't forget to initialize! arrStr = Split( sParam, "," ) On Error Resume Next for i=0 to Ubound(arrStr) ' again...trim to avoid problems with spaces junk = "X" junk = ", " & strIndex & CLng(Trim(arrStr(i))) & ", " If junk = "X" Then am_intarr = "The banner cannot display" 'This is the security message but doesn't make it obvious Exit Function End If Next On Error GoTo 0 am_intarr = sParam ' original was valid, why not just return it? End Function [/code] I believe the best place for this is on admentor2.asp. Then everywhere there is a request.form or request.querystring wrap it with this function. Here are the places in the non-admin section that I found (the same thing can easily be done for the admin): -- admentor2.asp -- scriptinject.asp -- adserve.asp -- adclick.asp -- admentorredir.asp This is not completely tested but shouldn't be that difficult to go through the code line-by-line to see where the requests are. Next step is to do this to the admin section. Do you spot anything in this that might conflict with the script? [quote] Need to check admentorredir.asp, I thought it wasn't used anymore. [/quote] All ads that display on the page are clicked through admentorredir.asp whether it's called via asp or j/s.