转到通道，TCP / IP端口映射未返回所有打开的端口

Question

I have been learning Golang to move all my penetration testing tools to it. Since I like to write my own tools this is a perfect way to learn a new language. In this particular case I think something is wrong with the way I am using channels. I know for a fact that is not finishing the port mapping because the other tools I use that I wrote on ruby are finding all the open ports but my golang tool is not. Can someone please help me understand what I'm doing wrong? Are channels the right way to go about doing this?

package main

import (
    "fmt"
    "log"
    "net"
    "strconv"
    "time"
)

func portScan(TargetToScan string, PortStart int, PortEnd int, openPorts []int) []int {
    activeThreads := 0
    doneChannel := make(chan bool)

    for port := PortStart; port <= PortEnd; port++ {
        go grabBanner(TargetToScan, port, doneChannel)
        activeThreads++
    }

    // Wait for all threads to finish
    for activeThreads > 0 {
        <-doneChannel
        activeThreads--
    }
    return openPorts
}

func grabBanner(ip string, port int, doneChannel chan bool) {
    connection, err := net.DialTimeout(
        "tcp",
        ip+":"+strconv.Itoa(port),
        time.Second*10)
    if err != nil {
        doneChannel <- true
        return
    }
    // append open port to slice
    openPorts = append(openPorts, port)

    fmt.Printf("+ Port %d: Open
", port)
    // See if server offers anything to read
    buffer := make([]byte, 4096)
    connection.SetReadDeadline(time.Now().Add(time.Second * 5))
    // Set timeout
    numBytesRead, err := connection.Read(buffer)
    if err != nil {
        doneChannel <- true
        return
    }
    log.Printf("+ Banner of port %d
%s
", port,
        buffer[0:numBytesRead])
    // here we add to map port and banner
    targetPorts[port] = string(buffer[0:numBytesRead])

    doneChannel <- true
    return
}

Note: seems to find the first bunch ports but not the ones that are above a hight number example 8080 but it usually does get 80 and 443... So I suspect something is timing out, or something odd is going on.

There are lots of bad hacks of code, mostly because I'm learning and searching a lot in how to do things, so feel free to give tips and even changes/pull requests. thanks

Answer 1

Your code has a few problems. In grabBanner you appear to be referencing openPorts but it is not defined anywhere. You're probably referencing a global variable and this append operation is not going to be thread safe. In addition to your thread safety issues you also are likely exhausting file descriptor limits. Perhaps you should limit the amount of concurrent work by doing something like this:

package main

import (
    "fmt"
    "net"
    "strconv"
    "sync"
    "time"
)

func main() {
    fmt.Println(portScan("127.0.0.1", 1, 65535))
}

// startBanner spins up a handful of async workers
func startBannerGrabbers(num int, target string, portsIn <-chan int) <-chan int {
    portsOut := make(chan int)

    var wg sync.WaitGroup

    wg.Add(num)

    for i := 0; i < num; i++ {
        go func() {
            for p := range portsIn {
                if grabBanner(target, p) {
                    portsOut <- p
                }
            }
            wg.Done()
        }()
    }

    go func() {
        wg.Wait()
        close(portsOut)
    }()

    return portsOut

}

func portScan(targetToScan string, portStart int, portEnd int) []int {
    ports := make(chan int)

    go func() {
        for port := portStart; port <= portEnd; port++ {
            ports <- port
        }
        close(ports)
    }()

    resultChan := startBannerGrabbers(16, targetToScan, ports)

    var openPorts []int
    for port := range resultChan {
        openPorts = append(openPorts, port)
    }

    return openPorts
}

var targetPorts = make(map[int]string)

func grabBanner(ip string, port int) bool {
    connection, err := net.DialTimeout(
        "tcp",
        ip+":"+strconv.Itoa(port),
        time.Second*20)

    if err != nil {
        return false
    }
    defer connection.Close() // you should close this!

    buffer := make([]byte, 4096)
    connection.SetReadDeadline(time.Now().Add(time.Second * 5))
    numBytesRead, err := connection.Read(buffer)

    if err != nil {
        return true
    }

    // here we add to map port and banner
    // ******* MAPS ARE NOT SAFE FOR CONCURRENT WRITERS ******
    // *******************  DO NOT DO THIS *******************
    targetPorts[port] = string(buffer[0:numBytesRead])

    return true
}

Your usage of var open bool and constantly setting it, then returning it is both unnecessary and non-idiomatic. In addition, checking if someBoolVar != false is a non-idiomatic and verbose way of writing if someBoolVar.

Additionally maps are not safe for concurrent access but your grabBanner function is writing to map from many go routines in a concurrent fashion. Please stop mutating global state inside of functions. Return values instead.

Here's an updated explanation of what's going on. First we make a channel that we will push port numbers onto for our workers to process. Then we start a go-routine that will write ports in the range onto that channel as fast as it can. Once we've written every port available onto that channel we close the channel so that our readers will be able to exit.

Then we call a method that starts a configurable number of bannerGrabber workers. We pass the ip address and the channel to read candidate port numbers off of. This function spawns num goroutines, each ranging over the portsIn channel that was passed, calls the grab banner function and then pushes the port onto the outbound channel if it was successful. Finally, we start one more go routine that waits on the sync.WaitGroup to finish so we can close the outgoing (result) channel once all of the workers are done.

Back in the portScan function We receive the outbound channel as the return value from the startBannerGrabbers function. We then range over the result channel that was returned to us, append all the open ports to the list and then return the result.

I also changed some stylistic things, such as down-casing your function argument names.

At risk of sounding like a broken record I am going to emphasize the following again. Stop mutating global state. Instead of setting targetPorts you should accumulate these values in a concurrency-safe manner and return them to the caller for use. It appears your usage of globals in this case is ill-thought out a mixture of convenience and not having thought about how to solve the problem without globals.