rapid7 / rex-socket

The Rex Socket Abstraction Library
Other
12 stars 33 forks source link

Typo Leads to Dotted IPs being used for self.sslsock.hostname When They Aren't Meant to Be Used #47

Closed gwillcox-r7 closed 2 years ago

gwillcox-r7 commented 2 years ago

https://github.com/rapid7/rex-socket/blob/master/lib/rex/socket/ssl_tcp.rb#L131 has a bug in that it should be elsif not else. Otherwise what happens is that the case that is provided will be ignored and we will always trigger the else condition if the above self.peerhostname condition isn't true. In reality we only want to execute that code if !Rex::Socket.dotted_ip?(self.peerhost) evaluates to true, in which case we then execute self.sslsock.hostname = self.peerhost.

In its current case we will be setting self.sslsock.hostname = self.peerhost even in cases where self.peerhost is a dotted IP address, which defeats the purpose of the check that was added here.

We can confirm this is the case with a simple test bit of code.

3.0.2 :016 > if nil
3.0.2 :017 >   print "A"
3.0.2 :018 > else nil
3.0.2 :019 >   print "B"
3.0.2 :020 > end
B => nil 
3.0.2 :021 > if nil
3.0.2 :022 >   print "A"
3.0.2 :023 > elsif nil
3.0.2 :024 >   print "B"
3.0.2 :025 > end
 => nil

As you can see in the elsif case, both conditions are properly skipped over. However in the if...else case, the second condition is executed even though the condition passed to else was false, since else doesn't expect an argument and therefore will ignore the argument.

adfoster-r7 commented 2 years ago

@gwillcox-r7 Good catch - let me know if you're handling this, or if I should fire up a PR :+1:

gwillcox-r7 commented 2 years ago

@adfoster-r7 Just putting up a PR now, should be up soon :)

gwillcox-r7 commented 2 years ago

@adfoster-r7 PR now up at https://github.com/rapid7/rex-socket/pull/48 which should fix this, simple change.