Add new mixin for Nuuo models

[pedrib]

This PR adds a new mixing that will be required for the next PR's that I will submit.
I will mention this in the new PRs and include more information there.

This adds functions that will be used by all four new modules.

[pedrib]

@bcoles any non specific questions you have about these modules please post here.
Setting up all the CMS versions can be quite a task, I have VMs with all of them, so if you want pcap's let me know. These exploits were pretty well tested against a variety of versions.

The older versions of CMS fail without specific VS 2005 redistributable DLLs. It's a pain to set those up. The newer ones install pretty cleanly.

[pedrib]

Fixed a bug in the library to allow more robust file download and upload.

[jmartin-tech]

Please address namespace on methods and method scoping where appropriate.

[pedrib]

@jmartin-r7 changes made!

[bcoles]

This conditional block:

        if data == nil
          next
        else
          @nucs_session = data
          break
        end

Might be cleaner using a guard clause:

        next if data.nil?
        @nucs_session = data
        break

It's slightly easier to read, especially given that this block is already deeply buried in spaghetti.

[pedrib]

Easier to read? I think exactly the opposite, your proposal is much more convoluted and less expressive. However if that's what you want I will make the change...

[bcoles]

It's a suggestion. You're welcome to ignore it.

As a common code pattern, it's easy to read for readers familiar with Ruby.

Once familiar with right-hand conditionals, and their typical use case (guard clauses), this pattern is easier to read than nested conditionals. In this instance, the conditional is already nested two levels deep.

Part of the reason I find the existing implementation subjectively unnecessarily hard to read is due to code duplication.

Consider:

  def nucs_login
    if datastore['SESSION'] != nil
      # since we're logged in, we don't need to guess the version any more
      @nucs_session = datastore['SESSION']
      return
    end

    @nucs_versions.shuffle.each do |version|
      @nucs_version = version

      if datastore['PASSWORD'].nil?
        data = login_nopass
      else
        data = login_password
      end

      if data.nil?
        next
      else
        @nucs_session = data
        return # or break if you'd prefer
      end
    end
  end

[pedrib]

@bcoles I will implement your suggestions even if I don't agree with them 100%, it's your show anyway. I just don't like to make a lot of changes as I'm paranoid with QA, and this means I will have to retest all the exploits against a variety of target versions to ensure nothing gets broken.

I'll make all the required changes, submit again and wait for your feedback. Once you are happy with the results (for all the modules), I will just test all changes at once.

[bcoles]

It's not my show. My suggestions are suggestions. I'm primarily concerned with complexity for maintainability purposes.

Looking at the login logic, it could be boiled down even further, as the login_nopass and login_password methods are very similar, both private, and only ever invoked once.

I could be missing something, but I don't see the need for two methods. Perhaps it would be a good idea to merge both private login methods into a single private login method and move the if datastore['PASSWORD'] conditional there?

[pedrib]

@bcoles while I believe in code re-use and generally follow the style guide, I believe most of your suggestions actually make the code harder to read.

A more experienced Ruby programmer might find this easy to read:

next if data.nil?
@nucs_session = data
break

However a less experienced Ruby programmer, or a programmer coming from C / Python / Java will think WTF for a few seconds before understanding. For this type of programmer, the code below is much more readable:

       if data == nil
          next
        else
          @nucs_session = data
          break
        end

And an experienced Ruby programmer, being experienced, will not take longer to understand it either.

This, as some of your golfed examples, look like to me, as an experienced programmer in other languages and a Ruby noob, as style over readability. Now I know you are just following the Ruby style guide, but looks like to me a clear case of the Ruby Style guide shooting itself in the foot and thinking "this is great".

The more verbose, clearer structured methods will always make an easier read. Therefore, I am not incorporating your suggestions above besides the "more_data", which is effectively a bug as you pointed out.

If this is a blocker for integration let me know and I'll change as suggested, otherwise I prefer to leave it as is.

[pedrib]

@bcoles what's the smart way to test all these branches at once?
So I have all these branches in my repo, and each one has an individual exploit, plus another one for the library. Is there a way to merge them together?

I'm asking this because I'm basically copying and pasting the individual files from each of these PR into my own local branch that has all 4 exploits plus the lib, then editing there, testing and finally pasting back here via the github interface. Yes, it's stupid... and this is cumbersome and prone to error. I'd like to leave my exploits well tested... how's your git magic?

[bcoles]

I'm basically copying and pasting the individual files from each of these PR into my own local branch that has all 4 exploits plus the lib

^ +1

[pedrib]

Damn... I thought there was a better way.

Please let me know when you are done reviewing all modules and I'll give them a good final shake together, against a variety of CMS versions I have installed. In the meantime, I will test the module changes individually to make sure any obvious stuff gets caught.

[jmartin-tech]

One option is to create a temporary local branch off this one and merge each of PR to that branch locally. If all testing passes you are good, if not you will need to port any changes to the other PRs.

