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

added Simatic WinCC <= 7.0 SP3 Update2 information gathering module #1869

Closed
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@v-egoshin
Copy link

v-egoshin commented May 28, 2013

screen

@wchen-r7

This comment has been minimized.

Copy link
Contributor

wchen-r7 commented May 28, 2013

Is there a trial version downloadable? Been looking for it for testing.

end
end

def get_info dbs

This comment has been minimized.

@wchen-r7

wchen-r7 May 28, 2013

Contributor

Would be nice to do a report_auth_info() at the end of this function?

This comment has been minimized.

@v-egoshin

v-egoshin May 29, 2013

Nice idea, done!

@v-egoshin

This comment has been minimized.

Copy link

v-egoshin commented May 29, 2013

Is there a trial version downloadable?

I don't have any information, where you can download trial from the official site. But for testing, you can search on torrent-trackers. For example in google: site:rutracker.org wincc

@jvazquez-r7

This comment has been minimized.

Copy link
Contributor

jvazquez-r7 commented Jun 4, 2013

The module isn't msftidy compliant:

$ tools/msftidy.rb modules/auxiliary/admin/scada/simatic_wincc_harvester.rb 
simatic_wincc_harvester.rb - [ERROR] '<' is a bad character in module title.
simatic_wincc_harvester.rb - [ERROR] '=' is a bad character in module title.
simatic_wincc_harvester.rb - [WARNING] Improper capitalization in module title: 'information'
simatic_wincc_harvester.rb - [WARNING] Improper capitalization in module title: 'gathering'
simatic_wincc_harvester.rb:49 - [WARNING] Bad indent: "    \tkey = \"This is my encryptionkey\"\n"
simatic_wincc_harvester.rb:50 - [WARNING] Bad indent: "    \t# convert string to ascii array\n"
simatic_wincc_harvester.rb:51 - [WARNING] Spaces at EOL
simatic_wincc_harvester.rb:51 - [WARNING] Bad indent: "    \tascii = -> str { str .scan(/./)  .map{|c|c.ord} } \n"
simatic_wincc_harvester.rb:52 - [WARNING] Bad indent: "    \t# convert hex string to array\n"
simatic_wincc_harvester.rb:53 - [WARNING] Bad indent: "    \thex = -> num { num .scan(/../) .map{|n|n.to_i 16 if n.to_i>0} }\n"
simatic_wincc_harvester.rb:54 - [WARNING] Spaces at EOL
simatic_wincc_harvester.rb:54 - [WARNING] Bad indent: "    \tkey, hash = ascii.(key), hex.(hash) \n"
simatic_wincc_harvester.rb:56 - [WARNING] Bad indent: "    \t# complements an array of zeroes element\n"
simatic_wincc_harvester.rb:57 - [WARNING] Bad indent: "    \tusername = ascii.(username.upcase) + [0] * (key.size - ascii.(username).size)\n"
simatic_wincc_harvester.rb:58 - [WARNING] Bad indent: "    \t# delete spaces from ascii key array\n"
simatic_wincc_harvester.rb:59 - [WARNING] Bad indent: "    \thash.delete(32)\n"
simatic_wincc_harvester.rb:60 - [WARNING] Bad indent: "    \t# xor each symbol key and hash\n"
simatic_wincc_harvester.rb:61 - [WARNING] Bad indent: "    \txor_key_user  = key.zip(hash) .reject{|i| i[1].nil? } .map{|x| x[0]^x[1]}\n"
simatic_wincc_harvester.rb:62 - [WARNING] Bad indent: "    \t# xor previous step with username\n"
simatic_wincc_harvester.rb:63 - [WARNING] Bad indent: "    \txor_password = xor_key_user.zip(username) .map{|x| x[0]^x[1]}\n"
simatic_wincc_harvester.rb:64 - [WARNING] Bad indent: "    \t# get password characters\n"
simatic_wincc_harvester.rb:65 - [WARNING] Bad indent: "    \txor_password.select! {|sym| sym > 18} .map! { |sym| sym.chr}\n"
simatic_wincc_harvester.rb:67 - [WARNING] Bad indent: "    \txor_password.join\n"
simatic_wincc_harvester.rb:68 - [WARNING] Bad indent: "  \tend\n"
simatic_wincc_harvester.rb:70 - [WARNING] Bad indent: "  \tdef run\n"
simatic_wincc_harvester.rb:71 - [WARNING] Bad indent: "  \t\t# try connect to DB\n"
simatic_wincc_harvester.rb:72 - [WARNING] Bad indent: "    \tif mssql_login_datastore\n"
simatic_wincc_harvester.rb:73 - [WARNING] Bad indent: "    \t\t# get db\n"
simatic_wincc_harvester.rb:93 - [WARNING] Bad indent: "    \t# print output table\n"
simatic_wincc_harvester.rb:94 - [WARNING] Bad indent: "    \ttbl = Rex::Ui::Text::Table.new(\n"
simatic_wincc_harvester.rb:95 - [WARNING] Bad indent: "    \t\t'Indent' \t=> 4,\n"
simatic_wincc_harvester.rb:96 - [WARNING] Bad indent: "        \t'Header' \t=> header,\n"
simatic_wincc_harvester.rb:97 - [WARNING] Bad indent: "        \t'Columns' \t=> columns\n"
simatic_wincc_harvester.rb:98 - [WARNING] Bad indent: "    \t)\n"
simatic_wincc_harvester.rb:99 - [WARNING] Spaces at EOL
simatic_wincc_harvester.rb:99 - [WARNING] Bad indent: "    \n"
simatic_wincc_harvester.rb:100 - [WARNING] Bad indent: "    \tunless rows.nil?\n"
simatic_wincc_harvester.rb:102 - [WARNING] Bad indent: "        \t\ttbl << row\n"
simatic_wincc_harvester.rb:103 - [WARNING] Bad indent: "      \t\tend\n"
simatic_wincc_harvester.rb:104 - [WARNING] Bad indent: "      \t\tprint_line tbl.to_s\n"
simatic_wincc_harvester.rb:105 - [WARNING] Bad indent: "    \tend\n"
simatic_wincc_harvester.rb:115 - [WARNING] Spaces at EOL
simatic_wincc_harvester.rb:117 - [WARNING] Spaces at EOL
simatic_wincc_harvester.rb:118 - [WARNING] Spaces at EOL
simatic_wincc_harvester.rb:126 - [WARNING] Spaces at EOL
simatic_wincc_harvester.rb:127 - [WARNING] Spaces at EOL
simatic_wincc_harvester.rb:128 - [WARNING] Spaces at EOL
simatic_wincc_harvester.rb:129 - [WARNING] Spaces at EOL
simatic_wincc_harvester.rb:130 - [WARNING] Spaces at EOL
simatic_wincc_harvester.rb:158 - [WARNING] Spaces at EOL
simatic_wincc_harvester.rb:170 - [WARNING] Spaces at EOL
simatic_wincc_harvester.rb:182 - [WARNING] Bad indent: "    \tend\n"
simatic_wincc_harvester.rb:183 - [WARNING] Bad indent: "    end\n"

Warnings and errors should be fixed. Thanks!

'DisclosureDate' => 'Jun 03 2012'))
end

def decrypt username, hash

This comment has been minimized.

@jvazquez-r7

jvazquez-r7 Jun 4, 2013

Contributor

Please Use def with parentheses when there are arguments. Omit the parentheses when the method doesn't accept any arguments.

As recommended by: https://github.com/bbatsov/ruby-style-guide

This comment has been minimized.

@jvazquez-r7

jvazquez-r7 Jun 4, 2013

Contributor

On the other hand I should say I don't feel like this method is easily readable. In my opinion the use of lambdas and syntax with spaces makes this method hard to read.

This comment has been minimized.

@jvazquez-r7

jvazquez-r7 Jun 4, 2013

Contributor

Indeed also the ruby style guide recommends to not putting spaces between method names and parenthesis. Definitely I'm going to ask, please, to review the ruby style guide: https://github.com/bbatsov/ruby-style-guide And try to follow its recommendations for this module. I know the msf code isn't strictly compliant with this ruby styling guide. Neither is a requirement for this module. But this module code is hard to read, so I really would like to ask to review it and try to make as compatible as possible with the mentioned ruby style guide.

# get db name
db = db.first
prj[db] = {}
prj[db]["name"] = q("SELECT DSN FROM #{db}.dbo.CC_CsSysInfoLog")

This comment has been minimized.

@jvazquez-r7

jvazquez-r7 Jun 4, 2013

Contributor

Please, avoid %q unless you have a string with both ' and " in it. Regular string literals are more readable and should be preferred unless a lot of characters would have to be escaped in them.

As recommended by the ruby syle guide https://github.com/bbatsov/ruby-style-guide

This comment has been minimized.

@wchen-r7

wchen-r7 Jun 14, 2013

Contributor

Note: This is function q, not actually %q

# convert string to ascii array
ascii = -> str { str .scan(/./) .map{|c|c.ord} }
# convert hex string to array
hex = -> num { num .scan(/../) .map{|n|n.to_i 16 if n.to_i>0} }

This comment has been minimized.

@limhoff-r7

limhoff-r7 Jun 4, 2013

Contributor

This one line is far too compact. One-liners in general are hard to debug and step through, but this also has the problem of relying heavily on the precedence of method calling and trailing if. It's also calling n.to_i twice, so it's saved lines of code (which isn't a real concern), but made the code less efficient during runtime.

# xor previous step with username
xor_password = xor_key_user.zip(username) .map{|x| x[0]^x[1]}
# get password characters
xor_password.select! {|sym| sym > 18} .map! { |sym| sym.chr}

This comment has been minimized.

@limhoff-r7

limhoff-r7 Jun 4, 2013

Contributor

Why is 18 significant? It's just a magic number from casual reading. This should be pulled out into a class constant with a meaningful name so that maintainers can differentiate this 18 from other 18 with different semantic meaning in metasploit-framework.

# complements an array of zeroes element
username = ascii.(username.upcase) + [0] * (key.size - ascii.(username).size)
# delete spaces from ascii key array
hash.delete(32)

This comment has been minimized.

@limhoff-r7

limhoff-r7 Jun 4, 2013

Contributor

Change this to hash.delete(' '.ord) so the intent of deleting spaces is obvious from the code instead of just the comment.

v-egoshin added some commits Jun 10, 2013

Merge branch 'module-simatic-wincc-harvester' of https://github.com/n…
…xnrt/metasploit-framework into module-simatic-wincc-harvester

Conflicts:
	modules/auxiliary/admin/scada/simatic_wincc_harvester.rb
@wchen-r7

This comment has been minimized.

Copy link
Contributor

wchen-r7 commented Jun 14, 2013

nxnrt, could you please send a pcap to msfdev[at]metasploit.com to demonstrate the module works? Been having a hard time finding a legit source to obtain the software... Thanks.

end
end

def q(query, verbose = false, only_rows = true)

This comment has been minimized.

@limhoff-r7

limhoff-r7 Jun 14, 2013

Contributor

Methods need more descriptive names than 1 letter.

This comment has been minimized.

@v-egoshin
@v-egoshin

This comment has been minimized.

Copy link

v-egoshin commented Jun 25, 2013

wchen-r7, of course! I can collect traffic for you. But if you have some troubles with testing this module, i can dump MSSQL DB with WinCC project (i don't think, that it's will be huge) and send it instead of pcap.

@wchen-r7

This comment has been minimized.

Copy link
Contributor

wchen-r7 commented Jun 25, 2013

Hi nxnrt,

A pcap would be great, would love to see this module in Metasploit. If MySQL dump + WinCC project is huge, best to avoid the hassle :-)

Thanks!

@wchen-r7

This comment has been minimized.

Copy link
Contributor

wchen-r7 commented Jun 25, 2013

Oh, you can e-mail the pcap: msfdev[at]metasploit.com, thanks again!

@wvu-r7

This comment has been minimized.

Copy link
Contributor

wvu-r7 commented Jul 2, 2013

@nxnrt: Did you send that pcap?

jvazquez-r7 pushed a commit that referenced this pull request Jul 25, 2013

@jvazquez-r7

This comment has been minimized.

Copy link
Contributor

jvazquez-r7 commented Jul 25, 2013

Hi @nxnrt ,

It has been moved to unstable atm until pcap's can be shared (or we have legit access to software) for verification/testing.

The module can be found on unstable here: 1b2c539

Thanks! Please feel free to reopen it if you can share pcap for verification at any moment!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment