IP parsing security audit

[miguelraz]

npm was recently attacked by passing in octal numbers to their IVP4 spec, which could reroute local connections to exterior connections and vice versa.

This is known is security terms as "bad".

This issue is started as a way of keeping track of potential concerns in Julia and the package ecosystem.

cc @StefanKarpinski @cmcaine

[StefanKarpinski]

(Wrong Stefan K 😄 btw)

The todo here is to audit all the weird ways to format IP addresses and make sure that we error on the ones that we don't support. Arguably, erroring on weird potentially ambiguous formats is better for security than correctly parsing all possible formats, since the danger here is that Julia interprets a host name or IP address one way while some other layer (kernel, C library, application) interprets it a different way. Refusing weirdo formats makes it less likely that some other layer that we don't control will have a different interpretation.

[cmcaine]

Expanding on my message from slack:

parse(IPv4, str) will error if str contains leading zeroes in any of the fields.

julia/stdlib/Sockets/src/IPAddr.jl

Lines 175 to 202 in f9720dc

	function parse(::Type{IPv4}, str::AbstractString)
	fields = split(str,'.')
	i = 1
	ret = 0
	for f in fields
	if isempty(f)
	throw(ArgumentError("empty field in IPv4 address"))
	end
	if length(f) > 1 && f[1] == '0'
	throw(ArgumentError(ipv4_leading_zero_error))
	else
	r = parse(Int, f, base = 10)
	end
	if i != length(fields)
	if r < 0 || r > 255
	throw(ArgumentError("IPv4 field out of range (must be 0-255)"))
	end
	ret |= UInt32(r) << ((4-i)*8)
	else
	if r > ((UInt64(1)<<((5-length(fields))*8))-1)
	throw(ArgumentError("IPv4 field too large"))
	end
	ret |= r
	end
	i+=1
	end
	IPv4(ret)
	end

This isn't how browsers understand IP addresses, but it is safe.

The parser also happily accepts a single big number or class A or B addresses like 127.1, which we might want to ban. Finally, I think the empty string will be parsed as address 0, which would be a bug.

The js netmask code was vulnerable because they silently removed leading zeroes when they should have errored or interpreted the values as octal.

The third party package IPNets.jl, which is similar to netmask, uses the IPv4 type and is also safe from this issue. https://github.com/JuliaWeb/IPNets.jl/blob/master/src/IPNets.jl

I only looked at the ipv6 parsing briefly but it looks fine. We could probably drop support for embedded ipv4 addresses.

[StefanKarpinski]

@cmcaine, you willing to take the lead on this? Sounds like you're already 95% of the way there and a PR to fix a few corner cases would finish things up. I'm willing to bet that we can just stop supporting IP formats like 127.1 although since they are necessary for IPv6 maybe we should continue to support them?

[cmcaine]

Pushed a PR that bans class A IP addresses. The code already rejected empty strings, I just misread that.

I haven't yet made ipv6 addresses with embedded ipv4 addresses an error, just thought I'd get some opinions on this first.

[StefanKarpinski]

IIRC @Keno originally wrote this code (kudos for already having made most of the dangerous things an error); he may have some opinions.

[Keno]

See #9187 for prior discussion. We considered removing the non-quad form entirely but never fully went that way. I'd be in favor, but we have to decide if it's breaking. Also, the original npm issue seems mostly due to treating ip addresses as quads of strings, which they are decidedly not.

[cmcaine]

Yeah I don't think Julia code using the IPv4 type is vulnerable to that class of error because you would parse the value to an integer and then use it, whereas the JS code is validating strings.