XXE: Changes for review #9
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
reviewed
Mar 3, 2022
jorgectf
reviewed
Mar 3, 2022
jorgectf
reviewed
Mar 3, 2022
jorgectf
reviewed
Mar 3, 2022
jorgectf
reviewed
Mar 3, 2022
jorgectf
reviewed
Mar 3, 2022
jorgectf
reviewed
Mar 3, 2022
jorgectf
reviewed
Mar 3, 2022
python/ql/src/experimental/Security/CWE-611/SimpleXmlRpcServer.ql
Outdated
Show resolved
Hide resolved
jorgectf
reviewed
Mar 3, 2022
python/ql/src/experimental/Security/CWE-611/SimpleXmlRpcServer.ql
Outdated
Show resolved
Hide resolved
jorgectf
reviewed
Mar 3, 2022
python/ql/src/experimental/Security/CWE-611/SimpleXmlRpcServer.ql
Outdated
Show resolved
Hide resolved
jorgectf
reviewed
Mar 3, 2022
|
Outstanding rewrite 🤯. I've commited some qldocs' typos and made a very little polishing.
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 <[email protected]>
Nice spotted @jorgectf!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Update for github#6112
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
.expectedfile 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).