1068851 - [mig modules] create a `ping` module

Status: RESOLVED FIXED

(Enterprise Information Security Graveyard :: MIG, task)

Description

[Julien Vehent]

A netcat module would be useful to verify that a given endpoint able to connect to a destination:port.

This feature could be used to detect endpoints that are, or are not, able to reach a given target.

Comment 1

[Michal Purzynski [:michal`] (use NEEDINFO)]

TCP
- try to ping the target, if nothing received return error for this part but try to continue (helps to tell when we have a routing problem and when it's the firewall)
- send SYN -> target, if received SYN+ACK, send ACK and than RST and report success.
- wait for a while, if nothing received, report 'dropped'
- if received RST or any ICMP code, report exactly what has been received and status FAIL

UDP
- try to ping the target, if nothing received return error for this part but try to continue (helps to tell when we have a routing problem and when it's the firewall)
- send UDP with random content to the target, if received nothing, report success with a note 'might be a fail'
- if ICMP received, FAIL and report

ICMP
- just a simple ping to check for routing issues

It would be useful to get in the report
- for ICMP -> type and code and L2 and L3 addresses
- for UDP and TCP -> flags and L2 and L3 addresses

I know that's a lot, just sayin would could be useful to have ;)

Comment 2

[Julien Vehent]

Hey, I said "netcat", not "nmap" !  :p

I looks like most of these features could be implemented the same way raw socket tests are implemented in the net package: https://golang.org/src/pkg/net/ipraw_test.go

Comment 3

[Sushant Dinesh]

Hey, I'd like to work on this bug. I've already forked the repo on Github and gone through the documentation there. I think I'm ready to write my first module :)

Comment 4

[Julien Vehent]

Hi Sushant,
This is great! I think the best way to go about it would be to start with a small module that tries to ping a remote IP. Then we can extend it to testing TCP and UDP ports.

The module documentation should help you get started http://mig.mozilla.org/doc/modules.rst.html
Along with this example module: https://github.com/mozilla/mig/blob/master/src/mig/modules/example/example.go

I'm happy to help you test and review your code as you go along. Feel free to post on this bug on ping me on irc (ulfr in #mig on irc.mozilla.org).

Cheers!

Comment 5

[Sushant Dinesh]

Hey,
So I've written a basic module for this. Do I post a diff here to show you the patch (for feedback) or is there a different workflow for projects on github?

Comment 6

[Julien Vehent]

Feel free to post the diff here, and keep the discussion in the bug.
When the code is ready, I can either merge your patch, or you can submit a pull request on github.

Comment 7

[Sushant Dinesh]

Attachment: nc

Comment 8

[Sushant Dinesh]

Attachment: nc

Comment 9

[Julien Vehent]

Comment on attachment 8543499 [details] [diff] [review]
nc

Review of attachment 8543499 [details] [diff] [review]:
-----------------------------------------------------------------

This is fantastic work! I really like the look of it.
There is a number of comments, and I hope they don't throw you off, because I'm very much impressed with this first version.

I have only read the code for now. I will test it live later today. I'm setting the review flag to '-' to indicate that additional work is needed, but overall the general approach is correct.

Thanks for this!

::: Makefile
@@ +106,3 @@
>  	$(GOGETTER) code.google.com/p/gcfg
> +#$(GOGETTER) github.com/gorilla/mux
> +#$(GOGETTER) github.com/jvehent/cljs

You should rebase your branch against the master branch and remove those lines from the patch.

::: conf/available_modules.go
@@ +11,4 @@
>  	//_ "mig/modules/filechecker"
>  	_ "mig/modules/netstat"
>  	//_ "mig/modules/upgrade"
> +	_ "mig/modules/netcat"

I would prefer if we don't commit this change for a little while, until the module has been tested thoroughly.

::: src/mig/modules/netcat/netcat.go
@@ +1,1 @@
> +package netcat

detail: the top of the file needs to include the license header, check out other files for an example.

@@ +44,5 @@
> +	}
> +	switch m.Type {
> +	case icmpv6EchoRequest, icmpv6EchoReply:
> +		return b, nil
> +	}

It's a style detail, but if that switch has only one case, I'd prefer to use an `if` statement instead.

@@ +59,5 @@
> +	// Place checksum back in header; using ^= avoids the
> +	// assumption the checksum bytes are zero.
> +	b[2] ^= byte(^s & 0xff)
> +	b[3] ^= byte(^s >> 8)
> +	return b, nil

a reference to the algorithm source (rfc?) would be useful here

@@ +72,5 @@
> +	m := &icmpMessage{Type: int(b[0]), Code: int(b[1]), Checksum: int(b[2])<<8 | int(b[3])}
> +	if msglen > 4 {
> +		var err error
> +		switch m.Type {
> +		case icmpv4EchoRequest, icmpv4EchoReply, icmpv6EchoRequest, icmpv6EchoReply:

It would be elegant to add a function to the icmpMessage type that returns true if the message is of type Echo.
func (m *icmpMessage) isEcho() (bool) {
    if ... {
        return true
    }
    return false
}

@@ +125,5 @@
> +}
> +
> +type Runner struct {
> +	Parameters params
> +	Results    results

You should use mig.ModuleResult directly:
    Results mig.ModuleResult
I should update the example module to reflect this.

@@ +137,5 @@
> +	// IPv4 or IPv6.
> +	Net string `json:"net"`
> +	// Number of times the host is to be pinged (-c option)
> +	Count int `json:"count"`
> +}

if `RemoteIP` can be a FQDN, then it would be better to rename it `Destination` and add a `DestinationPort` parameter.
The type of IP (v4 or v6) or FQDN could be determined automatically (ipv6 have semicolons).

What about something like this:

type params struct{
    // ipv4, ipv6 or fqdn
    Destination string `json:"destination"`
    // 16 bits integer
    DestinationPort float64 `json:"destinationport"`
    // icmp, tcp or udp
    Protocol string `json:"protocol"`
    // numbers of tests
    Count float64 `json:"count"`
}

Note that in json, all numbers are float64. There is no concept of int.

@@ +143,5 @@
> +type results struct {
> +	// Success = true => host was reachable.
> +	Success bool     `json:"success"`
> +	Errors  []string `json:"errors"`
> +}

Success should be used to indicate that the module ran successfully.
To indicate that the host was reachable, set FoundAnything to True instead.
https://github.com/mozilla/mig/blob/master/src/mig/module.go#L13-L36

@@ +148,5 @@
> +
> +type data struct {
> +	// true if the remote host was reachable.
> +	Reachable bool `json:"reachable"`
> +}

Is this type used? I can't find a reference to it.

@@ +152,5 @@
> +}
> +
> +func (r Runner) ValidateParameters() (err error) {
> +	// Check if RemoteIP is a valid IP or a FQDN.
> +	ip := net.ParseIP(r.Parameters.RemoteIP)

You need to check the resulting `ip` to verify that parsing worked.
if ip == nil {
    parsing failed
}

@@ +155,5 @@
> +	// Check if RemoteIP is a valid IP or a FQDN.
> +	ip := net.ParseIP(r.Parameters.RemoteIP)
> +	fqdn := regexp.MustCompilePOSIX(`^([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\-]{0,61}[a-zA-Z0-9])(\.([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\-]{0,61}[a-zA-Z0-9]))*$`)
> +	if !fqdn.MatchString(r.Parameters.RemoteIP) && ip == nil {
> +		return fmt.Errorf("ValidateParameters: Not a valid IP Address.")

Could you make the error message indicate that the fqdn does not match the expected format?

@@ +175,5 @@
> +		return r.buildResults()
> +	}
> +
> +	// ICMP Echo (Ping)
> +	if r.Parameters.Ping {

For the sake of extending it later, I think the ping code should be a separate function, and there should be a switch/case that calls the right test method based on the protocol type.

@@ +177,5 @@
> +
> +	// ICMP Echo (Ping)
> +	if r.Parameters.Ping {
> +		network := "ip4:icmp"
> +		typ := icmpv4EchoRequest

s/typ/icmpType/

@@ +181,5 @@
> +		typ := icmpv4EchoRequest
> +		if r.Parameters.Net == "IPv6" {
> +			network = "ip6:ipv6-icmp"
> +			typ = icmpv6EchoRequest
> +		}

As mentioned in the types discussions above, I think the IP version should be determined automatically based on the Destination.

@@ +198,5 @@
> +			Body: &icmpEcho{
> +				ID: xid, Seq: xseq,
> +				Data: bytes.Repeat([]byte("Ping!"), r.Parameters.Count),
> +			},
> +		}).Marshal()

Could you decompose the last 8 lines into something longer but easier to follow?
A few comment might help understand the logic, I'm particularly curious about that `os.Getpid()&0xffff` :)

@@ +211,5 @@
> +		}
> +
> +		rb := make([]byte, 20+len(wb))
> +		for {
> +			if _, err := c.Read(rb); err != nil {

Is the for loop necessary here? Why not read the entire response at once? It should never be larger than the MTU, which can be assumed to be 1500 bytes, so if the response is larger than that we can flag an error.
Yes? No?

@@ +245,5 @@
> +	if err != nil {
> +		panic(err)
> +	}
> +	return string(jsonOutput[:])
> +}

This needs to integrate FoundAnything and set it to true if at least one destination responded positively to the test.
https://github.com/mozilla/mig/blob/master/src/mig/modules/example/example.go#L178-L180

Comment 10

[Sushant Dinesh]

Attachment: nc

I hope I've addressed most of the issues and not left anything out.
I'm not sure about the "for" loop part being replaced by the reading 1500B of data. When I tried it, I ended up getting weird results. The problem is even I'm not sure why it's actually there (was taken from ip_raw tests link which you had posted in one of the above comments). I'll take a look at it again later if need be :)

Comment 11

[Julien Vehent]

I have this in my todo list, but under current load it will take me a couple days to get to it. Sorry for the delay...

Comment 12

[Julien Vehent]

Comment on attachment 8544539 [details] [diff] [review]
nc

Review of attachment 8544539 [details] [diff] [review]:
-----------------------------------------------------------------

Great progress! The code looks good. The module needs a little more work to be mergeable.

I think we should rename this "ping" instead of "netcat". It's more explicit, and will still cover the case of tcp and udp pings.

It seems that running it as non-root fails with error "Dial failed: dial ip4:icmp 10.0.0.1: operation not permitted". Can this be worked around? In the future, I'd like to be able to run modules as unprivileged users instead of root. It's not a blocker for the time being, just a nice-to-have.

When a target is found, it would be good to return some minimal statistics, like number of tests ran, average latency, etc.
    $ sudo ./bin/linux/amd64/mig-agent-20150110+51fb2e2.dev -m ping '{"destination": "10.0.0.1", "protocol": "icmp", "count": 10}'
    {"foundanything":true,"success":true,"elements":null,"statistics":null,"errors":null}

Parameters validation needs some work. At the moment, an invalid IP makes the module panic. A module should never panic, but return an intelligible error instead.
    $ ... -m ping '{"destination": "10.0.0.999", "protocol": "icmp", "count": 10}'
    panic: runtime error: index out of range

    goroutine 1 [running]:
    mig/modules/ping.Runner.ValidateParameters(0xc20802ab60, 0xa, 0x0, 0xc20802ab6c, 0x4, 0xa, 0x0, 0x0, 0x0, 0x0, ...)
	    /home/ulfr/git/opsec/build_mig/mig/src/mig/modules/ping/ping.go:189 +0x227

Comment 13

[Julien Vehent]

Another one: when a destination IP doesn't reply to a ping, `success` returns false but the module executed correctly and should be set to true instead.

{"foundanything":false,"success":false,"elements":null,"statistics":null,"errors":["Conn.Read failed: read ip4 10.0.0.200: i/o timeout"]}

Comment 14

[Sushant Dinesh]

Attachment: ping module

I still think that the patch is a bit rough, All the functionality is in there but I feel we can still display the data collected in a better way (or collect more data). The UDP scan is not fool proof and actually something like this should be implemented: http://nmap.org/book/man-port-scanning-techniques.htm.
But is such a thing required in this module or is something like that better to be left out for a separate nmap module? (I'm not sure)
I would love to hear your comments in this :)

Comment 15

[Guillaume Destuynder [:kang] [this account is no longer active]]

just a heads-up, i wont be able to review until at least mid/late next week (travels)

Comment 16

[Guillaume Destuynder [:kang] [this account is no longer active]]

@Sushant Dinesh:
Any reason for not using https://godoc.org/golang.org/x/net/icmp for example?

Comment 17

[Guillaume Destuynder [:kang] [this account is no longer active]]

Comment on attachment 8551752 [details] [diff] [review]
ping module

Review of attachment 8551752 [details] [diff] [review]:
-----------------------------------------------------------------

Couple of nitpicks, will look at the rest after needinfo is answered

::: src/mig/modules/ping/ping.go
@@ +3,5 @@
> +// file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +//
> +// Contributor: Sushant Dinesh [e-mail] [:sushant94]
> +
> +// netcat module is used to check the connection between an endpoint

i think you meant to replace that with some info om the ping module :)

@@ +301,5 @@
> +	}
> +	return
> +}
> +
> +// Open UDP ports are hard to determine. 

nit extra space

Comment 18

[Julien Vehent]

Comment on attachment 8551752 [details] [diff] [review]
ping module

Review of attachment 8551752 [details] [diff] [review]:
-----------------------------------------------------------------

Like :kang said, I think we should use the existing ICMP package for the ping part.
The TCP/UDP pings should be custom, as I doubt any package implements them. I still need to review that part.

::: src/mig/modules/ping/ping.go
@@ +236,5 @@
> +	// xid is the process ID.
> +	// 0xffff = 65535 = 2^16 - 1
> +	// We want the process ID to be within this range,
> +	// as identifier field in the icmp packet is of 16bit length.
> +	xid := os.Getpid() & 0xffff

why use the pid and not a random between 0 and 65535?

@@ +261,5 @@
> +	}
> +
> +	rb := make([]byte, 20+len(wb))
> +
> +	for {

Does reading the response need a loop? It seems to me that a valid reply to an icmp message should only take one read, and either return the icmp response or error with a timeout.

@@ +281,5 @@
> +
> +		if m.Type == icmpv4EchoRequest ||
> +			m.Type == icmpv6EchoRequest {
> +			continue
> +		}

I don't understand this if statement. Could you explain what it's supposed to do?

@@ +408,5 @@
> +	if len(b) < 20 {
> +		return b
> +	}
> +	hdrlen := int(b[0]&0x0f) << 2
> +	return b[hdrlen:]

if b[0] contains 0xff, hdrlen would have contain a value of 60. It shouldn't overflow b, because you initialize it with 42 bytes of data, plus icmp headers, plus 20 extra bytes. But for good measure, this function should perform a length check to make sure hdrlen never exceed len(b)-1.

Comment 19

[Sushant Dinesh]

To be honest I did not know that the net/icmp package existed. As I mentioned before this was my first go program and I just followed what :ulfr had mentioned in comment#3 ;) Sorry for not doing my research. Anyways, I can change it to use that package instead if need be.
And :ulfr I did ask you on IRC the other day if I needed to implement TCP and UDP using low level sockets but you said this was enough :P

Comment 20

[Guillaume Destuynder [:kang] [this account is no longer active]]

I think using the existing libs would make the code easier to maintain/audit/review (even thus its pretty cool to have done it without as well).

I let this up to ulfr and you. ulfr, can you re-set the review flag once its decided?

Comment 21

[Sushant Dinesh]

Attachment: ping module

Replaced functions with those from net/icmp rather than having our own functions for it.
This also meant I needed to add 3 more packages to dependencies. I've added them to the makefile, but I'm not sure if its in the right place. Would love your review on this patch.
Also, I could now remove the for loop which you mentioned in the above comment as we're using functions from ICMP package now and everything seems to be working fine.

Comment 22

[Julien Vehent]

Comment on attachment 8562976 [details] [diff] [review]
ping module

Review of attachment 8562976 [details] [diff] [review]:
-----------------------------------------------------------------

Fantastic progress! It's getting close to being ready.
Aside from the specific comments, I would suggest writing a documentation page, similar to file module: https://github.com/mozilla/mig/blob/master/src/mig/modules/file/doc.rst (it must be called doc.rst and be in the ping folder).

If you have time, I would also welcome some extra functions:
* PrintResults should pretty-print module results into strings, for display on the console.
  see netstat example: https://github.com/mozilla/mig/blob/master/src/mig/modules/netstat/netstat.go#L315

* ParamsParser should implement a command line flags parser to invoke the module from the command line
  see netstat example: https://github.com/mozilla/mig/blob/master/src/mig/modules/netstat/paramscreator.go#L161

Cheers!

::: Makefile
@@ +113,5 @@
>  	$(GOGETTER) github.com/ccding/go-stun/stun
> +go_get_ping:
> +	$(GOGETTER) golang.org/x/net/icmp
> +	$(GOGETTER) golang.org/x/net/ipv4
> +	$(GOGETTER) golang.org/x/net/ipv6

That looks good, but you need to add `go_get_ping` to the existing `go_get_agent_deps`. You should also rename it `go_get_ping_deps` for consistency with other names in the Makefile.

::: src/mig/modules/ping/ping.go
@@ +38,5 @@
> +type params struct {
> +	// ipv4, ipv6 or fqdn.
> +	Destination string `json:"destination"`
> +	// 16 bits integer.
> +	DestinationPort float64 `json:"destinationport"`

Maybe the destinationport should default to port 80? Right now, if no destinationport is defined, the ping hangs indefinitely.

I think it should also return an error if the protocol is set to `icmp` and a destinationport is defined.

@@ +44,5 @@
> +	Protocol string `json:"protocol"`
> +	// Number of tests
> +	Count int `json:"count,omitempty"`
> +	// Timeout for individual test. defaults to 5s.
> +	Timeout int `json:"timeout,omitempty"`

Count and Timeout should be float64. JSON has no notion of integers, all numbers are float64.

@@ +65,5 @@
> +	if err != nil {
> +		ip = r.Parameters.Destination
> +	} else {
> +		ip = ips[0]
> +	}

I don't think this works. When I try to use a FQDN as a destination, I get the following results:

$ sudo ./bin/linux/amd64/mig-agent-20150218+94b6e23.dev -m ping '{"destination": "google-public-dns-a.google.com.", "protocol": "icmp", "count": 2}'|j
[info] using builtin conf
{
    "elements": {
        "latency": null,
        "reachable": false
    },
    "errors": [
        "Fail on test#1 (Conn.Write Error: write ip4 8.8.8.8: i/o timeout)",
        "Fail on test#2 (Conn.Write Error: write ip4 8.8.8.8: i/o timeout)"
    ],
    "foundanything": false,
    "statistics": null,
    "success": true
}

Moreover, that `ips[0]` lookup should have a len check to make sure there's at list one element in the array before calling it.

@@ +222,5 @@
> +			err = r.Ping()
> +		case "tcp":
> +			err = r.tcpPing()
> +		case "udp":
> +			err = r.udpPing()

UDP ping returns wrong results, it seems:

$ sudo ./bin/linux/amd64/mig-agent-20150218+94b6e23.dev -m ping '{"destination": "8.8.8.8", "protocol": "udp", "count": 2, "destinationport": 53}'|j
[info] using builtin conf
{
    "elements": {
        "latency": null,
        "reachable": true
    },
    "errors": null,
    "foundanything": false,
    "statistics": null,
    "success": true
}

Comment 23

[Sushant Dinesh]

Attachment: ping module

Thansk for the review :)
Updated ping module with your suggestions.
Um, UDP ping actually does not return any reliable results. If it results in a Read Timeout and the write was successful it means that the port is probably open as the service may not respond to the write. That is why reachable is set to 'true' in the case that you have presented. I have mentioned this in the comments above the UDP ping method.
The ICMP ping in your case failed as the default setting to 5s for timeout was not working. I've updated to fix that too.
I will also start writing the documentation and the auxiliary methods.

Comment 24

[Julien Vehent]

Comment on attachment 8566587 [details] [diff] [review]
ping module

Review of attachment 8566587 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. I moved the patch to a branch named 'pingmodule' on the github repo. Pull request at https://github.com/mozilla/mig/pull/37

Comment 25

[Sushant Dinesh]

Cool.
I've also implemented the params parser and Pretty printer but I'm not sure on how to test them. Also wrote the documentation for the module. Do I post the same here for you to commit them?

Comment 26

[Sushant Dinesh]

Attachment: Documentation, paramsparser and prettyprinter

Comment 27

[Julien Vehent]

Comment on attachment 8572187 [details] [diff] [review]
Documentation, paramsparser and prettyprinter

Thanks Sushant. I reviewed the patch and made a number of small changes directly in the pull request at https://github.com/mozilla/mig/pull/37

It looks good overall, and I think it is ready to merge. My changes are mostly cosmetic, with a few modifications of the structure returned to be more consistent with other mig modules. Feel free to comment directly on the pull request.

Comment 28

[Julien Vehent]

Pull request has been merged. Thanks a lot to Sushant for his contribution!