Rubocop recently landed modules #13261

adfoster-r7 · 2020-04-16T01:08:27Z

Let's run Rubocop on recently landed modules to ensure we're still happy with the output. This is a pre-requisite to enabling Rubocop as part of CI runs.

Command ran:

rubocop -a modules/auxiliary/scanner/http/limesurvey_zip_traversals.rb modules/exploits/linux/http/nexus_repo_manager_el_injection.rb modules/exploits/linux/http/pandora_ping_cmd_exec.rb modules/exploits/linux/http/vestacp_exec.rb modules/exploits/linux/misc/tplink_archer_a7_c7_lan_rce.rb modules/exploits/multi/http/liferay_java_unmarshalling.rb modules/exploits/multi/http/playsms_template_injection.rb modules/exploits/osx/local/vmware_fusion_lpe.rb modules/exploits/unix/webapp/thinkphp_rce.rb modules/exploits/windows/local/cve_2020_0796_smbghost.rb modules/post/windows/gather/credentials/teamviewer_passwords.rb modules/post/windows/manage/execute_dotnet_assembly.rb modules/auxiliary/scanner/rdp/cve_2019_0708_bluekeep.rb modules/exploits/linux/local/exim4_deliver_message_priv_esc.rb modules/exploits/unix/smtp/opensmtpd_mail_from_rce.rb modules/exploits/windows/http/file_sharing_wizard_seh.rb modules/exploits/windows/misc/ais_esel_server_rce.rb modules/exploits/windows/misc/crosschex_device_bof.rb modules/exploits/windows/smb/ms17_010_eternalblue.rb modules/exploits/windows/telnet/goodtech_telnet.rb modules/payloads/singles/windows/pingback_reverse_tcp.rb modules/post/windows/manage/sshkey_persistence.rb

adfoster-r7 · 2020-04-16T01:24:39Z

modules/exploits/multi/http/liferay_java_unmarshalling.rb

    version, build = res.headers['Liferay-Portal'].scan(
      /^Liferay.*Portal ([\d.]+.*GA\d+).*Build (\d+)/
    ).flatten

