Sandbox::https://github.com/jorgectf/codeql/pull/9

RasmusWL · 2022-03-03T21:23:03Z

It turned out quite long... but also involved me writing some tests for what the actual behaviors of the parsing frameworks were.

I kept up with looking at the .expected file for quite a long time, too long I would say, but rewriting the tests to NOT use these (and instead inline expectations) was probably the best decision for this rewrite. I think I did this to try and keep your work as intact as possible... but in the end I think I had to break that.

This IS quite a mouthful. I think there are interesting things for you to learn from this, but I can also understand that it could take some time for you to process this. If you have not had time to review this within 1 week, I think I will just merge your PR, and apply this commits on top, so we can get your good work closer to being part of the default query suite (unless you object to this 1 week).

The tests for type-trackers were not that interesting, since they did not have XML input in both cases, which is the problem we were trying hard to solve. I did keep the test-case of not-user-supplied url alive as well though :+1: I added OK/NOT OK annotations. Notice that we report all 4 kinds of vulnerabilities on line 93

This gives some freedom in changing the name presented, and not worrying about whether you have made a typo that makes everything break :|

This made it much easier to debug the current alerts on tests at least. Notice that it's important that we have `strictconcat` and not just `concat`, since `concat` will also allow flow to sinks that are not vulnerable to any kind of XML vulnerability :|

And add annotations, see PoC.py for reference Some of these needs fixing though

This was a rough quick-n-dirty query, and should get some qhelp as well at some point.

I was loosing my mind from looking through those .expected files Just going to take it one file at time, to make reviewing easier

I had forgotten about this, but better late than never... also added a small representative test

But handling this in a nice way will require some restructuring

and handle parser being passed as positional argument

jorgectf · 2022-03-04T00:08:54Z

Outstanding rewrite 🤯. I've commited some qldocs' typos and made a very little polishing.

I think there are interesting things for you to learn from this

This PR is gold! I've made some notes on stuff to give a deeper look soon. Really liked the commit messages' explanations :)

Thank you! 😊

(I should have made a review instead of throwing simple comments 😳)

Co-authored-by: Jorge <46056498+jorgectf@users.noreply.github.com>

@jorgectf

Nice spotted @jorgectf!

RasmusWL and others added 30 commits March 2, 2022 14:24

Python: XML: Expose vuln kind on sink

ee23c05

Python: Add XMLVulnerabilityKind

aaf55b2

This gives some freedom in changing the name presented, and not worrying about whether you have made a typo that makes everything break :|

Python: Improve QLDoc for XML parsing/parsers

16e482b

Python: rewrite xml sax modeling

7f7758b

Python: Add lxml positive test

515b824

Python: Better handling of resolve_entities arg in lxml

661d8bf

Python: Add PoC for XML vulns

52891cb

Python: Model lxml.etree.get_default_parser in own class

3c321dd

Python: Expand lxml tests

124c03c

And add annotations, see PoC.py for reference Some of these needs fixing though

Python: Properly handle huge_tree in lxml

e295399

Python: Handle DTD retrieval vuln in lxml

703e3e8

Python: Properly model xml.etree

6129193

Python: Annotate xmltodict tests

3affa6c

Python: Expand XML PoC with minidom/pulldom/expat

c4d08db

Python: Annotate xml.dom tests

5a65248

Python: Fix vuln detection for xml.minidom with parser arg

9406a97

Python: Add separate query for SimpleXMLRPCServer

7cda901

This was a rough quick-n-dirty query, and should get some qhelp as well at some point.

Python: Rename xml.sax test for consistency

4b03f5c

Python: Use concept tests for XML Parsing

faebaee

I was loosing my mind from looking through those .expected files Just going to take it one file at time, to make reviewing easier

Python: Port xml.dom tests

a7134ca

Python: Port xml.etree tests

5fb4c4d

Python: Port xml.sax tests

0b12d91

Python: Port xmltodict tests

c739ae4

Python: Move XML PoC to new test dir

2451123

Python: Handle more functions and kw-args

3278793

Python: Update XmlEntityInjection.expected

f72f673

I had forgotten about this, but better late than never... also added a small representative test

Python: Support feed method of lxml/xml.etree Parsers

33ebcdf

Python: Add test for XMLPullParser

46238d5

But handling this in a nice way will require some restructuring

RasmusWL added 7 commits March 3, 2022 21:34

Python: Align QLdocs of XML modeling

a033b71

Python: Restructure modeling of xml.etree parsers

c0a2c25

Python: Restructure lxml modeling

c0a6f9f

and handle parser being passed as positional argument

Python: Minor fixup of qldoc

df8e0fc

Python: Remove XMLParser concept

837daaa

Python: Minor qldoc improvement

0d69dc8

Python: Rename vulnerable predicate => vulnerableTo

3f6c55e

github-actions Bot added the Python label Mar 3, 2022