Metasploit-framework: Thoughts on enabling https://houndci.com/ on Pull Requests?

[busterb]

I'd like to make it easier for contributors to have a positive experience without anyone having to pick on Ruby syntax or code style, but instead focusing on the meat of the contribution. To that end, I'd like to enable a tool like https://houndci.com/ which adds a bot to do the initial review of style / code changes based on what we have in .rubycop.yaml instead of needing people to do it.

I think it would make initial reviews a lot more self-service for new and old contributors, and allow folks to spend more time on the things humans do best. I believe HoundCI does have an internal limit on how many things it will comment on, which is probably good since rubocop can be really noisy sometimes.

I have more rubocop rule changes that I think will help make it even less noisy, especially since a lot of Metasploit's codebase was not developed with this enabled. I don't want to necessarily force people to have to reformat some well-tested library code just because a one-line change triggered a full-fledged bot review. Hopefully things can be focused and not introduce extra churn.

Any thoughts or suggestions for other tools we might integrate?

[bcoles]

I believe Travis CI supports rubocop to some extent? (unverified)

If we go this route, the .rubocop.yml configuration file will require a fair amount of pruning.

Running rubocop over modules/exploits/unix/webapp/ on master produces:

137 files inspected, 4393 offenses detected

A rate of approximately 30 violations per module.

Even with your modified config in #11543, Rubocop produces:

137 files inspected, 4390 offenses detected

Style is somewhat subjective. The style guide usually makes sense, but not always.

[busterb]

