search

ronin-rb / ronin

Ronin is a Free and Open Source Ruby Toolkit for Security Research and Development. Ronin also allows for the rapid development and distribution of code, exploits, payloads, etc, via 3rd-party git repositories.
https://ronin-rb.dev
GNU General Public License v3.0
679 stars 49 forks source link

Address mixed IP list behavior, fix IPv6 handling, and add extended IPv6 formatting options #156

Closed cmhobbs closed 1 year ago

cmhobbs commented 1 year ago
postmodern commented 1 year ago

One question would be if you were processing a mixed list of IPv4 and IPv6 addresses, and you selected an option that only formats IPv4 addresses, what should be done with the IPv6 addresses? Pass them through unchanged? Or omit them from the output? The current behavior seems like a bug:

$ ./bin/ronin ip --hex-octet 2606:2800:220:1:248:1893:25c8:1946 127.0.0.1
a2e
7f.0.0.1
cmhobbs commented 1 year ago

The man page and --help output generally allude to a single address being used as input, everything is singular and IPv4 addresses are used nearly everywhere. That should be updated.

One question would be if you were processing a mixed list of IPv4 and IPv6 addresses, and you selected an option that only formats IPv4 addresses, what should be done with the IPv6 addresses? Pass them through unchanged? Or omit them from the output? The current behavior seems like a bug:

My personal preference would be to have Ronin::CLI::Commands::Ip#run detect the version of each IP automatically, perhaps as its first task? Ip doesn't have any idea that a list was passed in, I don't think. It looks to me like ValueProcessorCommand is handling the iteration.

The bug in --hex-octet with an IPv6 address is present because the call to split in Ip#format_ip is only splitting on :, thusly:

[1] pry(main)> six = "2606:2800:220:1:248:1893:25c8:1946"
=> "2606:2800:220:1:248:1893:25c8:1946"
[2] pry(main)> six.split(".")
=> ["2606:2800:220:1:248:1893:25c8:1946"]
[3] pry(main)> six.split(".").map { |octet| octet.to_i.to_s(16) }.join(".")
=> "a2e"
[4] pry(main)> six.split(":").map { |octet| octet.to_i.to_s(16) }.join(":")
=> "a2e:af0:dc:1:f8:765:19:79a"

--hex-octet, --octal-octet, and --ipv6-compat will all need to be updated to support IPv6 addresses. I also wonder if ip -r is behaving correctly with IPv6:

$ bin/ronin ip -r 2606:2800:220:1:248:1893:25c8:1946
6.4.9.1.8.c.5.2.3.9.8.1.8.4.2.0.1.0.0.0.0.2.2.0.0.0.8.2.6.0.6.2.ip6.arpa
postmodern commented 1 year ago

ronin ip's man page does say it accepts multiple IP addresses as arguments ([IP ...]) or via --file FILE. Maybe that could be clearer?

The IP version check should probably be in format_ip, since it's called from multiple places in process_value. Certain formatting options like --hex-octet should only be applied if the IP is a IPv4 address; technically IPv6 is already in hex format. IPAddr.new won't parse an IPv6 address with octal segments, so I'm going to guess that's an IPv4 only thing that browsers support.

cmhobbs commented 1 year ago

ronin ip's man page does say it accepts multiple IP addresses as arguments ([IP ...]) or via --file FILE. Maybe that could be clearer?

I do think it could use a couple of examples.

The IP version check should probably be in format_ip, since it's called from multiple places in process_value. Certain formatting options like --hex-octet should only be applied if the IP is a IPv4 address; technically IPv6 is already in hex format. IPAddr.new won't parse an IPv6 address with octal segments, so I'm going to guess that's an IPv4 only thing that browsers support.

Fair point. I considered handling it in #process_value just in case it got big enough that separate methods for IPv4 and IPv6 were required.

cmhobbs commented 1 year ago

what should be done with the IPv6 addresses? Pass them through unchanged? Or omit them from the output?

@postmodern what do you prefer here? initially i thought that passing them through unchanged made the most sense but perhaps an error is more appropriate? if the output is to be scripted, maybe no error or further messaging is necessary.

cmhobbs commented 1 year ago

Going to pause on my branch for the evening. I started with the hex conversion by extracting the logic from #format_ip but I'll need to do that a couple of times so it probably makes more sense to just make #format_ipv4 and #format_ipv6 methods instead and let #format_ip simply check for the version, calling the appropriate method accordingly.

cmhobbs commented 1 year ago

got a bunch of the pending specs to pass but i really want to refactor #format_ip at this point. i'll have to work on that tomorrow a bit.

TODO:

postmodern commented 1 year ago

what should be done with the IPv6 addresses? Pass them through unchanged? Or omit them from the output?

@postmodern what do you prefer here? initially i thought that passing them through unchanged made the most sense but perhaps an error is more appropriate? if the output is to be scripted, maybe no error or further messaging is necessary.

