Skip to content

Add cookie management to HttpClient and improve standards compliance (Content-Type and PRG pattern)#14139

Merged
smcintyre-r7 merged 5 commits intorapid7:masterfrom
wvu:feature/http
Sep 16, 2020
Merged

Add cookie management to HttpClient and improve standards compliance (Content-Type and PRG pattern)#14139
smcintyre-r7 merged 5 commits intorapid7:masterfrom
wvu:feature/http

Conversation

@wvu
Copy link
Copy Markdown
Contributor

@wvu wvu commented Sep 15, 2020 •
edited
Loading

Superseded by #14831

Synopsis

Set opts['keep_cookies'] in send_request_cgi(!) to keep cookies from HTTP responses for reuse in HTTP requests.

Usage

This is an example from #14126:

res = send_request_cgi!({
  'method' => 'POST',
  'uri' => normalize_uri(target_uri.path, '/owa/auth.owa'),
  'vars_post' => {
    'username' => username,
    'password' => password,
    'flags' => '',
    'destination' => full_uri('/owa/', vhost_uri: true)
  },
  'keep_cookies' => true
}, datastore['HttpClientTimeout'], 2) # timeout and redirect_depth

Subsequent send_request_cgi(!) calls will use the cookies in the "cookie jar."

Reviewing

The abridged diff might be useful.

Testing

You may test against #14126.

References

Required by #14126. Fixes #12281, #12510, and #12916. See also: #13092.

@wvu wvu added library enhancement code quality Improving code quality labels Sep 15, 2020
@wvu wvu requested a review from smcintyre-r7 September 15, 2020 21:00
@wvu wvu force-pushed the feature/http branch 2 times, most recently from bc45e4e to 023c079 Compare September 15, 2020 21:14
@wvu
Copy link
Copy Markdown
Contributor Author

wvu commented Sep 15, 2020

Jenkins, test this, please.

@wvu wvu mentioned this pull request Sep 15, 2020
Merged
@wvu wvu force-pushed the feature/http branch 5 times, most recently from ef48b86 to 9c95a36 Compare September 16, 2020 02:32
@wvu wvu removed the request for review from smcintyre-r7 September 16, 2020 02:40
@wvu wvu force-pushed the feature/http branch 4 times, most recently from 0f9548b to a82466f Compare September 16, 2020 05:02
@smcintyre-r7
Copy link
Copy Markdown
Contributor

smcintyre-r7 commented Sep 16, 2020

I noticed in commit a946bdb you added a datastore option to configure whether or not cookies are kept. I'm concerned that a user may change that setting to a value that the module author hasn't accounted for and thus break the module. It seems like logic would need to be used to handle this setting and thus it should be left up to the module author.

Is there a particular use case you have in mind where that should be left up to the user?

@wvu
Copy link
Copy Markdown
Contributor Author

wvu commented Sep 16, 2020

That's fair. I was thinking of removing HttpPartialResponses, too. I'm not sure either of these options are good for the user to see, since they're mostly for the module dev.

@wvu
Don't expose HttpClient dev options to the user
3508ba2 
HttpKeepCookies and HttpPartialResponses have been removed.
@wvu
Copy link
Copy Markdown
Contributor Author

wvu commented Sep 16, 2020

Feels good to remove them! You're right, you can trust users to break things. (:

@wvu
Copy link
Copy Markdown
Contributor Author

wvu commented Sep 16, 2020

Jenkins, test this, please. Again.

smcintyre-r7
smcintyre-r7 approved these changes Sep 16, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@smcintyre-r7 smcintyre-r7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this in conjunction with the Exchange ECP DLP exploit (CVE-2020-16875 / PR #14126). Everything is working as intended and my comments have been addressed so I'm going to merge this in. Thanks!

@smcintyre-r7 smcintyre-r7 merged commit 4c1ce88 into rapid7:master Sep 16, 2020
@wvu
Copy link
Copy Markdown
Contributor Author

wvu commented Sep 16, 2020 •
edited
Loading

FWIW, there were no consumers of HttpPartialResponses, so I was okay removing it in this PR. Might make a sensible default in the future, since it's typically expected behavior in modules. Keeping cookies isn't necessarily, though.

@smcintyre-r7
Copy link
Copy Markdown
Contributor

smcintyre-r7 commented Sep 16, 2020 •
edited by pbarry-r7
Loading

Release Notes

Updated the HTTP client library that is used by many Metasploit modules to be more compliant across standards in regards to redirection handling. Also added a new feature to more easily manage cookies.

@wvu wvu deleted the feature/http branch September 16, 2020 20:06
@wvu
Copy link
Copy Markdown
Contributor Author

wvu commented Sep 16, 2020

Hope I didn't break master, lol. 🤞

@pbarry-r7 pbarry-r7 added the rn-enhancement release notes enhancement label Sep 29, 2020
@wvu wvu changed the title Add cookie management to HttpClient and improve standards compliance Add cookie management to HttpClient and improve standards compliance (PRG pattern) Apr 14, 2021
adfoster-r7
adfoster-r7 reviewed Apr 30, 2021
View reviewed changes
lib/msf/core/exploit/http/client.rb
Comment on lines +93 to +94
# Initialize an empty cookie jar to keep cookies
self.cookie_jar = Set.new
Copy link
Copy Markdown
Contributor

@adfoster-r7 adfoster-r7 Apr 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For future travellers, this implementation was superseded by
#14831

@wvu wvu mentioned this pull request Apr 30, 2021
Merged
4 tasks
@wvu wvu changed the title Add cookie management to HttpClient and improve standards compliance (PRG pattern) Add cookie management to HttpClient and improve standards compliance (Content-Type and PRG pattern) May 6, 2021
@wvu wvu mentioned this pull request Dec 20, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adfoster-r7 adfoster-r7 adfoster-r7 left review comments

@smcintyre-r7 smcintyre-r7 smcintyre-r7 approved these changes

Assignees

@smcintyre-r7 smcintyre-r7

Labels

code quality Improving code quality enhancement library rn-enhancement release notes enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

send_request_cgi! redirection is only good for GET

4 participants

@wvu @smcintyre-r7 @adfoster-r7 @pbarry-r7