I see 6894 offenses for that same scan, which probably means mine is even more strict :(

I think the web review tools only lint around the code that changes though. So, for a new module, yeah we need to make it possible to get past the linter without too much effort. But I don't believe it's going to throw a hundred errors on old code for a one-liner. Needs to be tested though.

[jhart-r7]

I like the idea if we can keep .rubocop.yaml updated. Does hound handle situations where existing code is modified and it only mentions style issues in modified code? Otherwise for a simple 1-line change to an old module would result in hound commenting on 400 irrelevant lines.

[bcoles]

Otherwise for a simple 1-line change to an old module would result in hound commenting on 400 irrelevant lines.

Easily fixed:

rubocop --auto-correct lib modules
git add lib modules
git commit -m "style"
git push origin master

Can't have 400 violations if 400 violations don't already exist. Practice what you preach. This will also avoid the "but Metasploit already does it like this so I'm not changing it" argument.

[gschneider-r7]

You can use rubocop-diff in travis-ci or whatever to give usefully bite-sized actionable reports. Bonus points if you find a tool that will post them into the Checks tab on PRs.

Hound can get very noisy and become overwhelming on big PRs, but I think it's slightly improved with the Github review system changes in the last year. It would be much better if they adopt the Checks API: https://github.com/houndci/hound/issues/1536

BTW hound only comments on changed lines, which is nice. But if you rebase your branch it will re-comment the same things (assuming you didn't fix them yet) which is annoying.

[wvu-r7]

I've been working on my own .rubocop.yml after #11527. It would be nice to combine forces on this. I approve of this direction.

[busterb]

For reference, here are the highest violated cops, and given the vast majority are just layout.

   1020 Style/BlockDelimiters:
   1032 Style/PerlBackrefs:
   1074 Style/MethodCallWithoutArgsParentheses:
   1079 Layout/SpaceInsideBlockBraces:
   1130 Layout/MultilineArrayBraceLayout:
   1142 Style/RedundantParentheses:
   1188 Layout/CaseIndentation:
   1193 Layout/EmptyLinesAroundClassBody:
   1274 Lint/UnusedMethodArgument:
   1309 Style/BarePercentLiterals:
   1379 Performance/RegexpMatch:
   1451 Style/UnpackFirst:
   1682 Layout/MultilineOperationIndentation:
   1745 Layout/EmptyLineBetweenDefs:
   2040 Layout/Tab:
   2104 Style/Not:
   2123 Lint/UselessAssignment:
   2192 Layout/EmptyLineAfterGuardClause:
   2234 Layout/SpaceAroundEqualsInParameterDefault:
   2486 Style/LineEndConcatenation:
   2515 Layout/EmptyLinesAroundModuleBody:
   2515 Style/RandomWithOffset:
   2682 Layout/EmptyLines:
   2709 Style/TrailingCommaInHashLiteral:
   2711 Layout/EmptyLinesAroundMethodBody:
   3014 Style/BracesAroundHashParameters:
   3049 Layout/ExtraSpacing:
   3127 Layout/SpaceInsideHashLiteralBraces:
   3354 Style/ParenthesesAroundCondition:
   3401 Style/PercentLiteralDelimiters:
   3876 Layout/LeadingCommentSpace:
   3939 Style/RedundantSelf:
   3949 Layout/SpaceInsideParens:
   4262 Layout/MultilineMethodCallBraceLayout:
   4378 Style/AndOr:
   4597 Layout/IndentationWidth:
   4654 Style/TrailingCommaInArrayLiteral:
   4699 Layout/IndentArray:
   4736 Layout/IndentHash:
   7825 Layout/SpaceAroundOperators:
   7833 Style/HashSyntax:
  27646 Layout/SpaceInsideArrayLiteralBrackets:
  42445 Layout/AlignHash:
  57862 Layout/SpaceAfterComma:

Even meeting in the middle with rubocop --fix-layout --safe-auto-correct lib modules would banish a lot of the noise.

[wvu-r7]

For me, Layout/AlignHash is my worst offender, since I'm a serial alignist (I wasn't before).

[ccondon-r7]

@busterb Still interested in this? cc @adfoster-r7

[adfoster-r7]

Seems like a good idea to me, happy to integrate this :+1:

[ccondon-r7]

@busterb and @wvu-r7 , if you guys have additional style or code rules you want included in scope for this integration, please make sure they're in .rubycop.yaml as soon as possible. Cheers, all.

[wvu-r7]

The number of exceptions we've added (or would add) to .rubocop.yml continues to grow, and some of the current rules still produce absolutely bizarre output, mostly in the info hash, once autocorrected.

It would be nice to decide on a middle ground, at least in the info hash, before unleashing the hound on contributors, ourselves included. Thoughts on how to decide which rules to include?

I have a bias toward my own personal style, but I'm willing to change my style to suit a more consistent Metasploit.

[ccondon-r7]

The time is now, @wvu-r7! The goal is to make things easier for contributors, so if you have some suggestions on adding OR subtracting, feel free to make 'em.

[wvu-r7]

Here's what the info hash looks like with rubocop -x --safe-auto-correct modules/exploits/windows/smb/ms17_010_eternalblue.rb _with our current_ .rubocop.yml:

  def initialize(info = {})
    super(update_info(info,
                      'Name'           => 'MS17-010 EternalBlue SMB Remote Windows Kernel Pool Corruption',
                      'Description'    => %q(
        This module is a port of the Equation Group ETERNALBLUE exploit, part of
        the FuzzBunch toolkit released by Shadow Brokers.

        There is a buffer overflow memmove operation in Srv!SrvOs2FeaToNt. The size
        is calculated in Srv!SrvOs2FeaListSizeToNt, with mathematical error where a
        DWORD is subtracted into a WORD. The kernel pool is groomed so that overflow
        is well laid-out to overwrite an SMBv1 buffer. Actual RIP hijack is later
        completed in srvnet!SrvNetWskReceiveComplete.

        This exploit, like the original may not trigger 100% of the time, and should be
        run continuously until triggered. It seems like the pool will get hot streaks
        and need a cool down period before the shells rain in again.

        The module will attempt to use Anonymous login, by default, to authenticate to perform the
        exploit. If the user supplies credentials in the SMBUser, SMBPass, and SMBDomain options it will use
        those instead.

        On some systems, this module may cause system instability and crashes, such as a BSOD or
        a reboot. This may be more likely with some payloads.
      ),

                      'Author' =>
                      [
                        'Sean Dillon <[email protected]>',  # @zerosum0x0
                        'Dylan Davis <[email protected]>',  # @jennamagius
                        'Equation Group',
                        'Shadow Brokers',
                        'thelightcosine' # RubySMB refactor and Fallback Credential mode
                      ],
                      'License'        => MSF_LICENSE,
                      'References'     =>
                        [
                          ['MSB', 'MS17-010'],
                          ['CVE', '2017-0143'],
                          ['CVE', '2017-0144'],
                          ['CVE', '2017-0145'],
                          ['CVE', '2017-0146'],
                          ['CVE', '2017-0147'],
                          ['CVE', '2017-0148'],
                          ['URL', 'https://github.com/RiskSense-Ops/MS17-010']
                        ],
                      'DefaultOptions' =>
                        {
                          'EXITFUNC' => 'thread',
                          'CheckModule' => 'auxiliary/scanner/smb/smb_ms17_010',
                          'WfsDelay' => 5
                        },
                      'Privileged'     => true,
                      'Payload'        =>
                        {
                          'Space'        => 2000, # this can be more, needs to be recalculated
                          'EncoderType'  => Msf::Encoder::Type::Raw
                        },
                      'Platform'       => 'win',
                      'Targets'        =>
                        [
                          [
                            'Windows 7 and Server 2008 R2 (x64) All Service Packs',
                            {
                              'Platform'       => 'win',
                              'Arch'           => [ARCH_X64],
                              'os_patterns'    => ['Server 2008 R2', 'Windows 7', 'Windows Embedded Standard 7'],
                              'ep_thl_b'       => 0x308,  # EPROCESS.ThreadListHead.Blink offset
                              'et_alertable'   => 0x4c,   # ETHREAD.Alertable offset
                              'teb_acp'        => 0x2c8,  # TEB.ActivationContextPointer offset
                              'et_tle'         => 0x420   # ETHREAD.ThreadListEntry offset
                            }
                          ]
                        ],
                      'DefaultTarget'  => 0,
                      'DisclosureDate' => 'Mar 14 2017',
                      'Notes'          =>
                        {
                          'AKA' => ['ETERNALBLUE']
                        }))

It looks kinda wild.

[ccondon-r7]

Neato! @dwelch-r7 may have some opinions on what should change for optimum developer and user experience, too—why don't you three (@adfoster-r7 ) have a chat when you get a chance to enumerate problems? No super rush on this, it's just in the backlog anyway.

[adfoster-r7]

@wvu-r7 I'll double check, but I think I'd be able to create a custom rubocop rule to force update_info and info onto their own lines, that way rubocop will autoformat the rest as:

    super(
      update_info(
        info,
        'Name'           => 'MS17-010 EternalBlue SMB Remote Windows Kernel Pool Corruption',
        'Description'    => %q(
        This module is a port of the Equation Group ETERNALBLUE exploit, part of
        the FuzzBunch toolkit released by Shadow Brokers.

        There is a buffer overflow memmove operation in Srv!SrvOs2FeaToNt. The size
        is calculated in Srv!SrvOs2FeaListSizeToNt, with mathematical error where a
        DWORD is subtracted into a WORD. The kernel pool is groomed so that overflow
        is well laid-out to overwrite an SMBv1 buffer. Actual RIP hijack is later
        completed in srvnet!SrvNetWskReceiveComplete.

        This exploit, like the original may not trigger 100% of the time, and should be
        run continuously until triggered. It seems like the pool will get hot streaks
        and need a cool down period before the shells rain in again.

        The module will attempt to use Anonymous login, by default, to authenticate to perform the
        exploit. If the user supplies credentials in the SMBUser, SMBPass, and SMBDomain options it will use
        those instead.

        On some systems, this module may cause system instability and crashes, such as a BSOD or
        a reboot. This may be more likely with some payloads.
      ),

        'Author' =>
        [
          'Sean Dillon <[email protected]>',  # @zerosum0x0
          'Dylan Davis <[email protected]>',  # @jennamagius
          'Equation Group',
          'Shadow Brokers',
          'thelightcosine' # RubySMB refactor and Fallback Credential mode
        ],
        'License'        => MSF_LICENSE,
        'References'     =>
          [
            ['MSB', 'MS17-010'],
            ['CVE', '2017-0143'],
            ['CVE', '2017-0144'],
            ['CVE', '2017-0145'],
            ['CVE', '2017-0146'],
            ['CVE', '2017-0147'],
            ['CVE', '2017-0148'],
            ['URL', 'https://github.com/RiskSense-Ops/MS17-010']
          ],
        'DefaultOptions' =>
          {
            'EXITFUNC' => 'thread',
            'CheckModule' => 'auxiliary/scanner/smb/smb_ms17_010',
            'WfsDelay' => 5
          },
        'Privileged'     => true,
        'Payload'        =>
          {
            'Space'        => 2000, # this can be more, needs to be recalculated
            'EncoderType'  => Msf::Encoder::Type::Raw
          },
        'Platform'       => 'win',
        'Targets'        =>
          [
            [
              'Windows 7 and Server 2008 R2 (x64) All Service Packs',
              {
                'Platform'       => 'win',
                'Arch'           => [ARCH_X64],
                'os_patterns'    => ['Server 2008 R2', 'Windows 7', 'Windows Embedded Standard 7'],
                'ep_thl_b'       => 0x308,  # EPROCESS.ThreadListHead.Blink offset
                'et_alertable'   => 0x4c,   # ETHREAD.Alertable offset
                'teb_acp'        => 0x2c8,  # TEB.ActivationContextPointer offset
                'et_tle'         => 0x420   # ETHREAD.ThreadListEntry offset
              }
            ]
          ],
        'DefaultTarget'  => 0,
        'DisclosureDate' => 'Mar 14 2017',
        'Notes'          =>
          {
            'AKA' => ['ETERNALBLUE']
          }
      )
    )

It's still not perfect; but it seems better IMO. Let me know your thoughts :+1:

[wvu-r7]

That would be awesome. You have now obligated yourself into being our RuboCop tuner. :P

[bcook-r7]

Looks good to me @adfoster-r7,

[adfoster-r7]

@wvu-r7 I've spiked a custom rubocop rule under this PR - https://github.com/rapid7/metasploit-framework/pull/12990

The first commit is the added rule, and the second commit is the output of rubocop -x --safe-auto-correct modules/exploits :+1:

[adfoster-r7]

Here's my latest thoughts on this ticket:

Improving RuboCop

The pre-requisite to enabling HoundCI is ensuring we're happy with Rubocop's advice. These PRs are the start of fully relying on Rubocop for automated code linting:

• https://github.com/rapid7/metasploit-framework/pull/12990 - Adding custom Rubocop rules for modules
• https://github.com/rapid7/metasploit-framework/pull/13261 - Additional example of running Rubocop on recently landed modules

The final steps are to integrate Rubocop into msftidy, which we have an internal ticket for.

Enabling HoundCI

Regarding HoundCI, I think the output of HoundCI isn't quite what we want for first time users. It's quite verbose, and most of the points can be automated with rubocop -a, which HoundCI doesn't mention.

For example, I wired HoundCI up a custom Repo and the output was overwhelming:

Focusing on the original intent of this ticket:

I think it would make initial reviews a lot more self-service for new and old contributors, and allow folks to spend more time on the things humans do best.

I agree; I think we can get closer with a slightly different set of automation tools. I'm in the process of adding automated comments based on a needs-linting label, which mentions msftidy and rubocop for now https://github.com/rapid7/metasploit-framework/pull/13280

Example:

For now I don't think HoundCI provides the education/self-serving piece that we're after, and there doesn't seem to be configuration for creating a simple message per file with a custom message similar to the above, "This file fails linting, please run rubocop -a .... on it"

[gschneider-r7]

This might be of interest: https://github.com/marketplace/actions/rubocop-linter-action

Edit: This one might not work actually 🤔

NOTE: The Checks API only looks for pushes in the repository where the check suite or check run were created. Pushes to a branch in a forked repository are not detected and return an empty pull_requests array.

There are a bunch of rubocop actions out there actually, and at least one is probably useful here.

Edit: This one seems more interesting https://github.com/marketplace/actions/octocop

[adfoster-r7]

As far as I can tell, all actions are unfortunately a no go until Github makes changes to their permissions when running from forks - https://github.com/rapid7/metasploit-framework/pull/13145#issuecomment-604086076

A Github app, rather than action, is required as it has the permission available to it to comment on pull requests

[gschneider-r7]

That's a bummer! It seems like a workaround for some use cases would be to have an action run on a schedule rather than on pull request. The scheduled action would go through open PRs and do the thing, but that's probably limited (like that checks API thing probably wouldn't work). 😞

[github-actions[bot]]

Hi!

This issue has been left open with no activity for a while now.

We get a lot of issues, so we currently close issues after 60 days of inactivity. It’s been at least 30 days since the last update here.
If we missed this issue or if you want to keep it open, please reply here. You can also add the label "not stale" to keep this issue open!

As a friendly reminder: the best way to see this issue, or any other, fixed is to open a Pull Request.