New feature for web form module: HTTP response codes, and a big cleanup#63
Open
martijnf1 wants to merge 16 commits into
Open
New feature for web form module: HTTP response codes, and a big cleanup#63martijnf1 wants to merge 16 commits into
martijnf1 wants to merge 16 commits into
Conversation
added 4 commits
March 8, 2024 16:01
…e index() man page
handling for for 301,302,307 and 308 which are all redirects. The default behaviour is to retrieve the resource specified at the Location header and continue with the response from that.
readable than before. Some static testing verified that it works... but some more testing should be done to make sure.
…string because it's hidden under a function call
Author
|
A shower-thought struck me: We're explicitly passing a |
Owner
|
Hi @martijnf1 Thanks for the pull request. Unfortunately, my ability to review and test this is limited. I have not historically used the web-form module, nor do I have a good environment to thoroughly test it in. However, I did notice a couple of items:
I no longer use Medusa on a daily basis. However, I do expect to tag a release in the next week to get the SMBv2 bits I worked on out there. I'm happy to merge this in if you get those items cleaned up and additional testing on your end looks good. Joe |
added 2 commits
March 25, 2024 10:38
… as missing while the module is successfully compiled with openssl support
…ng modules (medusa -q) shows a consistent message. solved double free by simply removing the free and making the caller responsible for freeing the pointer, which is what it was anyway. My bad.
Author
|
Hi, Thanks for reviewing. I've addressed the problems:
|
Owner
|
My initial testing is showing some issues. For example, against a simple Airsonic install: ./medusa -M web-form -n 8080 -h 192.168.10.10 -u admin -p admin -v 99 -w 99 -m FORM:/airsonic/login -m FORM-DATA:"post?j_username=&j_password=&_csrf=5efb72d6-fca9-4df1-b5e6-c327a3827a58&submit=Log+in" -m DENY-SIGNAL:Incorrect -m CUSTOM-HEADER:"Cookie: JSESSIONID=4BB07CD420D9B9F7E0CCC3DD08DAEABA" I'm getting this error when trying to free parameters at line 787: DEBUG MODULE [A8E1C6C0]: [web-form.mod] HTTP Response code was 302. It's also doing a POST on the redirect, when it'd need to be a GET for the testing to actually work. |
Author
|
Thanks for adressing any problems you encounter, i will look into this later in the week. For response codes i've followed the specification on the RFC 9110, it is stated there that for 301 and 302, the request MAY be changed from POST to GET, for 307 and 308 this behaviour is forbidden. Changing the method from POST to GET means that POST-body data is lost, because according to the same spec, it shoud be ignored in GET requests. i'll assume that is commonplace. |
…rtijnf1/medusa into feature-web-form-module-response-code
1. Typos in comments 2. newModuleData() function initialises all members of the struct to either 0 or NULL, which is implicit in the calloc, therefore the explicit assignments are removed. 3. Added break statements to parseHttpStatusCode. This is just for sanity because the status codes i put there are actually not implemented i.e. for future extension. Functionality does not change 4. Removed getLocationHeaderValue to not make the code overly specific. 5. Removed the check that tests whether the number of arguments that is given is between 0 and 5. It is nonsensical to test only the number of arguments given since i can provide only CUSTOM-HEADER arguments, or give a valid set of arguments with 10 CUSTOM-HEADERs which will make the test fail. 6. Made testing for NULL values explicit. `if (x)` and `if (x != NULL)` are equal but the latter is more clear and makes sense from a type-perspective. 7. GET_REQUEST_FMT_STR had an erroneous extra "%s" in the format string which caused GET requests to be mangled 8. The Location header can return relative or absolute paths. For now i made a simple distinction between the two and concatenate a relative path onto the old path, so path resolution is `outsourced' to the server. This is not elegant but it'll do, for now.
- HTTP_UNAUTHORIZED was spelled wrong, - HTTP_MOVED_PERMANENTLY was not included in the status code parser. - the _findHeaderValue function has been rewritten to coop with a wider range of Location header formatsm mostly including whitespace. - an initialised flag has been added to moduledata so that expensive string functions are skipped after the first initialisation - Following a redirect changes requests type from POST to GET, but restores it to POST on successive requests by using an additional flag. - Noted that urlencode DOES NOT work with unicode text, this medusa plugin is only compatible with ascii. using unicode WILL crash the application because mallocs will fail deep down. -
- Instead of checking whether a path is relative or absolute, try to interpret the actual type in a separate fuction. - A new module state, MSTATE_INITIALIZE, has been added. This state is for code that is only supposed to run once, instead of every run. This replaces the moduleData->initialised flag that i put in earlier. - the connect() method (medusaConnect or medusaConnectSSL) is chosen only once, instead of every cycle. - Fixed an error in the default switch(nState) case where the state was not set to MSTATE_EXITING after encountering an unknown module state.
is.
- POST to GET redirections set cookies, so the reponse has to be
searched for Set-Cookie headers and those cookies have to be
set.
- Removed a few superfluous strdups to make the code more
efficient.
The code is very heap-intensive and a lot of allocations happen on every
cycle. it's probably best to address this in the future by reserving a
single block of memory that is large enough for all subsequent requests
and rebuild the request string there.
different file because it became too large, and maybe it is useful to other modules as well. It seems to work for all cases that i've been able to throw at it. I'd like some testing feedback. The speed of these path resolutions with my new code are ~120M/s on a single core of a high end dell laptop. so it is not likely to be a bottleneck.
Author
|
Could you rerun your test with my additions? I'm reasonably certain that it should work now. |
Owner
|
I realize it's been a year since I've looked at this... The example from above still fails out. In this case, it looks like the Location: returned in the 302 isn't sent on the next request (i.e., "?error" is omitted). |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
I am a Dutch security tester and c programmer and stumbled over a limitation of the web-form module: Not being able to handle different HTTP status codes, specifically redirects. I decided to implement that functionality: If a status 301, 302, 307 or 308 is encountered then the Location header is read, and the requested resource is updated to match it's value. The request is then sent again.
The code was rather bloated, so i refactored it quite a bit and documented parts of the code while i worked on it. I also removed some deprecated functions such as index() and replaced unbounded string functions with bounded ones wherever possible.
I have used my new version succesfully, but i have not tested it in depth. I think we would all benefit if some people reviewed and tested my code as well before merging this to the master branch.