Skip to content

Add SSRF mitigations using filter_var and CURLOPT_RESOLVE#8400

Open
Inverle wants to merge 60 commits intoFreshRSS:edgefrom
Inverle:ssrf-blocklist
Open

Add SSRF mitigations using filter_var and CURLOPT_RESOLVE#8400
Inverle wants to merge 60 commits intoFreshRSS:edgefrom
Inverle:ssrf-blocklist

Conversation

@Inverle
Copy link
Copy Markdown
Member

@Inverle Inverle commented Jan 4, 2026

The idea is to prevent FreshRSS from sending any HTTP requests to internal services, except for the ones that are explicitly allowed in the config.

Based on https://github.com/moodle/moodle/blob/6e82b46a480826d1a85394d9e5087f7d82d1dd52/lib/filelib.php#L3818 and https://github.com/symfony/symfony/blob/8.1/src/Symfony/Component/HttpClient/NoPrivateNetworkHttpClient.php

@Inverle Inverle added this to the 1.28.1 milestone Jan 4, 2026
Comment thread app/Utils/httpUtil.php Outdated
Comment thread app/Utils/httpUtil.php
@Alkarex
Copy link
Copy Markdown
Member

Alkarex commented Jan 7, 2026

Since this will be a breaking change, let's target 1.29.0

@Alkarex Alkarex modified the milestones: 1.28.1, 1.29.0 Jan 7, 2026
Comment thread app/Utils/httpUtil.php
Comment thread app/i18n/fr/admin.php Outdated
Inverle and others added 2 commits January 10, 2026 18:38
Co-authored-by: Alexandre Alapetite <alexandre@alapetite.fr>
@Inverle
Copy link
Copy Markdown
Member Author

Inverle commented Feb 28, 2026

Related: simplepie/simplepie#968

@Inverle
Copy link
Copy Markdown
Member Author

Inverle commented Mar 8, 2026

@Inverle
Copy link
Copy Markdown
Member Author

Inverle commented Mar 8, 2026

TODO: i need to reimplement the same redirection logic in SimplePie's File.php as in httpGet() right now..

never mind, my test script was broken it works fine already

@Inverle
Copy link
Copy Markdown
Member Author

Inverle commented Mar 8, 2026

and also POST needs to change to GET on redirect

@Inverle Inverle marked this pull request as ready for review March 8, 2026 19:20
@Inverle Inverle requested review from Alkarex and Frenzie March 8, 2026 19:35
Comment thread phpcs.xml
<exclude name="Squiz.Functions.MultiLineFunctionDeclaration.Indent"/>
<exclude name="Squiz.Functions.MultiLineFunctionDeclaration.OneParamPerLine"/>
<exclude name="Squiz.WhiteSpace.ScopeClosingBrace.ContentBefore"/>
<exclude name="Generic.CodeAnalysis.EmptyStatement.DetectedIf"/>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That does not seem needed, is it?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not anymore, but I would prefer to keep it

@Inverle
Copy link
Copy Markdown
Member Author

Inverle commented Apr 2, 2026

Yes, something like that indeed. But I am wondering whether the IP is enough or whether we should also allow some hostnames. In container setups in particular, the IPs are not known because they are automatic, so ideally we should allow something like rss-bridge.

Is this not covered already by adding rss-bridge:443 and rss-bridge:80 into the allowlist?
or do you mean that the port shouldn't be required?

@Alkarex
Copy link
Copy Markdown
Member

Alkarex commented Apr 2, 2026

It doesn't seem that $errorMessage is being printed anywhere. The lines were added in #7760

Well spotted. It was probably inspired by the following code, but lacking the FreshRSS_Feed_Exception or similar logging...

if ($simplePieResult === false || $simplePie->get_hash() === '' || !empty($simplePie->error())) {
if ($simplePie->status_code() === 429) {
$errorMessage = 'HTTP 429 Too Many Requests!';
} elseif ($simplePie->status_code() === 503) {
$errorMessage = 'HTTP 503 Service Unavailable!';
} else {
$errorMessage = $simplePie->error();
if (empty($errorMessage)) {
$errorMessage = '';
} elseif (is_array($errorMessage)) {
$errorMessage = json_encode($errorMessage, JSON_UNESCAPED_SLASHES | JSON_UNESCAPED_LINE_TERMINATORS) ?: '';
}
}
throw new FreshRSS_Feed_Exception(
($errorMessage == '' ? 'Unknown error for feed' : $errorMessage) .
' [' . $this->url(includeCredentials: false) . ']',
$simplePie->status_code()
);
}

@Alkarex
Copy link
Copy Markdown
Member

Alkarex commented Apr 2, 2026