-    unless version && (build = Integer(build) rescue nil)
+    unless version && (build = begin


This formats weirdly IMO. But I think the side effect in a conditional statement that impacts code later on is a bit of a gotcha, and probably lives in the above line to clearly show the side effect that's being made.

Or alternatively I think this can safely be written as (build = build.to_i) due to the above regex matching only 1 or more digits.

~~I agree that to_i can safely be used here.~~

I don't think so:

> nil.to_i 0

Which will return Appears even when the regex does not match.

Agree that to_i fix shown above would be better, this one just looks a bit too weird for me imho.

Yeah, I went back and looked at how I wrote this method, and I was grabbing the version and build, simultaneously validating them with the regex, and THEN checking for nil and any potentially invalid integers (despite regex), all while assigning the build variable.

My code is getting more dense these days, and I'm getting dumber. I may want to break it up more and comment it better, huh?

Woops didn't see @acammack-r7's feedback before posting mine due to a bug with GitHub not updating whilst I was reviewing. Ignore my comment then.

I would have rather rewritten this section than included RuboCop's terrible formatting.

wvu · 2020-04-16T01:35:16Z

This conflicts with #13259, which contains manual changes. I would rather run RuboCop against my modules after that lands. Unless your intention is to create more work for me. ;)

wvu · 2020-04-16T01:38:24Z

modules/exploits/multi/http/liferay_java_unmarshalling.rb

-      Liferay-Portal: Liferay Community Edition Portal 7.2.0 CE GA1 (Mueller / Build 7200 / June 4, 2019)
-      [snip]
-=end
+    #     Building the Liferay-Portal header:


Since we're not using block comments, the extra indentation here is even more unnecessary.

Agreed; I think Rubocop was playing it safe here when automatically fixing this.

I'm on the fence about block comments, but they're really useful for pasting in structured information.

Also agreed! The docs shed a bit of light on the decision:

https://rubocop.readthedocs.io/en/stable/cops_style/#styleblockcomments

https://rubystyle.guide/#no-block-comments

Yeah I agree on this one, this seems unnecessary and seems like RuboCop was playing it too safe on this.

This was not fixed.

wvu · 2020-04-16T01:38:51Z

modules/exploits/unix/webapp/thinkphp_rce.rb

-  * Connection #0 to host 127.0.0.1 left intact
-  5.0.20wvu@kharak:~$
-=end
+  #   wvu@kharak:~$ curl -vs "http://127.0.0.1:8080/index.php?s=$((RANDOM))" | xmllint --html --xpath 'substring-after(//div[@class = "copyright"]/span[1]/text(), "V")' -


https://github.com/rapid7/metasploit-framework/pull/13261/files#r409230015

wvu · 2020-04-16T01:39:09Z

modules/exploits/unix/webapp/thinkphp_rce.rb

-  uid=33(www-data) gid=33(www-data) groups=33(www-data)
-  wvu@kharak:~$
-=end
+  #   wvu@kharak:~$ curl -gvs "http://127.0.0.1:8080/index.php?s=/Index/\think\app/invokefunction&function=call_user_func_array&vars[0]=system&vars[1][]=id" | head -1


https://github.com/rapid7/metasploit-framework/pull/13261/files#r409230015

wvu · 2020-04-16T01:39:18Z

modules/exploits/unix/webapp/thinkphp_rce.rb

-  uid=33(www-data) gid=33(www-data) groups=33(www-data)
-  wvu@kharak:~$
-=end
+  #   wvu@kharak:~$ curl -vsd "_method=__construct&filter[]=system&method=get&server[REQUEST_METHOD]=id" http://127.0.0.1:8081/index.php?s=captcha | head -1


https://github.com/rapid7/metasploit-framework/pull/13261/files#r409230015

adfoster-r7 · 2020-04-16T15:35:46Z

This conflicts with #13259, which contains manual changes. I would rather run RuboCop against my modules after that lands.

@wvu-r7 100% happy to do this, let me know which files you'd like reverted 👍 Or I can land this after your module ships

gwillcox-r7

Overall looks good. A few changes I didn't think were right but majority looks good. Sorry for extra comments, most of them were pointing out examples of where I thought things were going right for reference in case anyone wanted to discuss them more.

modules/auxiliary/scanner/http/limesurvey_zip_traversals.rb

modules/exploits/linux/http/vestacp_exec.rb

modules/exploits/osx/local/vmware_fusion_lpe.rb

modules/post/windows/manage/execute_dotnet_assembly.rb

gwillcox-r7 · 2020-04-16T15:50:01Z

modules/exploits/multi/http/liferay_java_unmarshalling.rb

    version, build = res.headers['Liferay-Portal'].scan(
      /^Liferay.*Portal ([\d.]+.*GA\d+).*Build (\d+)/
    ).flatten

-    unless version && (build = Integer(build) rescue nil)
+    unless version && (build = begin


Agree that to_i fix shown above would be better, this one just looks a bit too weird for me imho.

wvu · 2020-04-16T20:07:45Z

@adfoster-r7: I usually try to maintain some level of consistency between my modules, which leads to situations like #13259. We otherwise don't see much backporting of fixes or changes to old modules. It's seen as risky.

If this PR lands, it'll cause a lot of conflicts with the manual work I did in #13259, which I may have to redo. You can either redo the RuboCop run on my modules after the other PR lands, or I can revert your commits to my modules, reapply my changes, and then RuboCop again.

I plan to do a RuboCop sweep on my modules, at least the more recent ones we can test, since this train is going to keep rolling. Maybe it'll give me some release. We get more consistency between modules, and I can burn some "foolish consistencies." :)

gwillcox-r7 · 2020-04-16T21:01:11Z

@adfoster-r7: I usually try to maintain some level of consistency between my modules, which leads to situations like #13259. We otherwise don't see much backporting of fixes or changes to old modules. It's seen as risky.

If this PR lands, it'll cause a lot of conflicts with the manual work I did in #13259, which I may have to redo. You can either redo the RuboCop run on my modules after the other PR lands, or I can revert your commits to my modules, reapply my changes, and then RuboCop again.

I plan to do a RuboCop sweep on my modules, at least the more recent ones we can test, since this train is going to keep rolling. Maybe it'll give me some release. We get more consistency between modules, and I can burn some "foolish consistencies." :)

Personally I vote for landing the other one first then this one. We get another PR and exploit out, customers are happier, then can go back retrospectively and fix up stuff like this. That other PR has already got a lot of commits as it is so I'd rather land that before it turns into a larger mess than it already is, then apply RuboCop edits after we have finalized the last few sticking points on this PR.

Unless we are in favor of doing less PRs in which case it would make sense to do it the other way around and have this land first.

wvu · 2020-04-16T21:09:34Z

I will need to get to it after #13253 is ready. Apologies. I addressed all but one issue last night in #13259.

gwillcox-r7 · 2020-04-16T21:22:20Z

Will wait for @wvu-r7 to finish his edits, I think he meant to quote #13259, which has one issue left.

wvu · 2020-04-16T21:23:54Z

Thanks, corrected. :)

wvu · 2020-04-17T03:28:19Z

We are ready for you, @adfoster-r7. #13259 has landed. :)

gwillcox-r7 · 2020-04-17T05:09:12Z

@adfoster-r7 Once merge conflicts are resolved on your end and you've reuploaded, ping me and I'll get this merged into master.

adfoster-r7 · 2020-04-17T11:02:58Z

I've rebased + Rubocop'd again 👍

gwillcox-r7 · 2020-04-17T14:04:19Z

@adfoster-r7 Thanks, I'll get this merged into master now :)

wvu · 2020-04-17T15:26:05Z

So what was the decision on block comments? Yea or nay?

wvu · 2020-04-17T15:31:25Z

I've fixed the outstanding RuboCop mistakes in #13274. Please review and land.

adfoster-r7 · 2020-04-17T15:44:32Z

@wvu-r7 Let's go with nay for now, and if it ends up being a complete pain - we can revise 👍

wvu · 2020-04-17T15:47:35Z

It's actually less of a pain if RuboCop makes the change, haha. Woohoo, automation.

adfoster-r7 added rn-no-release-notes no release notes automation New APIs and services for automation code quality Improving code quality labels Apr 16, 2020

adfoster-r7 force-pushed the rubocop-recently-landed-modules branch 3 times, most recently from f2be156 to 540b1f1 Compare April 16, 2020 01:12

adfoster-r7 commented Apr 16, 2020

View reviewed changes

wvu reviewed Apr 16, 2020

View reviewed changes

wvu mentioned this pull request Apr 16, 2020

Backport fixes to various modules committed between Nexus and Liferay PRs #13259

Merged

gwillcox-r7 suggested changes Apr 16, 2020

View reviewed changes

adfoster-r7 added the blocked Blocked by one or more additional tasks label Apr 16, 2020

gwillcox-r7 approved these changes Apr 16, 2020

View reviewed changes

gwillcox-r7 self-assigned this Apr 17, 2020

Rubocop recently landed modules

f2c3fc5

adfoster-r7 force-pushed the rubocop-recently-landed-modules branch from 540b1f1 to f2c3fc5 Compare April 17, 2020 10:55

gwillcox-r7 merged commit f29b4e1 into rapid7:master Apr 17, 2020

wvu mentioned this pull request Apr 17, 2020

Fix RuboCop's mistakes in recently landed modules #13274

Merged

adfoster-r7 mentioned this pull request Apr 20, 2020

Thoughts on enabling https://houndci.com/ on Pull Requests? #11542

Closed

wvu mentioned this pull request Apr 22, 2020

Add rubocop rules to consistently format modules #12990

Merged

Rubocop recently landed modules #13261

Rubocop recently landed modules #13261

Conversation

adfoster-r7 commented Apr 16, 2020

Choose a reason for hiding this comment

wvu Apr 16, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wvu Apr 16, 2020 • edited

Choose a reason for hiding this comment

gwillcox-r7 Apr 16, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wvu commented Apr 16, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adfoster-r7 commented Apr 16, 2020

gwillcox-r7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wvu commented Apr 16, 2020 • edited

gwillcox-r7 commented Apr 16, 2020 • edited

wvu commented Apr 16, 2020 • edited

gwillcox-r7 commented Apr 16, 2020

wvu commented Apr 16, 2020

wvu commented Apr 17, 2020

gwillcox-r7 commented Apr 17, 2020

adfoster-r7 commented Apr 17, 2020

gwillcox-r7 commented Apr 17, 2020

wvu commented Apr 17, 2020

wvu commented Apr 17, 2020

adfoster-r7 commented Apr 17, 2020

wvu commented Apr 17, 2020

wvu Apr 16, 2020 •

edited

wvu Apr 16, 2020 •

edited

gwillcox-r7 Apr 16, 2020 •

edited

wvu commented Apr 16, 2020 •

edited

wvu commented Apr 16, 2020 •

edited

gwillcox-r7 commented Apr 16, 2020 •

edited

wvu commented Apr 16, 2020 •

edited