Fix XSS weakness in airflow/www/views.py by ZuhairORZaki · Pull Request #36469 · apache/airflow

ZuhairORZaki · 2023-12-28T10:20:26Z

While triaging your project, our bug fixing tool generated the following message-

In file: views.py, there is a method that is vulnerable to XSS which can compromise any cookies, session tokens and other sensitive information used with the website and browser.

views.py

@expose(\"/confirm\", methods=[\"GET\"])
@auth.has_access_dag(\"PUT\", DagAccessEntity.TASK_INSTANCE)
@action_logging
def confirm(self):
    \"\"\"Show confirmation page for marking tasks as success or failed.\"\"\"
    args = request.args

    ...

    dag_id = args.get(\"dag_id\")

    ...

    dag = get_airflow_app().dag_bag.get_dag(dag_id)
    if not dag:
        msg = f\"DAG {dag_id} not found\"
        return redirect_or_json(origin, msg, status=\"error\", status_code=404)

Here request handler method confirm reads the value of request parameter dag_id. If no associated dag object is found then it cobstructs a string msg with it and passes it to redirect_or_json method which sends back a response containing that msg. Since flask automatically url decodes request parameters dag_id can be assigned javascript code which will be sent back as part of the response.

But the response content-type is application/json, so injected payload remains as a JSON data value in a JSON name/value pair and doesn't execute. So this isn't exactly an XSS vulnerability. Still if a client-side script receives this JSON value and uses some unsafe DOM update method e.g. setInnerHTML, document.write etc. then this could be a case of client-side XSS vulnerability.

Exploitable Scenario

Consider the following simple client side page.

<!DOCTYPE html>
<html lang=\"en\">
<head>
    <meta charset=\"UTF-8\">
    <meta name=\"viewport\" content=\"width=device-width, initial-scale=1.0\">
    <title>Fetch and Display Data</title>
</head>
<body>
    <div id=\"detail\"></div>

<script>
    document.addEventListener('DOMContentLoaded', function() {
        var pos=document.URL.indexOf(\"dag_id=\")+7;

        var dagId = decodeURIComponent(document.URL.substring(pos));

        fetch(`https://example.airflow.com/confirm?dag_id=${dagId}`)
            .then(response => response.text())
            .then(data => {
                document.getElementById(\"detail\").innerHTML = data;

            })
            .catch(error => console.error('Error:', error));
    });
</script>

</body>
</html>

Let's assume the page invoked with an URL such as

http://www.some.site/confirm_page.html?dag_id=123

The script wihtin the page reads the dag_id parameter value from the url and sends a GET request to the above server with that value. The server then sends a Not Found response message if no associated dag object is found with the dag_id value included.

Now an XSS attack can be carried out by sending an URL like below to a victim.

http://www.some.site/confirm_page.html?dag_id=%3Cimg%20src%3D%27x%27%20onerror%3D%27alert%281%29%27%3E

The payload here is a URL-encoded form of the following:

<img src='x' onerror='alert(1)'>

Since the client-side code here uses an unsafe DOM update method (element).innerHTML the injected payload will execute in the victim's browser.

Suggested Fix

One possible fix is sanitizing user provided data dag_id by defining a regex pattern that represents all inappropriate characters like the following:

pattern = re.compile(r'[^a-zA-Z0-9_\-]')

Then any character that matches the above pattern can be replaced with an empty string using following statement:

dag_id = re.sub(pattern, '', dag_id)

Now if an attacker supplies javascript code as value for dag_id e.g. <script>alert(1)</script>, it will just become scriptalert1script.

CLA Requirements:

This section is only relevant if your project requires contributors to sign a Contributor License Agreement (CLA) for external contributions.

All contributed commits are already automatically signed off.

The meaning of a signoff depends on the project, but it typically certifies that committer has the rights to submit this work under the same license and agrees to a Developer Certificate of Origin (see https://developercertificate.org/ for more information).

Git Commit SignOff documentation

Sponsorship and Support:

This work is done by the security researchers from OpenRefactory and is supported by the Open Source Security Foundation (OpenSSF): Project Alpha-Omega. Alpha-Omega is a project partnering with open source software project maintainers to systematically find new, as-yet-undiscovered vulnerabilities in open source code - and get them fixed - to improve global software supply chain security.

The bug is found by running the iCR tool by OpenRefactory, Inc. and then manually triaging the results.

boring-cyborg · 2023-12-28T10:20:30Z

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:

Pay attention to the quality of your code (ruff, mypy and type annotations). Our pre-commits will help you with that.
In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
Consider using Breeze environment for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
Be sure to read the Airflow Coding style.
Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack

potiuk · 2023-12-28T11:54:15Z

NIT: can you please add a unit test showing the failure scenario?

Taragolis · 2023-12-28T13:54:04Z

Have no idea how to reproduce it.

In breeze this one link

http://127.0.0.1:28080/confirm?task_id=runme_0&dag_id=%3Cimg%20src%3D%27x%27%20onerror%3D%27alert%281%29%27%3E&state=success

redirect to the home page and just show as a text which are expected, because Flask by default use autoescape in Jinja templates

ZuhairORZaki · 2023-12-28T15:53:06Z

Have no idea how to reproduce it.

In breeze this one link
http://127.0.0.1:28080/confirm?task_id=runme_0&dag_id=%3Cimg%20src%3D%27x%27%20onerror%3D%27alert%281%29%27%3E&state=success
redirect to the home page and just show as a text which are expected, because Flask by default use autoescape in Jinja templates

Thank you for your feedback.
Yes jinja2 by default escapes values passed to a template.
I was saying if there exists a client-side page that updates DOM elements with the value sent back in the JSON response using methods like document.write, element.innerHTML, eval etc. then any javascript code passed will execute.

So If always jinja is used to render templates and in no way situation described above can occur, then there is no attack surface.

ZuhairORZaki · 2023-12-28T16:07:48Z

NIT: can you please add a unit test showing the failure scenario?

Seems the test failed because of using re module instead of re2.

potiuk · 2023-12-28T17:55:40Z

NIT: can you please add a unit test showing the failure scenario?

Seems the test failed because of using re module instead of re2.

Yes. We require re2 globally in order to protect against other security issues (recursive denial of service for example). So you will need to switch to re2 .

But my comment was to add a new unit test case not about failing static check. A unit test that passes the bad value and have the code reject it. Generally we almost never accept PR that add a functionality when there is no corresponding unit test. Because - for example it makes it impossible to verify if it still works after cherry-picking it to another branch. If we would like to merge it to 2.8 branch we need to have unit test that should fail before and work after the change - this way when we cherry-pick the change we can see that the fix works also in the bugfix branch.

And yes - we do not have really a "security" issue as explained in the security report. And there is no known security attack surface, that's why we asked to just add validation as a regular PR.

Taragolis · 2023-12-28T19:16:45Z

In addition, the is no possible to create DAG ID with this <img src='x' onerror='alert(1)'> name because it would failed on validation

airflow/airflow/utils/helpers.py

Lines 50 to 60 in e9ba37b

    
           def validate_key(k: str, max_length: int = 250): 
        
               """Validate value used as a key.""" 
        
               if not isinstance(k, str): 
        
                   raise TypeError(f"The key has to be a string and is {type(k)}:{k}") 
        
               if len(k) > max_length: 
        
                   raise AirflowException(f"The key has to be less than {max_length} characters") 
        
               if not KEY_REGEX.match(k): 
        
                   raise AirflowException( 
        
                       f"The key {k!r} has to be made of alphanumeric characters, dashes, " 
        
                       f"dots and underscores exclusively" 
        
                   )

And original regex a bit a different rather than in this PR

airflow/airflow/utils/helpers.py

Line 42 in e9ba37b

KEY_REGEX = re.compile(r"^[\w.-]+$")

So it also have a chance that change proposed by this PR rather than fix something just breaks someone pipeline

ZuhairORZaki · 2023-12-28T21:20:24Z

In addition, the is no possible to create DAG ID with this <img src='x' onerror='alert(1)'> name because it would failed on validation

airflow/airflow/utils/helpers.py

Lines 50 to 60 in e9ba37b

def validate_key(k: str, max_length: int = 250):

"""Validate value used as a key."""

if not isinstance(k, str):

raise TypeError(f"The key has to be a string and is {type(k)}:{k}")

if len(k) > max_length:

raise AirflowException(f"The key has to be less than {max_length} characters")

if not KEY_REGEX.match(k):

raise AirflowException(

f"The key {k!r} has to be made of alphanumeric characters, dashes, "

f"dots and underscores exclusively"

)

And original regex a bit a different rather than in this PR

airflow/airflow/utils/helpers.py

Line 42 in e9ba37b

KEY_REGEX = re.compile(r"^[\w.-]+$")

So it also have a chance that change proposed by this PR rather than fix something just breaks someone pipeline

^[\w.-]+$ matches alphanumeric characters, dashes, dots and underscores. \w is short for a-zA-Z0-9_.
^a-zA-Z0-9_\- matches everything except alphanumeric characters, dash and underscore in order to filter them out.
So . is missing from the PR regex as I wasn't aware dots were also allowed. Thank you for pointing that out. It can be added but if validate_key is invoked in the pipeline prior to the request being processed then there is no need to further sanitize dag_id.
If that is indeed the case can you describe where in the pipeline validate_key is called on the request parameters so that we can better identify such cases in future and not report them needlessly.

potiuk · 2023-12-29T08:06:38Z

If that is indeed the case can you describe where in the pipeline validate_key is called on the request parameters so that we can better identify such cases in future and not report them needlessly.

Isn't that something that your tool should find automatically ? I think it shows the weakness of the tool. The worst thing that happens with such security reporting tools is to show false posititves. This is actually why we are very careful with those kind of tools and we blank-refuse reports showig potential problems with security - because it undermines the whole value of such tools.

It creates more work for people who are volunteers and when you have to analyse those kind of issues and find false positive after false positive, you very quickly go into tool fatique and drop using it.

I think the only value of such tool is that it can automatically not only detect potential value, but also automatically generate (and verify) the exploitation scenario that can be proven to work (basically this is what we expect when security issue is reported to us).

So maybe @ZuhairORZaki -> treat this one as exercise. If you can make the tool works by generating such exploitation scenario, this is probably something you can report. If you cannot generate such exploitation scenario, it's probably not reportable as security bug.

Ideally also if such a tool could generate not only the proposal how to fix things, a unit test that fixes it as well - that would become really useful. Other than that it mostly adds work with very limited value, and distracts from real issues, which makes the tool far too noisy to be useful

potiuk

Since it does not seem to address any problem, and no scenarios to fix it can be found, I change my decision to "request changes" - and likely it should not be merged.

ZuhairORZaki · 2024-01-01T08:59:28Z

So maybe @ZuhairORZaki -> treat this one as exercise. If you can make the tool works by generating such exploitation scenario, this is probably something you can report. If you cannot generate such exploitation scenario, it's probably not reportable as security bug.

Ideally also if such a tool could generate not only the proposal how to fix things, a unit test that fixes it as well - that would become really useful. Other than that it mostly adds work with very limited value, and distracts from real issues, which makes the tool far too noisy to be useful

Will keep those in mind
Thank you for being patient with me.

Fixed XSS Weakness in airflow/www/views.py

67a42d6

ZuhairORZaki requested review from ashb, bbovenzi, pierrejeambrun and ryanahamilton as code owners December 28, 2023 10:20

boring-cyborg bot added the area:webserver Webserver related Issues label Dec 28, 2023

ZuhairORZaki changed the title ~~Fix XSS weakness in method confirm~~ Fix XSS weakness in airflow/www/views.py Dec 28, 2023

potiuk approved these changes Dec 28, 2023

View reviewed changes

potiuk added this to the Airflow 2.8.1 milestone Dec 28, 2023

potiuk requested changes Dec 29, 2023

View reviewed changes

ZuhairORZaki closed this Jan 1, 2024

ephraimbuddy removed this from the Airflow 2.8.1 milestone Jan 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix XSS weakness in airflow/www/views.py#36469

Fix XSS weakness in airflow/www/views.py#36469
ZuhairORZaki wants to merge 1 commit intoapache:mainfrom
ZuhairORZaki:Injection-2views.py5783650738349517611.diff

ZuhairORZaki commented Dec 28, 2023

Uh oh!

boring-cyborg bot commented Dec 28, 2023

Uh oh!

potiuk commented Dec 28, 2023

Uh oh!

Taragolis commented Dec 28, 2023

Uh oh!

ZuhairORZaki commented Dec 28, 2023

Uh oh!

ZuhairORZaki commented Dec 28, 2023

Uh oh!

potiuk commented Dec 28, 2023

Uh oh!

Taragolis commented Dec 28, 2023

Uh oh!

ZuhairORZaki commented Dec 28, 2023

Uh oh!

potiuk commented Dec 29, 2023 •

edited

Loading

Uh oh!

potiuk left a comment

Uh oh!

ZuhairORZaki commented Jan 1, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

ZuhairORZaki commented Dec 28, 2023

Exploitable Scenario

Suggested Fix

CLA Requirements:

Sponsorship and Support:

Uh oh!

boring-cyborg bot commented Dec 28, 2023

Uh oh!

potiuk commented Dec 28, 2023

Uh oh!

Taragolis commented Dec 28, 2023

Uh oh!

ZuhairORZaki commented Dec 28, 2023

Uh oh!

ZuhairORZaki commented Dec 28, 2023

Uh oh!

potiuk commented Dec 28, 2023

Uh oh!

Taragolis commented Dec 28, 2023

Uh oh!

ZuhairORZaki commented Dec 28, 2023

Uh oh!

potiuk commented Dec 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

potiuk left a comment

Choose a reason for hiding this comment

Uh oh!

ZuhairORZaki commented Jan 1, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

potiuk commented Dec 29, 2023 •

edited

Loading