Yes, something like that indeed. But I am wondering whether the IP is enough or whether we should also allow some hostnames. In container setups in particular, the IPs are not known because they are automatic, so ideally we should allow something like rss-bridge.

Is this not covered already by adding rss-bridge:443 and rss-bridge:80 into the allowlist? or do you mean that the port shouldn't be required?

Fine if providing a list of DNS/hostnames works. I mostly checked the IP parts, and I did not realise it was already there

@Inverle
Copy link
Copy Markdown
Member Author

Inverle commented Apr 2, 2026

I have added the environment variable support and documentation.

Comment thread app/views/configure/system.phtml
Comment thread app/i18n/fr/admin.php Outdated
Comment thread app/i18n/fr/admin.php Outdated
@Alkarex
Copy link
Copy Markdown
Member

Alkarex commented Apr 8, 2026

I have made a Docker image of this PR: freshrss/freshrss:8400
Help welcome to test.
@math-GH If you still have a Windows setup, a quick test of this PR would be appreciated

@Alkarex
Copy link
Copy Markdown
Member

Alkarex commented Apr 8, 2026

Testing in Docker (Debian image) on an Ubuntu server:

I observe a few feeds apparently failing to follow 302 redirects, which seems to be reproducible:

Retrieved unsupported status code "302" [https://www.technologyreview.com/stories.rss] 
Retrieved unsupported status code "302" [http://www.luc-damas.fr/hop/feed/]

I also see several DNS resolution bugs when refreshing by cron (seems better manually, but would need more tests):

Failed to resolve domain: https://feeds.feedburner.com/hackaday/LgoM [https://feeds.feedburner.com/hackaday/LgoM] 
Failed to resolve domain: https://www.lemonde.fr/les-decodeurs/rss_full.xml [https://www.lemonde.fr/les-decodeurs/rss_full.xml]
Failed to resolve domain: https://www.dr.dk/nyheder/service/feeds/udland [https://www.dr.dk/nyheder/service/feeds/udland] 
Failed to resolve domain: https://github.com/Alkarex.atom [https://github.com/Alkarex.atom]
Failed to resolve domain: https://blogs.windows.com/msedgedev/feed/ [https://blogs.windows.com/msedgedev/feed/] 
Failed to resolve domain: https://www.60millions-mag.com/rss.xml [https://www.60millions-mag.com/rss.xml]

All those domains are working fine when reverting to edge branch.

@Frenzie
Copy link
Copy Markdown
Member

Frenzie commented Apr 9, 2026

The CNAME points to another domain. Maybe that's related? Though it doesn't seem to be an immediate problem.

$ php -r '$h="feeds.feedburner.com"; $r=@dns_get_record($h, DNS_A+DNS_AAAA); var_dump($r); var_dump($r===false);'
array(2) {
  [0]=>
  array(5) {
    ["host"]=>
    string(17) "www4.l.google.com"
    ["class"]=>
    string(2) "IN"
    ["ttl"]=>
    int(300)
    ["type"]=>
    string(1) "A"
    ["ip"]=>
    string(14) "64.233.184.118"
  }
  [1]=>
  array(5) {
    ["host"]=>
    string(17) "www4.l.google.com"
    ["class"]=>
    string(2) "IN"
    ["ttl"]=>
    int(300)
    ["type"]=>
    string(4) "AAAA"
    ["ipv6"]=>
    string(22) "2a00:1450:400c:c0b::76"
  }
}
bool(false)
$ host -a feeds.feedburner.com
Trying "feeds.feedburner.com"
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 12873
;; flags: qr rd ra; QUERY: 1, ANSWER: 1, AUTHORITY: 0, ADDITIONAL: 0

;; QUESTION SECTION:
;feeds.feedburner.com.          IN      ANY

;; ANSWER SECTION:
feeds.feedburner.com.   256     IN      CNAME   www4.l.google.com.

Received 66 bytes from 127.0.0.1#53 in 0 ms

Except for blogs.windows.com, which does its own special thing:

$ host -a blogs.windows.com
Trying "blogs.windows.com"
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 33789
;; flags: qr rd ra; QUERY: 1, ANSWER: 1, AUTHORITY: 0, ADDITIONAL: 0

;; QUESTION SECTION:
;blogs.windows.com.             IN      ANY

;; ANSWER SECTION:
blogs.windows.com.      300     IN      HINFO   "RFC8482" ""

Received 56 bytes from 127.0.0.1#53 in 68 ms

@Alkarex
Copy link
Copy Markdown
Member

Alkarex commented Apr 19, 2026

@Inverle Are you able to reproduce the issues on your side?

@Inverle
Copy link
Copy Markdown
Member Author

Inverle commented Apr 20, 2026

@Alkarex I may have fixed the issues with a9c17ec, please retest (at least the 302 ones)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants