-
Notifications
You must be signed in to change notification settings - Fork 39
Change encoding for SP character #663
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@Mzack9999 PTAL :) |
|
Hi @ehsandeep @dogancanbakir |
According to RFC 1866 and RFC 3986, the `SP` character can only be encoded as `+` in application/x-www-form-urlencoded values. But encoding it as `%20` is always correct. Therefore, prefer to encode with `%20`.
|
Updated test cases and verified TC passes on my machine. |
|
Hello @mjkim610 thanks a lot ! It should indeed resolve a part of my issue too (#6342) Yet, when text/plain Space we should let as real Space not? In the changes I cannot see that? I think the code may have forgot this case ? |
|
Maybe we might add an option like encoding_type ? To cover all the case (plain, url, www-form,...)? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey @mjkim610 , thanks for the pr, but url utils in this repo are not meant to follow any RFC's and are implemented on case to case basis based on the requirement of the poc being written ( apart from that we have many other optimizations that are not part of any RFC https://github.com/projectdiscovery/utils/tree/main/url )
this ordereparams is used for url encoding throughout nuclei ( in request body / url etc ) , so any change here will most likely break existing nuclei-templates and integration tests ,
can you share any exploit / poc where you are able to exploit a particular vulnerability via %20 but not + , if so we can plan some changes where we can support that without breaking existing templates
|
@tarunKoyalwar see for example projectdiscovery/nuclei#6342 for an example where encoding is not good and consider an XXE for example in this example |
|
Or maybe just authorize unsafe: true in fuzzing? It will resolve a big part of it |
|
@tarunKoyalwar thanks for the feedback. To answer your question regarding POCs that require
Regarding your concern about breaking integration tests:
Regarding your concern about breaking existing templates:
|
|
@tarunKoyalwar could you provide feedback? :) I think the best course of action would be to set |
|
@mjkim610 Unfortunately we can't change the default behavior for edge cases. I've added an environment variable |
Thanks for providing a way to change space encoding type! Although I have some reservations about using env vars, I understand why, given that implementing it in other methods (e.g. flags) would require lots of planning and big changes. I will keep an eye out for any updates regarding these encoding issues. |
According to RFC 1866 and RFC 3986, the
SPcharacter can only be encoded as+in application/x-www-form-urlencoded values. But encoding it as%20is always correct. Therefore, prefer to encode with%20.Related nuclei issue: projectdiscovery/nuclei#6162
I tested it locally with a simple case, but much more thorough testing is required.