This seems like a reasonable choice in functionality. If a user requests octal octet encoded IP addresses which is incompatible with IPv6 (I'm assuming here, maybe there's some weird software out there that supports it), we should print an error. If anyone disagrees and wants the IPv6 addresses left in the output, they can submit an issue and I'll revisit it.

cmhobbs commented 1 year ago

Refactored #format_ip and extracted a couple methods from it. Just a couple of examples but I'm getting there with my branch:

$ bin/ronin ip -r 127.0.0.1 ::ffff:127.0.0.1
1.0.0.127.in-addr.arpa
1.0.0.0.0.0.f.7.f.f.f.f.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.ip6.arpa

$ bin/ronin ip --hex-octet 127.0.0.1 ::ffff:127.0.0.1
7f.0.0.1
::ffff:127.0.0.1

$ bin/ronin ip --hex 127.0.0.1 ::ffff:127.0.0.1
0x7f000001
::ffff:127.0.0.1

$ bin/ronin ip --octal 127.0.0.1 ::ffff:127.0.0.1
017700000001
07777757700000001
postmodern commented 1 year ago

I think --hex should behave the same for IPv6 as IPv4.

I also wonder if --hex-octet could convert ::ffff:127.0.0.1 into ::ffff:7f000001?

cmhobbs commented 1 year ago

I think --hex should behave the same for IPv6 as IPv4.

It should, I overlooked that while blasting through the tests. I'll add that to the list.

I also wonder if --hex-octet could convert ::ffff:127.0.0.1 into ::ffff:7f000001?

I believe it could but I wonder what it'll mean if we get an expanded address. I still need to look into the process for translating IPv6 addresses between compressed, expanded but shortened, and expanded. i'm also curious about IPv6 addresses that aren't just IPv4 compat. I'll have to write some more/different tests.

postmodern commented 1 year ago

I believe it could but I wonder what it'll mean if we get an expanded address. I still need to look into the process for translating IPv6 addresses between compressed, expanded but shortened, and expanded.

You can call IPAddr#to_string or Ronin::Support::Network::IP#canonical to get the expanded address, or call #to_s to get the compressed IPv6 address. Another more complex way would be to call #to_i to get an Integer version of the address, then bitmask/bitshift out each 16bit octet. 16bit octets that are 0 could be omitted for compressed addresses or left in for expanded IPv6 addresses.

cmhobbs commented 1 year ago

This seems like a reasonable choice in functionality. If a user requests octal octet encoded IP addresses which is incompatible with IPv6 (I'm assuming here, maybe there's some weird software out there that supports it), we should print an error. If anyone disagrees and wants the IPv6 addresses left in the output, they can submit an issue and I'll revisit it.

I'll adjust the output to raise errors.

cmhobbs commented 1 year ago

Updated my branch with IPv6 expansion. Ended up down a weird rabbit hole of manually converting it myself until I discovered IPAddr#to_string(). Will work on the errors and hex for IPv6 fixes maybe tomorrow.

postmodern commented 1 year ago

Also note that Ronin::Support::Network::IP#canonical is just an alias to IPAddr#to_string.

cmhobbs commented 1 year ago

I think --hex should behave the same for IPv6 as IPv4.

Just fixed this.

I also wonder if --hex-octet could convert ::ffff:127.0.0.1 into ::ffff:7f000001?

I'm not sure this makes sense to me because there technically aren't octets (are they hextets?). I'm also unsure how this would work for an IPv6 address that isn't IPv4 in an IPv6 compatible format.

cmhobbs commented 1 year ago

What is an appropriate error to raise when an address isn't supported for the operation? Are there some common error classes available in ronin already that might work? ArgumentError, perhaps?

postmodern commented 1 year ago

I'm not sure this makes sense to me because there technically aren't octets (are they hextets?). I'm also unsure how this would work for an IPv6 address that isn't IPv4 in an IPv6 compatible format.

IPv4 octets are called octets because they are eight bits or a byte. Maybe octets aren't the best term wrt IPv6, since IPv6 uses 16bit numbers. Technically, IPv6 uses the term segment.

postmodern commented 1 year ago

What is an appropriate error to raise when an address isn't supported for the operation? Are there some common error classes available in ronin already that might work? ArgumentError, perhaps?

If we're executing in CLI code, I would immediately call print_error and exit with 1, instead of raising an exception that later gets rescued.

cmhobbs commented 1 year ago

IPv4 octets are called octets because they are eight bits or a byte. Maybe octets aren't the best term wrt IPv6, since IPv6 uses 16bit numbers. Technically, IPv6 uses the term segment.

If that's the case, aren't the segments of the IPv6 address already in hex? Was your request simply to break down IPv6 compatible IPv4 addresses into the ::ffff: prefix followed by the IPv4 address in hex?

cmhobbs commented 1 year ago

What is an appropriate error to raise when an address isn't supported for the operation? Are there some common error classes available in ronin already that might work? ArgumentError, perhaps?

