rzo1 commented on code in PR #2127:
URL: https://github.com/apache/stormcrawler/pull/2127#discussion_r3944167855
##########
archetype/src/main/resources/archetype-resources/src/main/resources/default-regex-filters.txt:
##########
@@ -27,6 +39,14 @@
-^https?://(?:192\.168(?:\.(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?))){2})(?::\d+)?(?:/|$)
# 172.16.0.0/12
-^https?://(?:172\.(?:1[6789]|2[0-9]|3[01])(?:\.(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?))){2})(?::\d+)?(?:/|$)
+# 169.254.0.0/16 (link-local, e.g. cloud instance metadata services)
+-^https?://(?:169\.254(?:\.(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?))){2})(?::\d+)?(?:/|$)
+# 100.64.0.0/10 (carrier-grade NAT)
+-^https?://(?:100\.(?:6[0-4]|[7-9][0-9]|1[01][0-9]|12[0-7])(?:\.(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?))){2})(?::\d+)?(?:/|$)
Review Comment:
`100.64.0.0/10` is `100.64.0.0` to `100.127.255.255`, i.e. second octet 64
to 127.
The alternation is `6[0-4]|[7-9][0-9]|1[01][0-9]|12[0-7]`, which blocks 60
to 64 (60 to 63 are public space) and **misses 65 to 69 entirely**. The test
only checks `100.64.0.1` and `100.127.255.254`, so neither end of the mistake
is covered.
```suggestion
-^https?://(?:100\.(?:6[4-9]|[7-9][0-9]|1[01][0-9]|12[0-7])(?:\.(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?))){2})(?::\d+)?(?:/|$)
```
Please apply to all four copies of the file and add `100.65.0.1` and
`100.63.0.1` to `otherNonRoutableRangesAreRejected`.
##########
core/src/main/resources/crawler-default.yaml:
##########
@@ -161,8 +161,12 @@ config:
# - "linklocal" (matches InetAddress.isLinkLocalAddress())
# Only addresses matching an include rule are fetched (empty means all are
# allowed), addresses matching an exclude rule are always blocked.
- # http.filter.ipaddress.include:
- # http.filter.ipaddress.exclude: "localhost,sitelocal,linklocal"
+ # The exclude list is enabled by default: a fetched page decides which hosts
+ # the fetcher connects to, and loopback, private and link-local ranges host
+ # unauthenticated services (e.g. cloud instance metadata) which a public
+ # index must not leak into. Set http.filter.ipaddress.include to any value
Review Comment:
This advice does not work as written.
> Set http.filter.ipaddress.include to any value (e.g. "0.0.0.0/0") and
leave the exclude list empty
Exclude wins over include in `IPFilterRules.accept`, so setting only
`include` changes nothing: the shipped exclude list on line 169 still applies.
The operator has to write `http.filter.ipaddress.exclude: ""` explicitly.
Please say that.
Also, `fd00::/8` here only covers the locally-assigned half of ULA;
`fc00::/7` is the full range. Same point as in #2119.
##########
archetype/src/main/resources/archetype-resources/src/main/resources/default-regex-filters.txt:
##########
@@ -14,11 +14,23 @@
# of services running on the crawling machine (e.g., Elasticsearch,
# Storm)
#
-# - exclude localhost and loop-back addresses
+# NOTE: these rules judge the bytes of the URL only. A host name which
+# resolves into one of the ranges below is not caught here; the IP
+# address filter (http.filter.ipaddress.exclude) sees the address the
+# fetcher actually connects to and is the authoritative check.
+#
+# - exclude localhost and loop-back addresses, including the abbreviated
+# and integer IPv4 forms the resolver maps to 127.0.0.1
# http://localhost:8080
# http://127.0.0.1/ .. http://127.255.255.255/
+# http://127.1/
+# http://2130706433/
# http://[::1]/
--^https?://(?:localhost|127(?:\.(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?))){3}|\[::1\])(?::\d+)?(?:/|$)
+-^https?://(?:localhost|127(?:\.(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?))){0,3}|\[::1\])(?::\d+)?(?:/|$)
Review Comment:
Case-sensitive, so `LOCALHOST` passes. Worth `(?i)` here too, same as the
IPv6 rule on line 49.
##########
archetype/src/main/resources/archetype-resources/src/main/resources/default-regex-filters.txt:
##########
@@ -27,6 +39,14 @@
-^https?://(?:192\.168(?:\.(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?))){2})(?::\d+)?(?:/|$)
# 172.16.0.0/12
-^https?://(?:172\.(?:1[6789]|2[0-9]|3[01])(?:\.(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?))){2})(?::\d+)?(?:/|$)
+# 169.254.0.0/16 (link-local, e.g. cloud instance metadata services)
+-^https?://(?:169\.254(?:\.(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?))){2})(?::\d+)?(?:/|$)
+# 100.64.0.0/10 (carrier-grade NAT)
+-^https?://(?:100\.(?:6[0-4]|[7-9][0-9]|1[01][0-9]|12[0-7])(?:\.(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?))){2})(?::\d+)?(?:/|$)
+# 0.0.0.0/8 ("this network")
+-^https?://(?:0(?:\.(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?))){1,3})(?::\d+)?(?:/|$)
+# IPv6 unique-local (fc00::/7) and link-local (fe80::/10) addresses
+-^https?://\[(?:f[cd][0-9a-f]{2}|fe[89ab][0-9a-f]):[0-9a-f:]+\](?::\d+)?(?:/|$)
Review Comment:
Two gaps here.
**Case.** `RegexURLFilter` does not compile with `CASE_INSENSITIVE` (the
suffix rule at the top of the file carries its own `(?i)`), so `[FD00::1]` and
`[FE80::1]` pass. Same for `LOCALHOST` on line 29.
**Loopback spellings.** Both of these reach loopback through the JVM
resolver while matching neither rule:
```
InetAddress.getByName("::ffff:127.0.0.1") -> /127.0.0.1
InetAddress.getByName("0:0:0:0:0:0:0:1") -> /0:0:0:0:0:0:0:1 (== ::1)
```
```suggestion
-^(?i)https?://\[(?:f[cd][0-9a-f]{2}|fe[89ab][0-9a-f]):[0-9a-f:]+\](?::\d+)?(?:/|$)
# loopback reached through an IPv4-mapped or fully expanded IPv6 literal
-^(?i)https?://\[(?:::ffff:127\.|(?:0*:){7}0*1\])
```
`isLoopbackAddress()` catches all of these after DNS now that the IP filter
is on by default, so this is defence in depth, but catching them earlier is
what this file is for.
##########
core/src/test/resources/default-regex-filters.txt:
##########
@@ -3,11 +3,50 @@
# skip image and other suffixes we can't parse or are not likely to be relevant
# if you want to crawl images or videos or archives then you should comment
out this line
--(?i)\.(gif|jpg|png|ico|css|sit|eps|wmf|zip|gz|rpm|tgz|mov|exe|jpeg|bmp|js|mpg|mp3|mp4)(\?|&|$)
+-(?i)\.(apk|deb|cab|iso|gif|jpg|png|svg|ico|css|sit|eps|wmf|rar|tar|jar|zip|gz|bz2|rpm|tgz|mov|exe|jpeg|jpe|bmp|js|mpg|mp3|mp4|m4a|ogv|kml|wmv|swf|flv|mkv|m4v|webm|ra|wma|wav|avi|xspf|m3u)(\?|&|$)
# skip URLs with slash-delimited segment that repeats 3+ times, to break loops
-# very time-consuming : use only if necessary
+# very time-consuming : use BasicURLFilter instead
# -.*(/[^/]+)/[^/]+\1/[^/]+\1/
+# exclude localhost and equivalents to avoid that information
+# can be leaked by placing faked links pointing to web interfaces
+# of services running on the crawling machine (e.g., Elasticsearch,
+# Storm)
+#
+# NOTE: these rules judge the bytes of the URL only. A host name which
+# resolves into one of the ranges below is not caught here; the IP
+# address filter (http.filter.ipaddress.exclude) sees the address the
+# fetcher actually connects to and is the authoritative check.
+#
+# - exclude localhost and loop-back addresses, including the abbreviated
+# and integer IPv4 forms the resolver maps to 127.0.0.1
+# http://localhost:8080
+# http://127.0.0.1/ .. http://127.255.255.255/
+# http://127.1/
+# http://2130706433/
+# http://[::1]/
+-^https?://(?:localhost|127(?:\.(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?))){0,3}|\[::1\])(?::\d+)?(?:/|$)
+# the integer form of an IPv4 address resolves to the address it encodes
+# (e.g. http://2130706433/ is 127.0.0.1); such URLs are blocked outright,
+# a crawl that needs one can rewrite it to its dotted form
+-^https?://\d{1,10}(?::\d+)?(?:/|$)
+#
+# - exclude private IP address spaces
+# 10.0.0.0/8
+-^https?://(?:10(?:\.(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?))){3})(?::\d+)?(?:/|$)
+# 192.168.0.0/16
+-^https?://(?:192\.168(?:\.(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?))){2})(?::\d+)?(?:/|$)
+# 172.16.0.0/12
+-^https?://(?:172\.(?:1[6789]|2[0-9]|3[01])(?:\.(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?))){2})(?::\d+)?(?:/|$)
+# 169.254.0.0/16 (link-local, e.g. cloud instance metadata services)
+-^https?://(?:169\.254(?:\.(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?))){2})(?::\d+)?(?:/|$)
+# 100.64.0.0/10 (carrier-grade NAT)
+-^https?://(?:100\.(?:6[0-4]|[7-9][0-9]|1[01][0-9]|12[0-7])(?:\.(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?))){2})(?::\d+)?(?:/|$)
Review Comment:
Same CGNAT bug as in the archetype copy; see the comment there.
##########
core/src/test/resources/default-regex-filters.txt:
##########
@@ -3,11 +3,50 @@
# skip image and other suffixes we can't parse or are not likely to be relevant
# if you want to crawl images or videos or archives then you should comment
out this line
--(?i)\.(gif|jpg|png|ico|css|sit|eps|wmf|zip|gz|rpm|tgz|mov|exe|jpeg|bmp|js|mpg|mp3|mp4)(\?|&|$)
+-(?i)\.(apk|deb|cab|iso|gif|jpg|png|svg|ico|css|sit|eps|wmf|rar|tar|jar|zip|gz|bz2|rpm|tgz|mov|exe|jpeg|jpe|bmp|js|mpg|mp3|mp4|m4a|ogv|kml|wmv|swf|flv|mkv|m4v|webm|ra|wma|wav|avi|xspf|m3u)(\?|&|$)
# skip URLs with slash-delimited segment that repeats 3+ times, to break loops
-# very time-consuming : use only if necessary
+# very time-consuming : use BasicURLFilter instead
# -.*(/[^/]+)/[^/]+\1/[^/]+\1/
+# exclude localhost and equivalents to avoid that information
+# can be leaked by placing faked links pointing to web interfaces
+# of services running on the crawling machine (e.g., Elasticsearch,
+# Storm)
+#
+# NOTE: these rules judge the bytes of the URL only. A host name which
+# resolves into one of the ranges below is not caught here; the IP
+# address filter (http.filter.ipaddress.exclude) sees the address the
+# fetcher actually connects to and is the authoritative check.
+#
+# - exclude localhost and loop-back addresses, including the abbreviated
+# and integer IPv4 forms the resolver maps to 127.0.0.1
+# http://localhost:8080
+# http://127.0.0.1/ .. http://127.255.255.255/
+# http://127.1/
+# http://2130706433/
+# http://[::1]/
+-^https?://(?:localhost|127(?:\.(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?))){0,3}|\[::1\])(?::\d+)?(?:/|$)
+# the integer form of an IPv4 address resolves to the address it encodes
+# (e.g. http://2130706433/ is 127.0.0.1); such URLs are blocked outright,
+# a crawl that needs one can rewrite it to its dotted form
+-^https?://\d{1,10}(?::\d+)?(?:/|$)
+#
+# - exclude private IP address spaces
+# 10.0.0.0/8
+-^https?://(?:10(?:\.(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?))){3})(?::\d+)?(?:/|$)
+# 192.168.0.0/16
+-^https?://(?:192\.168(?:\.(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?))){2})(?::\d+)?(?:/|$)
+# 172.16.0.0/12
+-^https?://(?:172\.(?:1[6789]|2[0-9]|3[01])(?:\.(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?))){2})(?::\d+)?(?:/|$)
+# 169.254.0.0/16 (link-local, e.g. cloud instance metadata services)
+-^https?://(?:169\.254(?:\.(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?))){2})(?::\d+)?(?:/|$)
+# 100.64.0.0/10 (carrier-grade NAT)
+-^https?://(?:100\.(?:6[0-4]|[7-9][0-9]|1[01][0-9]|12[0-7])(?:\.(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?))){2})(?::\d+)?(?:/|$)
+# 0.0.0.0/8 ("this network")
+-^https?://(?:0(?:\.(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?))){1,3})(?::\d+)?(?:/|$)
+# IPv6 unique-local (fc00::/7) and link-local (fe80::/10) addresses
+-^https?://\[(?:f[cd][0-9a-f]{2}|fe[89ab][0-9a-f]):[0-9a-f:]+\](?::\d+)?(?:/|$)
Review Comment:
Same IPv6 gaps as in the archetype copy; see the comment there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
