1

While running scans with ZAP, I noticed that many of the reported vulnerabilities involve sending malicious content as a value to a parameter named "query". For example:

http://<url>:<port>/path/path/path?query=query+AND+1%3D1+--+

The api method in question, however, does not have a parameter that goes by this name, so I was wondering how the tool could possibly detect a vulnerability with this. ZAP did, in fact, report an SQL Injection vulnerability with "Medium" confidence from using a url like the above, and has had similar results with path traversal and xss.

Could this be a consequence of me not setting up ZAP correctly? Is it possible to for an api to be made to read parameters that the developers did not define? Is ZAP just guessing at the parameters the api uses?

schroeder
  • 123,438
  • 55
  • 284
  • 319
harrys
  • 109
  • 1
  • 8
  • How literal is your example? Did it also report a `path/` path parameter? – schroeder May 28 '19 at 18:09
  • URIs are just strings. Any parameter name can be supplied. I'm guessing there is/are frameworks (probably PHP) that have some vulnerability where the internal value of a DB query can be modified by a parameter. – JimmyJames May 28 '19 at 18:10
  • Is there anything in the logs that says what suite of tests is running this? There are 10 or so listed here: https://www.owasp.org/index.php/Appendix_A:_Testing_Tools#Testing_for_SQL_Injection – JimmyJames May 28 '19 at 18:16
  • The "path" is not literal". The literal parts are before "" and after "?" – harrys May 28 '19 at 18:37
  • Looking for the logs (nothing immediately available in the zap directory). The link provided, however, seems to refer to 10 separate tools, however, rather than 10 different suites that can be used with this tool. – harrys May 28 '19 at 18:47
  • Sorry, it came up on a google search for ZAP and I made the (probably bad) assumption that the tool is built upon other tools. I'm having trouble finding specifics on what tests are executed and how that is defined. – JimmyJames May 28 '19 at 19:26
  • @JimmyJames You don't like PHP, do you :) – Conor Mancone Aug 12 '19 at 18:53
  • @ConorMancone I honestly don't know much about the language itself. But I see an unending stream of vulnerabilities (typically SQLi) in PHP web frameworks. I'm not sure if it's the language or just a culture around it that continues to be willfully ignorant of basic security practice. When I read long (recent) discussions about how best to cleanse input in PHP for building SQL statements using concatenation, I just cringe. – JimmyJames Aug 12 '19 at 21:02
  • @JimmyJames I would be curious to see if PHP projects tend to have worse security than others. There is a good chance that your perception is driven by the fact that PHP is both common and also tends to be used by beginners. It is true that early versions of PHP had some options that were insecure by default and created issues, but that was true of most of the internet in the 90s/00s. Modern PHP is a much rosier picture. – Conor Mancone Aug 12 '19 at 21:37
  • @ConorMancone It's completely unscientific and mostly anecdotal but look at [this page](https://www.cvedetails.com/vulnerability-list/opsqli-1/sql-injection.html) and search for 'PHP' and see how frequently it comes up. But what really irks me is things like this: https://www.php.net/manual/en/function.mysql-real-escape-string.php which says basically "don't use this Rube Goldberg solution, use this other one." This is a solved problem. Why is anyone still talking about escaping SQL as a solution for this at all? – JimmyJames Aug 13 '19 at 14:03
  • @JimmyJames I mean, you did notice the large deprecation notice on the top of that page, right? That was introduced in PHP 4.3, aka 1995. I'm not even sure if prepared queries existed back then. It was removed from the language all together in PHP 7.0, aka 2015. The language has done plenty to move people along to better security, but there's only so much that can be done, especially for a language with such a long history and a wide prevalence of people on ancient versions. In many ways, it is a victim of its own success. – Conor Mancone Aug 13 '19 at 14:15
  • @ConorMancone "I mean, you did notice the large deprecation notice on the top of that page, right?" -> "don't use this Rube Goldberg solution, use this other one." – JimmyJames Aug 13 '19 at 14:35
  • @ConorMancone What is it that prevents the use of a proper solution here? Is there something about PHP makes using parameterized queries impossible? – JimmyJames Aug 13 '19 at 14:44
  • @JimmyJames Ah, misunderstanding. Parameterized queries are easily available and widely used in PHP. The mysqli and PDO interfaces it references both have parameterized options available. It is true that an equivalent method still exists in both APIs (a detail I forgot), but that is no different than in any other language (the python MySQLdb package has an equivalent also). The existence of this method doesn't make a language inherently insecure. It might be helpful for the docs to say "You probably want prepared queries instead" but I don't think it is reasonable to hold that against PHP – Conor Mancone Aug 13 '19 at 14:58
  • @ConorMancone Hard to find a direct "here's when PreparedStatements were introduced" but I found it in the JDK from Java 1.1 created in '96 or '97 – JimmyJames Aug 13 '19 at 14:58
  • @ConorMancone But why are we still seeing the use of this in 2019? What is it about PHP that makes using this flawed approach so common? – JimmyJames Aug 13 '19 at 15:00
  • Let us [continue this discussion in chat](https://chat.stackexchange.com/rooms/97380/discussion-between-conor-mancone-and-jimmyjames). – Conor Mancone Aug 13 '19 at 15:15

1 Answers1

4

Yes, its added by this code: https://github.com/zaproxy/zaproxy/blob/develop/zap/src/main/java/org/parosproxy/paros/core/scanner/VariantAbstractQuery.java#L148-L153

This is added to see how webapps react to queries - if they handle them badly then yes this can introduce vulnerabilities.

If you think ZAP has reported false positives then please report them too us: https://github.com/zaproxy/zaproxy/issues/new?labels=bug&template=Bug_report.md

Note: As of ZAP 2.8 the addition of parameters to requests that didn't originally have them is now optional (and default off). Ref: https://github.com/zaproxy/zap-core-help/wiki/HelpUiDialogsOptionsAscaninput#add-url-query-parameter

kingthorin
  • 574
  • 4
  • 6
Simon Bennetts
  • 1,390
  • 7
  • 10
  • OK. Thank you. It looks like my post is duplicated by another (I should've checked more carefully): https://security.stackexchange.com/questions/191758/false-positive-sql-injection-by-zap-with-adding-new-parameter-query?rq=1. So, I'll look at that. It looks like it might be a false positive, so I'll mention it on the board just in case. – harrys May 29 '19 at 14:43
  • The following request was reported as an SQL Injection vulnerability: GET http://:/?query=query+AND+1%3D1+--+ HTTP/1.1 I inserted it into my browser and then inserted this: GET http://:/ HTTP/1.1 I found that the content for both requests were identical. This appears to contradict what was reported by ZAP: “The vulnerability was detected by successfully retrieving more data than originally returned, by manipulating the parameter”. – harrys May 29 '19 at 14:45
  • It looks like a similar issue has been reported already and a new version released since then: https://github.com/zaproxy/zaproxy/issues/4231. I'll check my version, update if necessary and rescan. – harrys May 29 '19 at 15:09
  • The addition of parameters was made optional last fall, it will be available in 2.8. https://mobile.twitter.com/kingthorin_rm/status/1041855009263108097 – kingthorin May 31 '19 at 09:57