Another it to rebase each of the other branches off this one and test each branch individually. You could then push the branches to the other PRs and Travis tests would do a better job as well. When this PR lands as long and the last commit here is the base for the other branches there should not be any conflicts for the other PRs to land after.

[pedrib]

@jmartin-r7 understood, the first one sounds like a good option.

[pedrib]

Hey guys, do you need anything else from me to get this show on the road? Let me know!

[bcoles]

There's should probably be some error handling following the new_data = sock.recv(4096) calls. This would allow returning partial data in the event of an error, rather than failing and returning an empty ['', ''] array.

Based on your previous patch to initialize more_data = '' outside the loop, I presume returning partial data in the event of an error is the intended behavior. Something like:

break if !new_data || new_data.length == 0

Also, is there a reason nucs_send_msg and nucs_send_data_msg are implemented as separate methods when there's only one line different between them?

This design decision introduces comparitively significant complexity throughout the majority of the library. Merging these methods, and updating the methods which depend on them, would effectively cut the size of this library in half.

I offer the code below as a replacement for the following 5 methods:

• nucs_login
• nucs_send_msg
• nucs_send_data_msg
• login_password
• login_nopass

I've attempted to maintain your style throughout, with the exception of nucs_login which I firmly believe should make use of the guard clause as a "return early, return often" code pattern.

Note that, if these changes are implemented, any instances of nucs_send_data_msg in modules which make use of this library will need to be updated to nucs_send_msg.

  def nucs_login
    if datastore['SESSION'] != nil
      # since we're logged in, we don't need to guess the version any more
      @nucs_session = datastore['SESSION']
      return
    end

    @nucs_versions.shuffle.each do |version|
      @nucs_version = version

      res = nucs_send_msg(
        [
          "USERLOGIN",
          "Version: #{@nucs_version}",
          "Username: #{datastore['USERNAME']}",
          "Password-Length: #{datastore['PASSWORD'].length}",
          "TimeZone-Length: 0"
        ],
        datastore['PASSWORD']
      )

      if res[0] =~ /User-Session-No: ([a-zA-Z0-9]+)/
        @nucs_session = $1
        break
      end
    end
  end

  ##
  # Sends a protocol message synchronously - sends and returns the result
  ##
  def nucs_send_msg(msg, data='')
    ctx = { 'Msf' => framework, 'MsfExploit' => self }
    sock = Rex::Socket.create_tcp({ 'PeerHost' => rhost, 'PeerPort' => rport, 'Context' => ctx })
    sock.write(format_msg(msg))
    sock.write(data.to_s) unless data.to_s == ''
    res = sock.recv(4096)
    more_data = ''
    if res =~ /Content-Length:([0-9]+)/
      data_sz = $1.to_i
      recv = 0
      while recv < data_sz
        new_data = sock.recv(4096)
        break if !new_data || new_data.length == 0
        more_data << new_data
        recv += new_data.length
      end
    end
    # socket cannot be closed, it causes exploits to fail...
    #sock.close
    return [res, more_data]
  rescue
    return ['', '']
  end

[pedrib]

Ok after screwing up my local and remote branches, here is the final solution. I incorporated your changes with very small additions.

Since all the changes are internal it seems they only really affect one module, the file upload one. I will push it to the other branch - this time manually to avoid this chaos.

Check bae83e9 for the right commit

[pedrib]

btw there's a lot of crappy commits here - let me know if you want me to submit a new PR for this

[pedrib]

Guys anything else you need?

[jmartin-tech]

Please squash the commits here. It looks like your testing of all branches merged together was pushed here and then you removed the files as additional commits. This will cause conflicts when trying to merge the other PRs as well.

It might be reasonable to rebase the other PRs once the commits here are cleaned up.

If you have enabled the Allow edits from maintainers. option I can help with the cleanup.

[pedrib]

@jmartin-r7 that is enabled, can you please lend a hand? Sorry, I'm pretty much a git noob.

[wchen-r7]

I see your evil plan there :-)

[pedrib]

:) I'm not planning to dig deeper (for the moment), but pointing it out to whoever wants to look into it...

[jmartin-tech]

@wchen-r7 since I reworked the commit history on this you up for doing the final approach and landing procedure?

[pedrib]

Guys, before you land it, let me know you are done with all the reviewing - I will give it a good final shake across all the versions I have installed to make sure none of the recent changes have broken any of the exploits!

[jrobles-r7]

@pedrib I'm working on moving some of the code to lib/rex. After I'm done with that I'll submit a PR to your branch so you can test it.

[wchen-r7]

@jrobles-r7 is currently working on this pull request and putting some of the code in Rex. I'm sure he'll reach out to @pedrib. Thanks for the heads up!

[jrobles-r7]

Hello @pedrib, I update the modules. Could you test the updated modules as well?

I'm still working on the rex/mixin stuff but I will PR that separately so we can get the PRs you submitted landed.

[jrobles-r7]

Release Notes

This adds a mixin for the Nuuo NUCM protocol used for device and management software.