WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 6 years ago

#4579 closed defect (bug) (fixed)

IPv6 IPs

Reported by: xiand0 Owned by: westi
Milestone: 2.5 Priority: low
Severity: minor Version:
Component: General Keywords: has-patch ipv6 tested dev-feedback
Focuses: Cc:

Description

WordPress shows IPv6 IPs for comments

  • with only the last part of the IP
  • without the : seperators.
  • with all non-number parts of the IP stripped.

For example,

2001:0618:0400:f1a9:e000:dead:babe:cafe

is by WordPress shown as:

200161840019000

...which is very different from the IP. It's not even possible to make out what subnet the IP belongs to because the letters are stripped.

A little note on WordPress and IPv6: It works. It's almost "IPv6 ready". You can login. You can write posts. Everything which is "critical" works.

The ONLY issue with using WP on a IPv6-ready host that I've noticed is that it IPs all wrong.

Attachments (5)

comment.php.diff (940 bytes) - added by ruckus 7 years ago.
Patch to comment.php to revert changeset:3990
4579.patch (564 bytes) - added by pishmishy 6 years ago.
revised regular expression
comment-ipv6-patch.patch (1.2 KB) - added by OverlordQ 6 years ago.
ipv6 patch
4579.ipv4-and-ipv6-only.diff (1.0 KB) - added by ruckus 6 years ago.
Overly strict filtering that does not allow other protocols than IPv4 and IPv6
4579.liberal.diff (1.2 KB) - added by ruckus 6 years ago.
Liberal filtering that just protects the database and is more future proof

Download all attachments as: .zip

Change History (34)

comment:1 foolswisdom7 years ago

  • Component changed from Security to General
  • Milestone set to 2.4 (future)

xiand0, guessing you are using WordPress 2.2.1?

comment:2 xiand07 years ago

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)

comment:3 matt7 years ago

Perhaps you could propose some new sanitation plugins?

comment:4 matt7 years ago

By plugins I meant functions.

comment:5 ruckus7 years ago

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.

ruckus7 years ago

Patch to comment.php to revert changeset:3990

comment:6 ruckus7 years ago

  • Cc ruckus added

comment:7 pishmishy6 years ago

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

comment:8 pishmishy6 years ago

  • Owner changed from anonymous to pishmishy
  • Status changed from new to assigned

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.

comment:9 pishmishy6 years ago

  • Keywords has-patch ipv6 added

comment:10 follow-up: darkdragon6 years ago

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

[:0-9a-zA-Z]

comment:11 in reply to: ↑ 10 ; follow-up: pishmishy6 years ago

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)

comment:12 in reply to: ↑ 11 westi6 years ago

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.

pishmishy6 years ago

revised regular expression

comment:13 pishmishy6 years ago

  • Keywords needs-testing added

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

comment:14 OverlordQ6 years ago

Missed a spot, see my patch.

OverlordQ6 years ago

ipv6 patch

comment:15 OverlordQ6 years ago

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

comment:16 ruckus6 years ago

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?

comment:17 follow-up: santosj6 years ago

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

ip2long() and long2ip()

comment:18 ruckus6 years ago

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.

comment:19 in reply to: ↑ 17 ruckus6 years ago

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 ...

comment:20 overlordq6 years ago

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

comment:21 follow-up: error6 years ago

  • Keywords tested dev-feedback added; needs-testing removed

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

comment:22 in reply to: ↑ 21 westi6 years ago

  • Owner changed from pishmishy to westi
  • Status changed from assigned to new

Replying to error:

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

Thank you for testing

comment:23 westi6 years ago

  • Resolution set to fixed
  • Status changed from new to closed

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

comment:24 follow-up: ruckus6 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

[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.

ruckus6 years ago

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

ruckus6 years ago

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

comment:25 in reply to: ↑ 24 westi6 years ago

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.

comment:26 westi6 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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

comment:27 ruckus6 years ago

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?

comment:28 ruckus6 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:29 ruckus6 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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

Note: See TracTickets for help on using tickets.