Skip to content
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

FTP protocol implementation #8762

Merged
merged 4 commits into from
Aug 14, 2017
Merged

FTP protocol implementation #8762

merged 4 commits into from
Aug 14, 2017

Conversation

tabishimran
Copy link
Contributor

Adding an FTP protocol implementation with a few primitive functions - ls, cd, pwd, upload and download to the framework. Metasploit already had ftp mixins under '/lib/metasploit/framework/ftp/client.rb', but there was no standalone class. I have used code from existing mixins to create this implementation.

@busterb
Copy link
Member

busterb commented Jul 24, 2017

This would be good to go ahead and add support in the FTP mixin for using this.

Also run rubocop on the library and fix any errors you see.

@tabishimran
Copy link
Contributor Author

All right, I modified the code to reduce rubocop offenses from 99 to 52.
I followed metasploit's rubocop wiki to decide what to ignore.

Things changed

  • offenses related to indentation
  • offenses related to white spaces
  • offenses related to improper use of parentheses

Things ignored

  • Too many lines in the class
  • Suggestions to use unless, modifications to 'if' statements
  • Redundant self

That redundant self suggestion was ignored because I went through a lot of code written by many people and most of them seem to use self the way I did. It would be great if I could get some feedback about that!

As discussed, support for using this in the other two FTP mixins -

  • lib/metasploit/framework/ftp/client.rb,
  • lib/msf/core/exploit/ftp.rb

will be added in a separate PR after a little more testing.

@egypt
Copy link
Contributor

egypt commented Jul 28, 2017

This looks good; your rubocop decisions are spot on, I think.

I was thinking this would replace the copy-pasta versions in lib/msf/ and lib/metasploit/ -- basically instantiate this thing and call its methods instead of implementing them in both places. Do you think you could fit that into this pull request?

@tabishimran
Copy link
Contributor Author

All right, I'll get started with that! Changing the code itself shouldn't be a big deal, I'm kind of worried about breaking things though! I will change the mixins to use this FTP client implementation and then maybe try out a couple of modules/exploits which used those mixins to make sure everything is fine.

@busterb
Copy link
Member

busterb commented Aug 14, 2017

Since this is pure library code, I'm going to go ahead and land it, with a follow-up PR expected for actually using it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants