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.

Sign up for GitHub

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

Jump to bottom

add MySQL module #23

Merged
merged 51 commits into from
Dec 19, 2017
Merged

add MySQL module #23

merged 51 commits into from
Dec 19, 2017

Conversation

justinbastress
Copy link
Contributor

@justinbastress justinbastress commented Nov 27, 2017 •
edited
Loading

What it does:

  • Add mysql zgrab2 module
  • Generic TLS grab
    • Add tls.go, abstracting TLS configuration / scanning / output
    • Let ZGrab drive the TLS handshake
      1. The module's Scan() method uses the mysql driver
      2. If the server supports TLS, use the zgrab2 TLS APIs to do the scan based on the input flags
  • Add zschema schemas
  • Add travis integration
    • Pull down mysql docker containers to test against
    • Run zschema validation on output

How to test

When changes are published, Travis will automatically run the integration tests, but it's straightforward to run them manually:

  1. Pull down any requirements (see before_install and before_scripts in .travis.yml)
  1. Run start_mysql.sh
  2. Run integration_tests.sh
  3. Run cleanup_mysql.sh

Or, to directly run zgrab, set up a mysql container (see e.g. launch_mysql_container.sh), and run e.g.

  • echo 127.0.0.1 | zgrab2 mysql --heartbleed --keep-client-logs --heartbeat-enabled

Notes / Caveats

  • The debug-only tags are present in the mysql structs, but not in zcrypto or ssh
    • There is no custom JSON Marshaller to interpret these, either
  • Only the mysql module has integration tests
  • The scan statuses are currently very rudimentary (just success/fail)
  • The TLS command-line flags are almost certainly not in their final form (e.g. array values take a comma-separated list, which is processed via a naive split)
  • Future TODO: Include input configuration
    • It seems like this would be a good candidate for a zgrab-level result...i.e. drop the ScanFlags next to the result?

@zakird zakird requested a review from dadrian November 27, 2017 18:21
@justinbastress
Copy link
Contributor Author

justinbastress commented Nov 27, 2017 •
edited
Loading

<removed ridiculously-long, no-longer-relevant snippets>

@zakird
Copy link
Member

zakird commented Nov 27, 2017 via email

@justinbastress
Copy link
Contributor Author

@zakird You mean, could the SSL / MySQL transaction dump be pushed a layer up, or actually removing/paring down the content contained in result?

The former would require refactoring zgrab2; it takes the JSON object returned by the Scan() method and drops it into the "result" field.

Paring it down on the other hand would of course be simople -- e.g. it could be something as simple as { "mysql_server_version": "5.7.20-0ubuntu0.17.04.1", "tls_allowed": true }; I went for the shotgun approach and included everything I could, under the assumption that any unneeded data would just be ignored.

dadrian
dadrian reviewed Nov 28, 2017
View reviewed changes
Copy link
Member

@dadrian dadrian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some initial comments.

Could you give a quick description of the relationships between the interfaces in mysql.go and what they're used for?