If we're executing in CLI code, I would immediately call print_error and exit with 1, instead of raising an exception that later gets rescued.

while waiting, i started running with ArgumentError. i'll adjust course, thanks.

postmodern commented 1 year ago

If that's the case, aren't the segments of the IPv6 address already in hex? Was your request simply to break down IPv6 compatible IPv4 addresses into the ::ffff: prefix followed by the IPv4 address in hex?

If you ran ronin ip --hex-octet ::ffff:127.0.0.1 I think it should either output ::ffff:7f.0.0.1 or ::ffff:7f000001. WDYT?

cmhobbs commented 1 year ago

If that's the case, aren't the segments of the IPv6 address already in hex? Was your request simply to break down IPv6 compatible IPv4 addresses into the ::ffff: prefix followed by the IPv4 address in hex?

If you ran ronin ip --hex-octet ::ffff:127.0.0.1 I think it should either output ::ffff:7f.0.0.1 or ::ffff:7f000001. WDYT?

I prefer ::ffff:7f000001 because that tracks with the IPv4 output.

cmhobbs commented 1 year ago

my branch is updated with the feedback from the fediverse. all that's left is to raise errors appropriately. currently just using ArgumentError

$ bin/ronin ip --hex-octet ::ffff:127.0.0.1 127.0.0.1
::ffff:7f.0.0.1
7f.0.0.1
cmhobbs commented 1 year ago

Alright, current checklist:

postmodern commented 1 year ago

Note that exit(1) really just raises SystemExit, so you test for it this way:

        expect {
          ...
        }.to raise_error(SystemExit) do |error|
          expect(error.status).to eq(1)
        end

and to test that print_error is called, you can use stubs:

        expect(subject).to receive(:print_error).with(
          "must specify one or more arguments, or the --file option"
        )
cmhobbs commented 1 year ago

Note that exit(1) really just raises SystemExit, so you test for it this way:

        expect {
          ...
        }.to raise_error(SystemExit) do |error|
          expect(error.status).to eq(1)
        end

and to test that print_error is called, you can use stubs:

        expect(subject).to receive(:print_error).with(
          "must specify one or more arguments, or the --file option"
        )

I was trying to do this before I saw your comment. I was using value_processor_command_spec.rb as a reference. RSpec seems to completely ignore my code that are calling print_error. Not sure what I'm doing wrong but I'll probably get back to it tomorrow. might be catching errors or something due to the way I've organized my tests? Note not a single instance of the word complain below (there should be 8 tests containing that word):

$ rspec spec/cli/commands/ip_spec.rb 
Run options: exclude {:integration=>true}

Ronin::CLI::Commands::Ip
  man_page
    must define a man page
    must map to a markdown man page
  #process_value
    when -r option is given
      will convert and IPv4 address to reverse name format
      will convert and IPv6 address to reverse name format
    when --hex option is given
      will convert an IPv4 address to hexadecimal
      will convert an IPv6 address
    when --decimal option is given
      will convert an IPv4 address to decimal
      will convert an IPv6 address
    when --octal option is given
      will convert an IPv4 address to octal
      will convert an IPv6 address to octal
    when --binary option is given
      will convert an IPv4 address to binary
      will convert an IPv6 address to binary
    when --octal-octet option is given
      will convert an IPv4 address to octal by octet
ip: called with --octal-octet for ::ffff:127.0.0.1

Finished in 0.01972 seconds (files took 0.40427 seconds to load)
14 examples, 0 failures

Coverage report generated for RSpec to /home/cmhobbs/src/ronin/coverage. 105 / 151 LOC (69.54%) covered.
Stopped processing SimpleCov as a previous error not related to SimpleCov has been detected

The code is working as intended, though:

$ bin/ronin ip --ipv6-compat ::ffff:127.0.0.1
ronin ip: called with --ipv6-compat for ::ffff:127.0.0.1
$ echo $?
1

I'll likely dive back into this tomorrow. At least all of the functionality is implemented (I think).

postmodern commented 1 year ago

I added Ronin::Support::Network::IP#ipv4 method and switched to ronin-support 1.1.0 branch. Run git pull origin 2.1.0, git rebase 2.1.0, and then bundle up ronin-support to get access to the new ronin-support 1.1.0 code.

cmhobbs commented 1 year ago 
        expect {
          ...
        }.to raise_error(SystemExit) do |error|
          expect(error.status).to eq(1)
        end

this works in isolation.

and to test that print_error is called, you can use stubs:

        expect(subject).to receive(:print_error).with(
          "must specify one or more arguments, or the --file option"
        )

adding this (with my error message) before or after the previous block causes various errors..

I've updated my branch with the former block.

postmodern commented 1 year ago

You should specify the print_error expect first, then the SystemExit expect, then invoke subject.format_*.

postmodern commented 1 year ago

Closed by PR #157.