A way to make flask-caching optional / use a different caching mechanism

[michalc]

Description

flask-caching has a published vulnerability https://nvd.nist.gov/vuln/detail/CVE-2021-33026#vulnCurrentDescriptionTitle where access to the cache store would lead to authorised code execution in Superset. Unfortunately, a fix does not appear available at this time.

Use case / motivation

Would like to run Airflow without known vulnrabilities

Are you willing to submit a PR?

No...

Related Issues

pallets-eco/flask-caching#209
apache/superset#15271

[potiuk]

Hey @michalc - for the future please follow RESPONSIBLE disclosure of security vulnerabilities. I am not sure if you noticed, but when you create an issue and choose "Security vulnerability" you are directed to the process of raising security issues in responsible way. By creating an issue in the public repository you are putting users under additional danger.

[michalc]

@potiuk Apologies, will do in future

(Note: one reason I didn't is because of the content of the policy:

Note that this security address should be used only for undisclosed vulnerabilities.

So since this is a disclosed vulnerability, I thought the public issue was the appropriate way)

[potiuk]

Announcing non-fixed issue via public information (even you could figure out a software is vulnerable) is never a good idea. It's disclosed elsewehere, but not in Airflow. Anyhow the milk is spilled now.

While critical in severity, in order to exploit this vulnerability you need to get access to the underlying storage so I think it does not require an immediate action. Possibly we can wait for a fix.

@ashb @kaxil WDYT ?- I am not sure whether we can somehow replace flask-caching easily (I know in the past we used flask-cache but it's not been updated since 2014 and flask-caching is a fork of it)..

Currently cache mechanism is hard-coded in Airflow:

Cache(app=flask_app, config={'CACHE_TYPE': 'filesystem', 'CACHE_DIR': '/tmp'})

We can make it configurable, or disable it by default. Or simply wait until fixed WDYT?

[michalc]

Understood and sorry again: will certainly err more on the side of caution in future

[ashb]

I'm not even sure we actually use caching anywhere

[potiuk]

We initialize it at least

[mik-laj]

I would like to add that it is possible to configure the Flask-app quite freely(only 2 options are hardcoded), and therefore also choose most of the cache configuration options.

airflow/airflow/www/app.py

Lines 72 to 73 in 2d85a95

	flask_app.config['PERMANENT_SESSION_LIFETIME'] = timedelta(minutes=settings.get_session_lifetime_config())
	flask_app.config.from_pyfile(settings.WEBSERVER_CONFIG, silent=True)

https://github.com/sh4nks/flask-caching/blob/cb6a7a3d61057b07ebdc7a58359b41257fe0af93/flask_caching/__init__.py#L185-L195

I also don't think this threat is quite real, as we don't store cache on the remote server (e.g. Redis/Memcache). The attacker must have access to the local file system, which is already a much greater threat in most cases.

[potiuk]

Yeah. I think we should wait until they fix it. I agree with @mik-laj - by default the cache is configured to use "/tmp" folder locally, so the threat is minimal, and - as Kamil mentioned, if someone has access there, it's already a done-deal.

[uranusjr]

But echoing Ash’s comment, is this dependency even needed? I quickly checked flask-caching’s source code and it doesn’t seem to do anything magic during initialisation and needs explicit opt in to be effective. I quicked searched and didn’t find it used anywhere. Maybe we should consider removing it entirely?

[mik-laj]

@uranusjr plugins can still use this so we have to be careful when deleting this dependency.

[potiuk]

But echoing Ash’s comment, is this dependency even needed? I quickly checked flask-caching’s source code and it doesn’t seem to do anything magic during initialisation and needs explicit opt in to be effective. I quicked searched and didn’t find it used anywhere. Maybe we should consider removing it entirely?

@uranusjr plugins can still use this so we have to be careful when deleting this dependency.

I would be for it if we can 👍. I agree with @mik-laj that if plugins are using it, this migh be a problem - some plugins might add some complex rendering, and there caching might be an important optimization. Rather than entirely removing, maybe we could simply add another option "webserver -> enable_caching` which will be disabled by default. This is a low-risk change for 2.2 release maybe. If people will notice their 2.2 webserver introduced waiting time if they have custom plugins, we could explicitly add it the release notes to enable caching in this case.

[ThiefMaster]

IMHO the CVE is bogus. If someone has write access to your cache's storage, you are most likely already compromised.

[potiuk]

IMHO the CVE is bogus. If someone has write access to your cache's storage, you are most likely already compromised.

Very much so.

[potiuk]

Closing it as non-issue.