IPv6 IPs

xiand0, guessing you are using WordPress 2.2.1?

WordPress 2.2 (yeah, I know, I should get around to upgrading) and WPMU 1.2.3. IPv6 IPs look the same in both WP branches, for example 2001618400192047617489 not 2001:0618:0400:f1a9:0204:76ff:fe17:489f (2001618400192047617489 could just as easily be 2001:f618:beef:babe:400f:1920:4761:7489 or.. a whole range of IPs, really)

Perhaps you could propose some new sanitation plugins?

By plugins I meant functions.

The culprit appears to be in changeset [3990]. I think the changes made to wp-includes/comment.php should just be reversed.

The data in $_SERVER['REMOTE_ADDR'] is filled in by the web server using information from the socket structure, so it seems to me there is little need to further "sanitize" it.

Patch to comment.php to revert changeset:3990

Duplicates #3987 and #3262. Closing those tickets however as they have little work on them.

We should just add a colon to the regular expression. I realize that it's improbable that we'd get bad data from the web server but it's better practice to mistrust this external data regardless.

I think what ruckus is saying is that the regex should be

[:0-9a-zA-Z]

Of course, sorry. Although you missed out the '.' :-) We also don't need characters after f.

[^0-9a-fA-F.:, ]

(Question: why does the original regex include a comma, space and ^? I don't think those are necessary)

Replying to pishmishy:

Of course, sorry. Although you missed out the '.' :-) We also don't need characters after f. [^0-9a-fA-F.:, ] (Question: why does the original regex include a comma, space and ^? I don't think those are necessary)

The ^ is an invert on the replace - i.e. replace anything that doesn't match the regex

Comma and Space are needed because $_SERVER['REMOTE_ADDR'] can contain multiple IPs I believe.

revised regular expression

Revised patch attached. Needs testing though as I haven't got an IPv6 WordPress install.

Missed a spot, see my patch.

ipv6 patch

And I tested it on my blog that is IPv6 enabled and it is working.

I don't think comma and space should be included, if we really want to have such strict checking. I don't see how there could be multiple IP addresses in $_SERVER['REMOTE_ADDR']. If someone knows how this can happen, it should be documented. A network connection only has 2 end-points, local and remote.

However, I'd like to vote once more for less strict filtering of the data. We should protect against SQL injection, but not more. Having overly strict filtering doesn't have any benefits that I can see, but can cause unnecessary problems if new address formats come up in the future.

At the very minimum we should not mangle the value, but rather record something like the static string "invalid" if we don't like the contents. I don't think storing a mangled value (like is currently happening with IPv6) has any useful value.

I'd produce a new patch, but I couldn't find out a couple of things:

• where is the comment data escaped for database injection currently, to protect against SQL injection?
• where is $postc defined?

It is too bad that these functions don't work with IPv6, because we could have just used them.

ip2long() and long2ip()

http://xref.redalt.com/wptrunk/_variables/index.htm#postc

According to this, $postc is referenced 12 times, and all the references are in the get_commentdata() function. Maybe it is deprecated already then.

The variable index doesn't list $postc as being defined anywhere.

Replying to santosj:

ip2long() and long2ip()

Theoretically, the remote address could be e.g. an AF_UNIX socket, which would be represented as a path name. Assuming it is only ever either IPv4 (AF_INET) or IPv6 (AF_INET6) would be wrong.

Not to mention ISO TP4 or SNA or DECnet or Apple Talk or IPX or ...

I really dont think REMOTE_ADDR can have more then one address in it.

Patch 4579.patch seems to fix the problem for me on 2.3.2.

Replying to error:

Patch 4579.patch seems to fix the problem for me on 2.3.2.

Thank you for testing

(In [6658]) Allow IPv6 addresses as well. Fixes #4579 props pishmishy.

[6658] only changes one of the two instances where filtering takes place.

I still think it would be sufficient to just escape the data before inserting in the database, no other filtering necessary.

If we do filtering, we should document why the different characters are allowed. I don't see how space and comma would ever be in REMOTE_ADDR.

Overly strict filtering that does not allow other protocols than IPv4 and IPv6

Liberal filtering that just protects the database and is more future proof

Replying to ruckus:

[6658] only changes one of the two instances where filtering takes place.

Why on earth that code is there twice I don't know. It makes no sense!

I still think it would be sufficient to just escape the data before inserting in the database, no other filtering necessary.

Not really as we want IPs rather than just safe data in that field.

If we do filtering, we should document why the different characters are allowed. I don't see how space and comma would ever be in REMOTE_ADDR.

As I have said above. AFAIK it is perfectly possible for REMOTE_ADDR to have a list of addresses in it - for example if the incoming request comes via a chain of proxy servers.

(In [6668]) Remove the duplicate code for sanitising IP Addresses we only need to do it once. Fixes #4579.

When a request is proxied, the IP addresses are collected in the HTTP header X-Forwarded-For. That is a comma separated list.

The REMOTE_ADDR comes from the web server and records the address (IP or other protocol) of the remote end of the connection to the server. It is always a single address. Its definition comes from the CGI environment spec: http://hoohoo.ncsa.uiuc.edu/cgi/env.html

Can you show an example of web server software or other circumstances when REMOTE_ADDR has a comma separated list of addresses?

Actually, IPv6 (and IPv4) is now stored correctly, so technically this is fixed.