lib/mysql/mysql.go Outdated
@@ -0,0 +1,651 @@
/**
* @TODO @FIXME: copyright info
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where did the MySQL code come from? Is it largely copy-pasted from one of the libraries, or is it written yourself based on the RFC?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was all written myself, though the consts were copied from the go mysql driver.

lib/mysql/mysql.go Outdated

// Capability flags
const (
CLIENT_LONG_PASSWORD uint32 = (1 << iota)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these in an RFC, or are they just a construct in this "library"?

Copy link
Contributor Author

@justinbastress justinbastress Nov 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These were copied from the go driver, and referenced in the client spec (I should have added a link to that, will do)

  • Add link to reference guide

lib/mysql/mysql.go Outdated
}

// Fill in a (possibly newly-created) Config instance with the default values
func NewConfig(base *Config) *Config {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't really a new config given that it modifies base.

Copy link
Contributor Author

@justinbastress justinbastress Nov 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. InitConfig is probably less misleading.

  • Rename

lib/mysql/mysql.go Outdated
Handshake *Handshake_Packet

// If this is true, the Connection is a TLS connection object and there should be a TLSHandshake log.
IsSecure bool
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could maybe be a function return TLSHandshake != nil? I could see either way

Copy link
Contributor Author

@justinbastress justinbastress Nov 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That’s probably safer. Less opportunity for forgetting to set/reset it.

  • Make IsSecure a method

lib/mysql/mysql.go Outdated
// The sequence number used with the server to number packets
SequenceNumber uint8
// The "Handshake" packet sent by the server, holding flags used in future calls
Handshake *Handshake_Packet
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There aren't normally underscores in types. Are you matching to an RFC or something here?

Copy link
Contributor Author

@justinbastress justinbastress Nov 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sort of — the OK_Packet/ERR_Packet had underscores in the reference manual, but Handshake did not. Since it’s already straying from the spec, it sounds like OKPacket, ERRPacket (or ErrPacket, or ErrorPacket), HandshakePacket would be just as consistent with the spec, and more consistent with Go

  • Rename

lib/mysql/mysql.go Outdated
// @TODO @FIXME: This is protocol version 10; handle previous / future versions
type Handshake_Packet struct {
// protocol_version: int<1>
ProtocolVersion byte `json:"protocol_version"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is protocol version zero a thing? Otherwise I'd omitempty

lib/mysql/mysql.go Outdated

// Handshake_Packet defined at https://web.archive.org/web/20160316105725/https://dev.mysql.com/doc/internals/en/connection-phase-packets.html
// @TODO @FIXME: This is protocol version 10; handle previous / future versions
type Handshake_Packet struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there anything in Handshake_Packet that really shouldn't have omitempty?

Copy link
Contributor Author

@justinbastress justinbastress Nov 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Certainly the non-length integers (character set, protocol version, etc) should be included even if they are 0, right?

For length integers and strings, my thinking is as above — but omitting empties there wouldn’t be a serious problem.

lib/mysql/mysql.go Outdated
// auth_plugin_name: string<NUL>, but old versions lacked null terminator, so returning string<EOF>
AuthPluginName string `json:"auth_plugin_name,omitempty"`
// }
// Synthetic field buily from capability_flags_1 || capability_flags_2 << 16
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/buily/built

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Fix this

lib/mysql/mysql.go Outdated
CapabilityFlags uint32 `json:"capability_flags"`
}

func (p *Handshake_Packet) GetDescription() string {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I follow why this exists. What's the goal with GetDescription()?

Copy link
Contributor Author

@justinbastress justinbastress Nov 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After looking at some other zgrab code, it probably shouldn’t. The JSON attributes in the struct definition achieve what I was going for here.

  • Remove

lib/mysql/mysql.go Outdated
c.Handshake = handshakePacket

// How to handle mismatched reserved? It will be available in the output, but should it trigger a 'failure'?
// if hex.EncodeToString(parsed.reserved) != "00000000000000000000" { ... }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would probably ignore reserved, and/or only output it if it's not 00000....

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Selectively output

dadrian
dadrian requested changes Dec 5, 2017
View reviewed changes
Copy link
Member

@dadrian dadrian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll need to convert packet_log to be a dict instead of a list, but aside from that looks pretty good. I'll try to get you copyright information.

@dadrian
Copy link
Member

dadrian commented Dec 5, 2017

@ajholland If you want to do a quick pass on this and confirm we're using the framework correctly, that'd be great.

@justinbastress
Copy link
Contributor Author

@dadrian The ConnectionLog is now a dict rather than a list; and now MySQLScanResults is just an alias for that (so you get results: { handshake: { ... }, tls_handshake: { ... }, ... } instead of e.g. results: { connection_log: { handshake :{ ... }, tls_handshake: { ... }, ... } })

ajholland
ajholland approved these changes Dec 5, 2017
View reviewed changes
Copy link
Contributor

@ajholland ajholland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@zakird zakird self-requested a review December 5, 2017 21:02
@zakird
Copy link
Member

zakird commented Dec 5, 2017

@justinbastress can you paste the current output structure?

@justinbastress
Copy link
Contributor Author

justinbastress commented Dec 5, 2017 •
edited
Loading

@zakird
Latest format, with a variety of scenarios (I tossed the individual responses into a JSON array to do the pretty-print in one shot):

@zakird
Copy link
Member

zakird commented Dec 5, 2017

Some notes:

                     "sql_state_marker":"",
                     "sql_state":"",

looks like both of those should be ignore if null

@zakird
Copy link
Member

zakird commented Dec 5, 2017

"time":"2017-12-05T15:48:58-05:00"

Is that timestamp needed, or do we already have that in an outer record?

@justinbastress
Copy link
Contributor Author

@zakird Last push added omitempty to a few more optional string fields (including the two you called out).
As for that time field, @dadrian noted that as well - that is added by the zgrab2 framework, presumably to get the time of that particular scan of that node/service. The result / error are what get returned by the module; the framework constructs the data object containing the results for each scan, containing the result/time/error for each service.

@justinbastress
Copy link
Contributor Author

justinbastress commented Dec 7, 2017 •
edited
Loading

Tasks from meeting with @zakird and @dadrian:

  • Add struct-field annotation to mark fields as debug-only (zgrab:"debug"?)
  • Omit default reserved in SSLRequest
  • Remove null terminator from strings (even those that are not defined to be NUL-terminated)
  • Make StatusFlags, CapabilityFlags lists of consts rather than flags (e.g. "capability_flags": [ "CLIENT_SSL", "CLIENT_PROTOCOL_41" ] instead of "capability_flags": 0x82)

The following would probably be better suited to #25:

  • Rename tls_handshake to tls
  • Move heartbleed flags into their own object (similar to existing zgrab) that is omitted if heartbleed is not checked
  • Include input configuration

And this would be a framework change (pretty small, but still beyond the scope of any single module):

  • Instead of { result, error, time }, use { result, error, status, timestamp }, where status is one of a finite set of enumerated strings (connection timeout, connection rejected, IO timeout after connecting, TLS handshake failure, application error, ...)

@justinbastress
Copy link
Contributor Author

justinbastress commented Dec 7, 2017 •
edited
Loading

Updated format, with items mentioned above:
removed

@zmap zmap deleted a comment from justinbastress Dec 15, 2017
justinbastress and others added 6 commits December 15, 2017 16:57
@justinbastress
pull/use zschema from pip
de41685
@justinbastress
move build to install, remove unused checkout
8f49912
@justinbastress
use set +e instead of bash +e
fcf62fb
@justinbastress
log.Warnf -> log.Fatalf for not implemented
d49b60f
@andrewsardone
Add baseline gitignore
4f85cf4 
A gitignore for the project didn't exist until now. While this
doesn't have to do directly with the work of the current PR, it seems
good to me to just have _something_ in place for future gitignores, and
no time like the present.

Throwing a .DS_Store ignore in there to just be extra cautious that
those pesky OS X/macOS files don't make their way into the repo.
@andrewsardone
Ignore zgrab output from git
0bd08ae 
None of this stuff should be checked in, since it's just the results of
zgrab or test runs.
@andrewsardone
Copy link
Contributor

Question (pull request commenting with a purpose)

@justinbastress, what's the purpose of this empty integration_tests.py?

@andrewsardone
Ignore build dependencies & artifacts
c0b0e7e
@andrewsardone
Move all test scripts into integration_tests/ directory
f45ab31 
This is just a very minor directory organization change, but it has the
advantage of keeping a bunch of files out of the root directory and
packaging them together since they are related to each other.

Now, our `integration_tests/` directory has a nice pattern of
setup/cleanup/test.sh scripts at the top global level and at each module
level:

```
❯ tree --dirsfirst integration_tests
integration_tests
├── mysql
│   ├── util
│   │   ├── launch_mysql_container.sh
│   │   └── wait_for_mysqld.sh
│   ├── cleanup.sh
│   ├── setup.sh
│   ├── single_run.sh
│   └── test.sh
├── ssh
│   ├── cleanup.sh
│   ├── setup.sh
│   └── test.sh
├── cleanup.sh
├── setup.sh
└── test.sh

3 directories, 12 files
```

The scripts are runnable via:

```
./integration_tests/setup.sh && ./integration_tests/test.sh && ./integration_tests/cleanup.sh
```
@andrewsardone
Add script to generate new-protocol tests [minor]
f38d85b 
Just a minor little utility script for generating shell scripts around
the setup/test/cleanup rhythm of the zgrab integration tests.
andrewsardone
andrewsardone reviewed Dec 18, 2017
View reviewed changes
Copy link
Contributor

@andrewsardone andrewsardone left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is all really cool, @justinbastress! The code reads well and is put together quite nicely. 😻 seeing TravisCI run through these containers and validating the output.

If you're interested, I committed some minor changes up on origin/aps/feature/addMySQLZGrabModule. They're just some stylistic project hygiene around the gitignore, consolidating test scripts into a single directory, and adding a new protocl test scripts generator.

If you want these changes, feel free to pull them in from my branch:

# on your local feature/addMySQLZGrabModule branch
git fetch origin && git merge origin/aps/feature/addMySQLZGrabModule

dadrian
dadrian approved these changes Dec 19, 2017
View reviewed changes
Copy link
Member

@dadrian dadrian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should think about a better way to keep the ZCrypto and ZGrab schemas in sync.

@zakird
Copy link
Member

zakird commented Dec 19, 2017

@dadrian I started to build dependency management into ZSchema at one point. But it's not ready for use yet.

@zakird
Copy link
Member

zakird commented Dec 19, 2017

@andrewsardone anything you'd like to see, or this seem good from your perspective?

@dadrian dadrian mentioned this pull request Dec 19, 2017
Closed
andrewsardone
andrewsardone approved these changes Dec 19, 2017
View reviewed changes
Copy link
Contributor

@andrewsardone andrewsardone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andrewsardone anything you'd like to see, or this seem good from your perspective?

This seems solid to me! Merge it in.

@zakird zakird merged commit 02f94e2 into master Dec 19, 2017
@justinbastress justinbastress mentioned this pull request Dec 19, 2017
Merged
@justinbastress justinbastress deleted the feature/addMySQLZGrabModule branch December 29, 2017 19:58
@justinbastress justinbastress mentioned this pull request Jan 31, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ajholland ajholland ajholland approved these changes

@dadrian dadrian dadrian approved these changes

@andrewsardone andrewsardone andrewsardone approved these changes

@zakird zakird Awaiting requested review from zakird

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@justinbastress @zakird @dadrian @andrewsardone @ajholland